Bugs item #1871269, was opened at 2008-01-14 11:44
Message generated for change (Comment added) made by jmalicki
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=1871269&group_id=976

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: libcurl
Group: hang
Status: Closed
Resolution: Fixed
Priority: 7
Private: No
Submitted By: Joe Malicki (jmalicki)
Assigned to: Daniel Stenberg (bagder)
Summary: Curl hangs when sending large request-headers

Initial Comment:
Using libcurl 7.13.2, 7.15.5 or 7.17.1 with software that sends extremely large (> ~40KB, whatever is greater than one syscall) request-headers as part of a post. The following patch fixes it, passes regression tests the same as the old curl, and has made it through several months of QA integration in a larger project with no issues:

This patch is against 7.15.5, and works with patch "fuzz" against 7.13.2 as well (7.17.1 requires a slightly different patch, still haven't tested fix for 7.17.1):

Index: lib/http.c
===================================================================
--- lib/http.c (revision 67598)
+++ lib/http.c (revision 67599)
@@ -778,7 +778,7 @@
     memcpy(buffer, http->postdata, (size_t)http->postsize);
     fullsize = (size_t)http->postsize;

- if(http->backup.postsize) {
+ if(http->backup.postsize || http->backup.fread) {
       /* move backup data into focus and continue on that */
       http->postdata = http->backup.postdata;
       http->postsize = http->backup.postsize;
@@ -787,6 +787,7 @@

       http->sending++; /* move one step up */

+ http->backup.fread=NULL;
       http->backup.postsize=0;
     }
     else

----------------------------------------------------------------------

>Comment By: Joe Malicki (jmalicki)
Date: 2008-01-14 17:22

Message:
Logged In: YES
user_id=1981241
Originator: YES

Oh, I figured out why it doesn't hang for very large values of headers:

Apache (if that's the webserver you tested against) has by default a
header length limit of 8190, and I'm running against an apache patched to
bump to fieldsize limit up to 32766... if Apache sees a field larger than
that it bounces with an error immediately, not giving curl time to try to
send a message body and hang.

However, a lower fieldsize will cause the headers to be small enough to be
in one send.

The attached testcase should reproduce against a normal apache, with
suggested values in usage message (hopefully it can be of use for making a
unit test).
File Added: curl-large-header-bug3.c

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2008-01-14 17:04

Message:
Logged In: YES
user_id=1110
Originator: NO

Thanks for verifying, I've committed this now and I'll close this report.

That authneg part of current CVS code is due to a fix of a different bug
from december 2007:

http://cool.haxx.se/cvs.cgi/curl/lib/http.c.diff?r1=1.352&r2=1.353

----------------------------------------------------------------------

Comment By: Joe Malicki (jmalicki)
Date: 2008-01-14 16:40

Message:
Logged In: YES
user_id=1981241
Originator: YES

After playing with the test cases more, I can reproduce here reliably with
sending $((20*1024)) or $((30*1024)) on the commandline to the second test,
but _NOT_ with $((100*1024)), so I don't know what the difference is.

However, once I apply your patch (remove the if conn->bits.authneg: 7.17.1
doesn't have that), I can't reproduce the bug with any value.

Thank you so much for your quick help, and for all your work on curl!

----------------------------------------------------------------------

Comment By: Joe Malicki (jmalicki)
Date: 2008-01-14 16:14

Message:
Logged In: YES
user_id=1981241
Originator: YES

That patch makes total sense to me (I didn't originally try to do it that
way because I was afraid of what other ramifications setting http->postsize
might have, so the other way seemed more conservative since it only
specifically fixed this bug and changed no other behavior).

Interesting that it didn't fail for you.... if you strace curl, does it
complete the headers with one send? That is the distinction between when
it fails vs. succeeds... I'm amazed that 200k wasn't large enough, it fails
reliably for me (even at 5000 over ethernet, before I have a grown TCP
window, but 40k over the Linux "lo" interface).. do you just have very
large default TCP windows configured?

----------------------------------------------------------------------

Comment By: Joe Malicki (jmalicki)
Date: 2008-01-14 16:09

Message:
Logged In: YES
user_id=1981241
Originator: YES

Also, I neglected to answer your other question... I believe it's still
necessary against 7.17.1 with patching fread_func instead, but I just
didn't post a patch for that because I haven't tested it yet extensively,
as I've been using 7.15.5 and 7.13.2 (debian etch and sarge curl versions,
respectively) so they got priority.

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2008-01-14 16:08

Message:
Logged In: YES
user_id=1110
Originator: NO

I can't make the problem (hang) happen with your test code even when I
upped the size to 200K headers (for some reason, I agree with your
conclusion). I do think your second attempt of a fix is correct, although I
would put it within the following condition as well as there's no point in
doing that assingment while in "auth negotiation". Alas, my adjusted
version of your patch is (made against current CVS):

--- lib/http.c 10 Jan 2008 10:30:20 -0000 1.356
+++ lib/http.c 14 Jan 2008 21:04:58 -0000
@@ -2705,8 +2705,10 @@
           /* set the pointer to mark that we will send the post body
using the
              read callback, but only if we're not in authenticate
              negotiation */
- if(!conn->bits.authneg)
+ if(!conn->bits.authneg) {
             http->postdata = (char *)&http->postdata;
+ http->postsize = postsize;
+ }
         }
       }
       /* issue the request */

What do you think?

----------------------------------------------------------------------

Comment By: Joe Malicki (jmalicki)
Date: 2008-01-14 16:05

Message:
Logged In: YES
user_id=1981241
Originator: YES

(Oh, I neglected to mention that if it doesn't fail, try bumping up the
size parameter passed into get_big_header()... the following updated
testcase takes it as a commandline argument for easier testing). Sorry for
not doing that earlier, I've been sitting on getting this to you for awhile
and I forgot some of the details temporarily.
File Added: curl-large-header-bug2.c

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2008-01-14 15:53

Message:
Logged In: YES
user_id=1110
Originator: NO

Sorry, I must be blind. I'll take it for a spin right away! Comments will
follow...

----------------------------------------------------------------------

Comment By: Joe Malicki (jmalicki)
Date: 2008-01-14 15:49

Message:
Logged In: YES
user_id=1981241
Originator: YES

If you notice, I attached a testcase that fails... does it not fail for
you?

In this testcase, it is clearly posting data. The reason why
http->backup.postsize is 0 is
that http->postsize is only ever initially set in curl 7.17.1 http.c:2643,
which is only
if CURLOPT_POSTFIELDS is set. Because I'm writing the posts with a
callback, this value is
never set (perhaps this is the real bug? The patch I posted was that way
because it was less intrusive).

I provided a testcase (curl-large-header-bug.c) so it could be reproduced
with code rather than description.

Are you proposing that the attached patch is rather the correct one? (it
is untested)

File Added: curl-large-headers2.diff

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2008-01-14 15:34

Message:
Logged In: YES
user_id=1110
Originator: NO

Can you please be more specific on as exact details as possible on what
you do to get this problem so that I can work on writing up a test case for
this?

I don't understand the patch. If http->backup.postsize is zero, then what
exactly is libcurl going to send when that condition equals true?

Also, while the 'fread' is now 'fread_func' in recent libcurls, how come
the patch isn't needed the exact same way there? I can't recall any
particular fixes in this area in a very long time?

----------------------------------------------------------------------

Comment By: Joe Malicki (jmalicki)
Date: 2008-01-14 11:45

Message:
Logged In: YES
user_id=1981241
Originator: YES

Above patch for 7.15.5 attached as a file.
File Added: curl-large-headers.diff

----------------------------------------------------------------------

You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=1871269&group_id=976
Received on 2008-01-14