Menu

#1000 Pipelined HTTP requests with a zero-length body broken

closed-fixed
libcurl (356)
5
2013-06-21
2011-03-15
Anonymous
No

The scenario:

cURL is running pipelined. It's trying to get a series of three documents from the server, call them http://server/one, two and three. 'one' is zero-length.

What happens:

1. cURL sends all three requests
2. Server sends two replies in quick succession.
3. cURL enters Curl_readwrite --> readwrite_data --> Curl_read
4. We're at the HTTP-read-headers stage, so we don't have an expected length. We read too much, saving the buffer in conn->master_buffer.
5. It's still header-time, so we enter Curl_http_readwrite_headers.
6. The first response's headers indicate Content-Length: 0, so we set "stop_reading"
7. ...and consequently bail out of the do-while loop.

The problem: that buffer saved in conn->master_buffer contains not only the zero-length request's headers, but also the headers and body of its successor requests. However it will never yield them unless we set conn->read_pos using read_rewind, but we only do that if we read a short body, *not* if we set stop_reading, which bails out immediately.

Consequently the responses which were in that buffer are silently dropped, and subsequent requests get paired with replies which are not their own! Example: Requests 1, 2 and 3 meet reply-packets [1, 2] and [3]. Request 1 is zero-length, triggering the bug and silently discarding the rest of its packet. Request 2 is then answered using reply [3]... curse HTTP's lack of a transaction-identifier field!

Solution: read_rewind whenever we've read too much data, not just in the special cases dealt with further down readwrite.

I attach as always a pair of test cases, somewhat tricky this time: a server and a client. They'll prompt you to press enter a few times: the drill should be:

1. Start server ("dumb_server 9000")
2. Start client, no params. We immediately spin up a pipelining, maxconnects=1 multi-context and submit 4 requests, "init", "zero_length", "nonzero_length_one", "nonzero_length_two".
3. We'll immediately submit the first request and the server will answer. So,e purpose is to convince cURL it's dealing with HTTP/1.1 and can pipeline the rest of them.
4. cURL will automatically send the other three as a pipeline and quiesce.
5. Now press return at the server: it'll send a single-packet (hopefully!) reply to the first two requests
6. Press return at cURL's console: it'll consume the first reply and silently eat the second one.
7. Press return at the server: it'll reply to the third request.
8. Press return at the client: it'll consume the third reply (but match it to the second request!) and end.

To check, see /tmp/curl_*_out.

If it works right, you'll get:

curl_zero_out: "init"
curl_first_out: ""
curl_second_out: "nz1"
curl_third_out: "nz2"

If the bug manifests, instead you'll get:

curl_zero_out: "init"
curl_first_out: ""
curl_second_out: "nz2"
curl_third_out: ""

Note that the third request has not returned, whilst the second has been given the value that rightfully belongs to the third.

I'd really appreciate a fix to this one, as I can't find a workaround that isn't disabling pipelining.

Discussion

  • Anonymous

    Anonymous - 2011-03-15
    • milestone: --> hang
    • labels: --> libcurl
     
  • Daniel Stenberg

    Daniel Stenberg - 2011-03-16

    Thanks for the report and analysis. I agree that we want this fixed, but the reality is that we're heavily undermanned in the debugging department of the curl project, as it consists mostly of just me. Pipelining is unfortunately not very widely used by libcurl users so it has this kind of quirks left to smooth away.

    This is just a long way of saying: it may take a little while until I can get to looking into this.

     
  • Daniel Stenberg

    Daniel Stenberg - 2011-03-16
    • assigned_to: nobody --> bagder
     
  • Anonymous

    Anonymous - 2011-04-06

    I finally got around to making a proper patch for this -- the attached includes a test case which fails in trunk but works with this patch applied. It doesn't cause trouble with any other test cases, and I *think* it's fairly safe, as it only does anything against a pipelined handle.

     
  • Anonymous

    Anonymous - 2011-04-06

    D'oh, just realised that patch contains a second hunk against transfer.c which stems from testing your patch to the previous, fixed rewinding-related bug.

     

    Last edit: Anonymous 2015-08-25
  • Daniel Stenberg

    Daniel Stenberg - 2011-04-07
    • status: open --> closed-fixed
     
  • Daniel Stenberg

    Daniel Stenberg - 2011-04-07

    Thanks for the report, this problem is now fixed in the git repository.

    To try it out, you either checkout/update your git clone: http://curl.haxx.se/source.html

    or you try tomorrow's daily snapshot: http://curl.haxx.se/snapshots/