Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

libcurl writes body if CURLOPT_NOBODY but no server headers #973

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

libcurl writes body if CURLOPT_NOBODY but no server headers #973

jay opened this issue Aug 21, 2016 · 4 comments
Labels
HTTP

Comments

@jay
Copy link
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
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
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 closed this as completed in a8e751a Sep 11, 2016
@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
HTTP
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bagder @jay