ext Daniel Stenberg wrote:
> On Thu, 24 Apr 2008, Jari Urpalainen wrote:
>
>>> Yeah, a new test for that would be great!
>>
>> thanks and sorry if i'm barking at the wrong item,
>
> Could you give it a shot and try to write up a modified test case that
> repeats your reported flaw?
>
Sorry Daniel, not a test (at least yet) but attached the second proposal
to fix the bug. So I've been studying the case (and the code) more
thoroughly and have finally located the culprit (and yes it is indeed
simple once you really get it). So this is about a race condition
between writing and reading. So when you have a body and you write it to
the server within a "digest session" and the server declines the request
by a 401 response (stale session), the keepon flag will stay with the
KEEP_WRITE bit set if the read stream is not read again second time
(small body which fits into a single writebuffer) before interpreting
the 401 response. So if the keepon flag is not cleared, you write again
the body to the socket before a "real" retry and the beginning of the
request (headers etc.) comes then afterwards. A rewind happens in
Curl_http_auth_act and once you are in a write mode, you repeat the body
writing from the beginning. So this flag can easily be cleared (and the
bug fixed) by requesting the eof situation of the read stream after
sending, i.e. you don't need to make a second read stream request which
responds with zero bytes (== eof situation). So there's not a race
possibility then since the server waits for the full body arrival before
sending a response (at least when the _server_ acts properly). If the
server is slow at responding the current (libcurl) client may be able to
come back from the select loop so that it reads the input stream (with
zero bytes) before reading the request response information, and thus
this bug doesn't (always) appear.

This is what the proposed patch is about (also removed the "obfuscated"
writedone boolean variable, leftover ?). Certainly there are other (and
many) possibilities for fixing this and there are pros/cons in each of
them, but I just want to get this fixed in _some_ reasonable way...
The current code breaks pretty deterministically if you put some sleep
time after Curl_write in this kind of scenario.

Btw. if these two lines are added after Curl_http_auth_act() in
Curl_readwrite, it will also fix this bug (though i still seem to prefer
EOF testing instead):
        if (k->writebytecount && (401 == k->httpcode || 407 ==
k->httpcode))
            k->keepon &= ~KEEP_WRITE; /* disable writing before retry */

But as always, it's the maintainers responsibility (and duty imho) to
select the proper fixes ;-)

br, Jari