Crash with CURLOPT_READFUNCTION + CURLAUTH_ANYSAFE #346
Comments
|
Sorry, cannot repeat. I converted your code into plain C and used the following verbatim with no valgrind warnings and no memory leaks. Can you tell me what I need to change to make it crash? What libcurl version are you using? |
|
Hi, I'm using github's master curl. Yes, the C program doesn't crash for me, you need to use the C++ program as-is. So undefined behaviour is going to be difficult to nail down, Use your C code and run it. I'm pointing it to a server that only accepts DIGEST auth (and refuses to auth the user). The output I get from your test C code is below. If I change the CURLFORM_STREAM to (void*)0 then the content length prints as zero (as it should). Does it behave that way for you? $ gcc -g -ggdb -I~/build_curl/install/include -o test test.c -rdynamic ~/build_curl/install/lib/libcurl.so -Wl,-rpath,~/build_curl/install/lib
$ ./test
* Trying THE_IP...
* Connected to THE_HOST (THE_IP) port 80 (#0)
> POST /A_FORM HTTP/1.1
Host: THE_HOST
Accept: */*
Content-Length: 140733193388192
Content-Type: multipart/form-data; boundary=------------------------cc3b722562ca287e
< HTTP/1.1 413 Request Entity Too Large
< Connection: close
< Content-Length: 0
< Date: Mon, 20 Jul 2015 02:04:55 GMT
< Server: lighttpd/1.4.19
<
* Closing connection 0 |
Tested master (aab76af 2015-07-18) in Ubuntu and Windows. The content length issue is reproducible in Ubuntu 14 x64 LTS. The content-length varies each run I assume it's uninitialized. For example: Initial impression- Windows I couldn't reproduce. I used Visual Studio 2010 project file and tried |
|
valgrind warnings went away when I used
Should probably be changed to 0L and oveview should probably have something in bold like "if a parameter requires a long you must pass a Also, from CURLFORM_STREAM:
That should be added to CURLFORM_CONTENTSLENGTH, basically something explicit that states 0L is not valid for stream. Possibly none of this may help with your original issue though. |
|
On second thought why wouldn't |
|
Right, the varargs functions need 'long' for the numerals. Like 0L and not just 0. With that fixed, do you still get this problem @paulharris ? I tried sending my post to a page returning 401 due to bad auth but I still haven't triggered any crash. |
|
This is now believed to be fixed, closing |
|
I didn't fix this though. I think the fix would be somehow documenting that in the case of CURLFORM_STREAM a content length of 0 is taken to be the length (again I'm assuming this is acceptable, I need clarification). How about instead of what I mentioned prior I just preface it with "If you are not using CURLFORM_STREAM " which I think gets the same point across:
Related discussion in 4673094 to better warn of type adherence; eg 0L not 0 and I will continue that there. |
|
Hi guys, I have this earmarked for me to revisit in the next week or two, just a bit busy at the moment. I dislike the whole varargs 0 vs 0L thing, it is a very easy trap to fall into. curl_formadd_name(&formpost, &lastptr,"file");
curl_formadd_stream(&formpost, &lastptr,(void*)0x121);
curl_formadd_contentslength(&formpost, &lastptr, 0); // note: no need for 0L
curl_formadd_filename(&formpost, &lastptr,fn);
where in the new header, eg:
inline void curl_formadd_contentslength( whatnot * formpost, whatnot * lastptr, long n)
{
curl_formadd(formpost, lastptr,
CURLFORM_CONTENTSLENGTH, n,
CURLFORM_END
);
}This is far more robust and a safer call, especially for new users... Back to work, I'll try and double-check the bug fix suggestion soon. |
|
The way to build multipart formposts can certainly be made into something that is easier to use with less risks of accidentally using the wrong type, and what you're suggesting seems like a fine approach. If you're willing to work on it, I'll certainly help out to review. |
|
I'll see what I come up with when I revisit, but I won't be able to do full test coverage, and I may not get all the APIs right. I have only starting using libarchive so level of code completeness can't be expected from such a contributor. But hopefully I can give you something to get the ball rolling. |
|
Every little bit counts! |
|
Not sure if anyone is still working on this issue, but I think I've found the cause: Commit b0143a2 removed Before the commit, the user callback ( This leads to the issue that @paulharris found: the user callback is switched to the standard version, but called with user-provided read-data pointer. The valgrind output tells the same story as well:
A quick fix is to cache both |
|
This sounds a bit complicated but it feels like you're on the right track. Can you figure out a use case that actually triggers this problem? @chubiei, can you show your suggested fix as a patch? |
|
Hello @bagder, Although the code execution path has been identified, but I don't have a way to 100% reproduce the issue. In my company, the issue can be occasionally reproduced using Baidu PCS API to upload a large file to Baidu server with chunks. The patch below is a temporary fix that I mentioned earlier based on the method used in db6ff22 and has been committed to our company's repository. Personally I think a better way of fixing the issue is to extend the
The suggestion above is quite long, if you need me to write it as a patch, please let me know and I'll submit to you later as I am not that familiar with open source code contributing. diff --git a/lib/http.c b/lib/http.c
index fe2c2ca..6008029 100644
--- a/lib/http.c
+++ b/lib/http.c
@@ -1486,6 +1486,17 @@ CURLcode Curl_http_done(struct connectdata *conn,
#endif
/* set the proper values (possibly modified on POST) */
+ if (http->curl_7_43_backup.fread_func) {
+ data->set.fread_func = http->curl_7_43_backup.fread_func; /* restore */
+ http->curl_7_43_backup.fread_func = NULL;
+ }
+ if (http->curl_7_43_backup.fread_in) {
+ data->set.in = http->curl_7_43_backup.fread_in; /* restore */
+ http->curl_7_43_backup.fread_in = NULL;
+ }
+
conn->seek_func = data->set.seek_func; /* restore */
conn->seek_client = data->set.seek_client; /* restore */
@@ -2453,6 +2464,11 @@ CURLcode Curl_http(struct connectdata *conn, bool *done)
stream. */
http->form.fread_func = data->set.fread_func;
+ /* backup fread_func and in, will be restored on done */
+ http->curl_7_43_backup.fread_func = data->set.fread_func;
+ http->curl_7_43_backup.fread_in = data->set.in;
+
/* Set the read function to read from the generated form data */
data->set.fread_func = (curl_read_callback)Curl_FormReader;
data->set.in = &http->form;
diff --git a/lib/http.h b/lib/http.h
index 415be39..0437753 100644
--- a/lib/http.h
+++ b/lib/http.h
@@ -144,6 +144,13 @@ struct HTTP {
curl_off_t postsize;
} backup;
+
+ // FIXME: fix curl 7.43 update regression, issue #346
+ struct fix_curl_7_43_back {
+ curl_read_callback fread_func;
+ void *fread_in;
+ } curl_7_43_backup;
+
enum {
HTTPSEND_NADA, /* init */
HTTPSEND_REQUEST, /* sending a request */ |
|
Okay, yeah I can certainly see how it never restoring the backup there is a bad idea! But I would like to go for another fix. Namely to stop changing the 'set' struct member, which was always a bad idea. My larger patch can be found here: http://pastebin.com/raw.php?i=i5EbHLwH It runs through all my tests fine. |
|
Thanks for the quick fix! But I am afraid that this commit will not fix this issue. The main reason is that |
|
So you can repeat the crash even with my patch? The point is: what exactly is the purpose of restoring those values after the request is done? Answer: for the subsequent request. And my patch now sets the values unconditionally in the beginning of every request. I would imagine that it would remove the need for restoring them. Unless you've figured out another reason we need to restore them? |
|
Hi guys, Sorry I've been unresponsive, I've been working on another part of the project for a while, I just want to quickly throw in a thought to check up on. The crash for me was because the request would be automatically replayed when authentication failed in a POST transfer. So its not a new Request from the library user's end, its actually Curl that is re-executing the Request. Does your patch correctly set those values prior to the automatic re-request? (I can't tell from looking at the patch) cheers, |
|
@paulharris ah, right, thanks for that feedback. No, I added the init code in the wrong spot as this now only sets it up correctly before everything starts. I'll move that piece of code and post an update. |
|
I created a temporary branch for playing with the fix for this (see issue-346). Here's what I suggest that now sets the pointers correctly for every new HTTP request during a single easy "transfer": 263192f35b77b Thoughts? |
... and assign it from the set.fread_func_set pointer in the Curl_init_CONNECT function. This A) avoids that we have code that assigns fields in the 'set' struct (which we always knew was bad) and more importantly B) it makes it impossibly to accidentally leave the wrong value for when the handle is re-used etc. Introducing a state-init functionality in multi.c, so that we can set a specific function to get called when we enter a state. The Curl_init_CONNECT is thus called when switching to the CONNECT state. Bug: #346
|
I can't comment on the patch as I'm not familiar with the code, On 8 October 2015 at 21:21, Daniel Stenberg notifications@github.com
|
|
The easy transfer function is just a thin layer built on top of the multi interface, so internally there really isn't any difference in which way you use to drive the transfer. It runs the same code. My hope with posting the patch/branch here is that some of you who have actually seen this crash happen and can repeat it, would try the patch and tell me if it fixes the problem for you. The patch works and runs all existing test cases fine in my end. I'll try to spend some more time writing up a test case that can reproduce the original crash. |
|
okay, then I'll move to merge my fix and close this issue, and if anything still remains will take it up then |
... and assign it from the set.fread_func_set pointer in the Curl_init_CONNECT function. This A) avoids that we have code that assigns fields in the 'set' struct (which we always knew was bad) and more importantly B) it makes it impossibly to accidentally leave the wrong value for when the handle is re-used etc. Introducing a state-init functionality in multi.c, so that we can set a specific function to get called when we enter a state. The Curl_init_CONNECT is thus called when switching to the CONNECT state. Bug: #346
|
I finally had a chance to test the fix. |
The good news is I narrowed it down.
The "ANYSAFE" auth method can cause a form to be reposted from scratch.
In this situation, the user-provided readfunction callback is switched to the standard fread callback, and then called with the user-provided readdata pointer, which the fread callback is incorrectly casting and writing to.
Valgrind also has a heap of warnings.
First, is the gdb output showing when the fread_func is reset, and then called.
Note: readstream is my user callback function.
Curl_FormReader is the built-in callback function
Curl_FormReader casts my user callback data 0x121 and then tries to access it.
Then, the code.
It has lots of odd variables and padding, I used them to force a segfault.
I believe your compiler may not crash in the same way, so I'm attaching a lot of output.
Then, GDB and Valgrind output
The code:
#include <curl/curl.h> #include <string> using std::string; const char* BASE_URL2 = "http://mysite?action=put"; const string BASE_URL = "http://mysite?action=put"; const string SOMETHING = "1"; static size_t readcallback(char *buffer, size_t size, size_t nitems, void *instream) { return 0; } struct SomeStruct { SomeStruct( string const& url ) { } }; int main(int argc, char **argv) { struct curl_httppost *formpost = NULL; struct curl_httppost *lastptr = NULL; CURL * handle = NULL; curl_slist * headerlist = NULL; const char* fn = "filename"; char space2[17]; // 17 is the sweet spot char space[8+8+8]; SomeStruct curl(BASE_URL2); curl_global_init(CURL_GLOBAL_ALL); handle = (curl_easy_init()); headerlist = (curl_slist_append(NULL,"Expect:")); // we don't want "Expect: 100-continue" -- it screws up proxy curl_easy_setopt(handle, CURLOPT_HTTPHEADER, headerlist); curl_easy_setopt(handle, CURLOPT_URL, BASE_URL2); curl_easy_setopt(handle, CURLOPT_READFUNCTION, readcallback); // authentication (at the server end) curl_easy_setopt(handle, CURLOPT_HTTPAUTH, CURLAUTH_ANYSAFE); curl_easy_setopt(handle, CURLOPT_USERNAME, ""); curl_easy_setopt(handle, CURLOPT_PASSWORD, ""); curl_formadd( &formpost, &lastptr, CURLFORM_COPYNAME, "file", CURLFORM_STREAM, (void*)0x121, CURLFORM_CONTENTSLENGTH, 0,//reader_data.size, CURLFORM_FILENAME, fn, CURLFORM_END ); curl_easy_setopt(handle, CURLOPT_HTTPPOST, formpost); curl_easy_perform(handle); // should die by now return 0; }The GDB output:
The valgrind output:
The text was updated successfully, but these errors were encountered: