libcurl writes body if CURLOPT_NOBODY but no server headers #973

Closed
jay opened this Issue Aug 21, 2016 · 4 comments

Projects

None yet

2 participants

@jay
Member
jay commented Aug 21, 2016 edited

I did this

From the mailing list: Crash in curl library while processing HTTP HEAD response

The thread is a hard read due to the top-posting. The reporter experiences a crash when using CURLOPT_NOBODY in an HTTP request and a server improperly replies. libcurl defaults to body when it encounters an improper reply and attempts to write it using the body function. That doesn't seem right for this case. CURLOPT_NOBODY doc says "Enabling this option means asking for a download but without a body." Also see Daniel's reply.

This shouldn't cause a crash. We don't yet know why the reporter's app is crashing but it's probably something to do with the app not expecting body data.

server:

while true; do printf adfadfadfd | nc -4l localhost 8000; done

client:

curl -v -I http://localhost:8000

output:

> HEAD / HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.50.2-DEV
> Accept: */*
>
* STATE: DO => DO_DONE handle 0x961aa8; line 1653 (connection #0)
* STATE: DO_DONE => WAITPERFORM handle 0x961aa8; line 1780 (connection #0)
* STATE: WAITPERFORM => PERFORM handle 0x961aa8; line 1790 (connection #0)
* Increasing bytecount by 10 from hbuflen
adfadfadfd* nread <= 0, server closed connection, bailing
* STATE: PERFORM => DONE handle 0x961aa8; line 1950 (connection #0)
* multi_done
* Connection #0 to host localhost left intact
* Expire cleared

possible patch

I have a feeling I might be cutting in too early or too late but before we write any body check the userdefined for opt_no_body and in that case close the connection and return:

diff --git a/lib/transfer.c b/lib/transfer.c
index e4a2835..ee54d5f 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -534,6 +534,13 @@ static CURLcode readwrite_data(struct Curl_easy *data,
        is non-headers. */
     if(k->str && !k->header && (nread > 0 || is_empty_data)) {

+      if(data->set.opt_no_body) {
+        connclose(conn, "ignoring body");
+        k->keepon &= ~KEEP_RECV;
+        *done = TRUE;
+        return CURLE_OK;
+      }
+
 #ifndef CURL_DISABLE_HTTP
       if(0 == k->bodywrites && !is_empty_data) {
         /* These checks are only made the first time we are about to

That will actually cause a return of CURLE_GOT_NOTHING, but we did get something so does that seem misleading?

> HEAD / HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.50.2-DEV
> Accept: */*
>
* STATE: DO => DO_DONE handle 0xa91aa8; line 1653 (connection #0)
* STATE: DO_DONE => WAITPERFORM handle 0xa91aa8; line 1780 (connection #0)
* STATE: WAITPERFORM => PERFORM handle 0xa91aa8; line 1790 (connection #0)
* STATE: PERFORM => DONE handle 0xa91aa8; line 1950 (connection #0)
* multi_done
* Empty reply from server
* Closing connection 0
* The cache now contains 0 members
* Expire cleared
curl: (52) Empty reply from server

curl/libcurl version

libcurl/7.50.2-DEV

operating system

Windows 7 x64 Enterprise

@bagder
Member
bagder commented Aug 22, 2016

That will actually cause a return of CURLE_GOT_NOTHING, but we did get something so does that seem misleading?

Yes, it seems to not be true. unless we modify the meaning (and docs) slightly and make it mean "got nothing complying with the protocol". I think the key here might be to add an infof() line that explains the action.

I suppose CURLE_RECV_ERROR is the only other semi-decent return code for this error case. Even that would need an infof() for users to full grasp what happened.

I don't feel strongly either way actually.

@bagder bagder added the HTTP label Aug 22, 2016
@jay
Member
jay commented Aug 22, 2016

I don't like recv error that's more misleading than got nothing. CURLE_FTP_WEIRD_SERVER_REPLY ? also see #975 for proposed alias CURLE_WEIRD_SERVER_REPLY

@bagder
Member
bagder commented Sep 9, 2016

Here's my adaptation of what you started @jay: 0001-http-refuse-to-pass-on-response-body-with-NO_NODY-wa.patch, including a new test case.

@bagder
Member
bagder commented Sep 11, 2016

Oops, there's a test 1144 that I should update instead of adding a new one since that fails with this patch applied.

@bagder bagder added a commit that closed this issue Sep 11, 2016
@bagder bagder http: refuse to pass on response body with NO_NODY was set
... like when a HTTP/0.9 response comes back without any headers at all
and just a body this now prevents that body from being sent to the
callback etc.

Adapted test 1144 to verify.

Fixes #973

Assisted-by: Ray Satiro
a8e751a
@bagder bagder closed this in a8e751a Sep 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment