curl_multi_remove_handle crash after enabling http2 server push #1249

Closed
zelinchen opened this Issue Feb 6, 2017 · 2 comments

Projects

None yet

2 participants

@zelinchen

I did this

Attachment curl_server_push_test.zip is code modified from https://curl.haxx.se/libcurl/c/http2-serverpush.html.
The difference is that, it will remove easy handle before transfer is finished, instead of waiting for "CURLMSG_DONE".
In this case, it cause crash within curl_multi_remove_handle().

Here are part of the trace infromation from valgrind:

==23947== Process terminating with default action of signal 11 (SIGSEGV)
==23947==  Access not within mapped region at address 0xC
==23947==    at 0x40922B4: http2_disconnect (http2.c:137)
==23947==    by 0x405A1D6: Curl_disconnect (url.c:3024)
==23947==    by 0x40741FE: multi_done (multi.c:626)
==23947==    by 0x407440B: curl_multi_remove_handle (multi.c:727)
==23947==    by 0x804930D: main (curl_server_push_test.c:332)
==23947==  If you believe this happened as a result of a stack
==23947==  overflow in your program's main thread (unlikely but
==23947==  possible), you can try to increase the size of the
==23947==  main thread stack using the --main-stacksize= flag.
==23947==  The main thread stack size used in this run was 16777216.

I expected the following

No crash.

curl/libcurl version

curl-7.52.1

operating system

Ubuntu-12.0.5

@zelinchen

Here is the quick patch, works fine for me:

--- a/lib/http2.c       2016-12-19 15:27:56.000000000 +0800
+++ b/lib/http2.c       2017-02-07 10:11:20.882128999 +0800
@@ -415,6 +415,7 @@ static int push_promise(struct Curl_easy
       free(stream->push_headers[i]);
     free(stream->push_headers);
     stream->push_headers = NULL;
+    stream->push_headers_used = 0;

     if(rv) {
       /* denied, kill off the new handle again */
@bagder bagder added a commit that closed this issue Feb 7, 2017
@bagder bagder http2: reset push header counter fixes crash
When removing an easy handler from a multi before it completed its
transfer, and it had pushed streams, it would segfault due to the pushed
counted not being cleared.

Fixed-by: zelinchen@users.noreply.github.com
Fixes #1249
d836123
@bagder bagder closed this in d836123 Feb 7, 2017
@bagder
Member
bagder commented Feb 7, 2017

Thanks! The fix is perfectly sensible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment