On Mon, Jun 13, 2011 at 1:15 PM, Daniel Stenberg <daniel_at_haxx.se> wrote:

> On Mon, 13 Jun 2011, Aaron Orenstein wrote:
>
> Scratch that. I instead suggest the following patch, can you check if this
>>> fixes the problem you experience?
>>>
>>
>> Yes - that fixes it - although it seems a bit heavy-handed to me. I
>> prefer the way it was where the function that opened the file was the one
>> that (was supposed to) close it as well.
>>
>
> I agree about that as a general principle, but I was mostly interested in
> making sure I understood and covered your error case. I still don't quite
> underst during which situation readfromfile() never gets a zero return code
> from fread() and thus closes the file.
>
>
>
>

The problem is that fread() is returning a value which is greater than 0 but
less than size. As a result readformfile() isn't closing the file but since
the return value is smaller than size then Curl_FormReader() thinks it's
done.

> I still think the "nread > size" is incorrect - under what circumstance
>> (other than a misbehaving OS) would nread be larger than the requested read?
>>
>
> Look at the code again and see how it *might* call the read callback
> instead of fread() and the user provided callback can indeed return a bigger
> value.
>
>
So - you're okay with the callback overwriting the buffer and scribbling all
over memory? Because if the return value is the number of bytes read then
that's what it's doing.

> On the other hand a short read ("nread < size") is expected - and matches
>> the given comment.
>>
>
> "man fread" says:
>
> Upon successful completion, fread() shall return the number of
> elements
> successfully read which is less than nitems only if a read error
> or
> end-of-file is encountered.
>
> So for the fread() case, which I believe is what you're using, the
> condition seems to be correct to me as it does indeed trap the final
> fread(). Doesn't it?
>
>
No. The current code says:

formdata.c_at_1423: if(!nread || nread > size)

Let's say you do an fread() of 16 bytes and there are only 8 bytes left in
the file. fread() will return 8. (!8 || 8 > 16) is false so therefore the
cleanup won't happen. This is exactly what I'm saying the bug is - the
check should be that if nread < size (or nread != size) then you should
assume that the file is done and do the cleanup.

> The flaw that I can find is that it treats the read callback return code
> the same way, and we haven't documented this behavior/restriction anywhere.
> Possibly this is reason enough to always do fread/callback until a zero is
> returned and not bail out on short reads.
>
>
> You can test the problem without a windows machine by using a debugger and
>> breaking at the fclose() line and noting that with the given test case
>> fclose() is never called. Stepping through the code should give you a good
>> idea of what is going on.
>>
>
> I don't think it's that simple to repeat. If your error case would trigger
> the problem that easily then why doesn't unit test 1308 leak a FILE *?
> https://github.com/bagder/curl/blob/master/tests/unit/unit1308.c
>
>

It's very simple to repeat - just use the test case I gave you. It won't
assert() on unix because (IIRC) unix allows you to unlink open files - but
it's certainly easy enough to trace through what's going on in a debugger.
I did it about 10 times prepping the report. I just did it again checking
your patch. It really IS that simple to repeat.

For that unit test maybe I missing something but:

(a) I don't see where in unit1308.c it uses a CURLFORM_FILE (which is a
necessary case to reproduce the error)

(b) where does it ensure that you didn't leak a FILE*? Is there some
checking in UNITTEST_STOP?

- Aaron

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2011-06-13