curl_multi_remove_handle may cause invalid memory access when configure with --enable-debug #1329

Closed
zelinchen opened this Issue Mar 16, 2017 · 2 comments

Projects

None yet

2 participants

@zelinchen
zelinchen commented Mar 16, 2017 edited

I did this

Configure libcurl with "--enable-debug", then use curl_multi_remove_handle() to cancel multiple long pull requests, such as follows:

curl_multi_remove_handle(multi_handle, easy);
curl_easy_cleanup(easy);
curl_multi_remove_handle(multi_handle, easy_2);
curl_easy_cleanup(easy_2);

I got such error with valgrind:

== Info: http2_recv: easy 0x45aa5e4 (stream 3)
== Info: Kill stream: Removed with partial response
== Info: multi_done
== Info: free header_recvbuf!!
== Info: Connection still in use, no more multi_done now!
==8061== Invalid read of size 1
==8061==    at 0x404A21B: Curl_infof (sendf.c:225)
==8061==    by 0x40638D5: Curl_conncontrol (connect.c:1399)
==8061==    by 0x4065409: curl_multi_remove_handle (multi.c:701)
==8061==    by 0x8049159: main (curl_server_push_test.c:327)
==8061==  Address 0x45a1cec is 836 bytes inside a block of size 18,492 free'd
==8061==    at 0x402C06C: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==8061==    by 0x4060BB5: curl_dofree (memdebug.c:333)
==8061==    by 0x404B82D: Curl_close (url.c:485)
==8061==    by 0x405EC21: curl_easy_cleanup (easy.c:833)
==8061==    by 0x8049142: main (curl_server_push_test.c:326)
==8061==
==8061== Invalid read of size 1
==8061==    at 0x404AE53: Curl_debug (sendf.c:808)
==8061==    by 0x404A286: Curl_infof (sendf.c:233)
==8061==    by 0x40638D5: Curl_conncontrol (connect.c:1399)
==8061==    by 0x4065409: curl_multi_remove_handle (multi.c:701)
==8061==    by 0x8049159: main (curl_server_push_test.c:327)
==8061==  Address 0x45a1cd4 is 812 bytes inside a block of size 18,492 free'd
==8061==    at 0x402C06C: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==8061==    by 0x4060BB5: curl_dofree (memdebug.c:333)
==8061==    by 0x404B82D: Curl_close (url.c:485)
==8061==    by 0x405EC21: curl_easy_cleanup (easy.c:833)
==8061==    by 0x8049142: main (curl_server_push_test.c:326)
==8061==
==8061== Invalid read of size 4
==8061==    at 0x404AD88: showit (sendf.c:780)
==8061==    by 0x404AF83: Curl_debug (sendf.c:837)
==8061==    by 0x404A286: Curl_infof (sendf.c:233)
==8061==    by 0x40638D5: Curl_conncontrol (connect.c:1399)
==8061==    by 0x4065409: curl_multi_remove_handle (multi.c:701)
==8061==    by 0x8049159: main (curl_server_push_test.c:327)
==8061==  Address 0x45a1b28 is 384 bytes inside a block of size 18,492 free'd
==8061==    at 0x402C06C: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==8061==    by 0x4060BB5: curl_dofree (memdebug.c:333)
==8061==    by 0x404B82D: Curl_close (url.c:485)
==8061==    by 0x405EC21: curl_easy_cleanup (easy.c:833)
==8061==    by 0x8049142: main (curl_server_push_test.c:326)
==8061==
==8061== Invalid read of size 4
==8061==    at 0x404AD95: showit (sendf.c:781)
==8061==    by 0x404AF83: Curl_debug (sendf.c:837)
==8061==    by 0x404A286: Curl_infof (sendf.c:233)
==8061==    by 0x40638D5: Curl_conncontrol (connect.c:1399)
==8061==    by 0x4065409: curl_multi_remove_handle (multi.c:701)
==8061==    by 0x8049159: main (curl_server_push_test.c:327)
==8061==  Address 0x45a1b28 is 384 bytes inside a block of size 18,492 free'd
==8061==    at 0x402C06C: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==8061==    by 0x4060BB5: curl_dofree (memdebug.c:333)
==8061==    by 0x404B82D: Curl_close (url.c:485)
==8061==    by 0x405EC21: curl_easy_cleanup (easy.c:833)
==8061==    by 0x8049142: main (curl_server_push_test.c:326)
==8061==
==8061== Invalid read of size 4
==8061==    at 0x404AD9E: showit (sendf.c:781)
==8061==    by 0x404AF83: Curl_debug (sendf.c:837)
==8061==    by 0x404A286: Curl_infof (sendf.c:233)
==8061==    by 0x40638D5: Curl_conncontrol (connect.c:1399)
==8061==    by 0x4065409: curl_multi_remove_handle (multi.c:701)
==8061==    by 0x8049159: main (curl_server_push_test.c:327)
==8061==  Address 0x45a1ab8 is 272 bytes inside a block of size 18,492 free'd
==8061==    at 0x402C06C: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==8061==    by 0x4060BB5: curl_dofree (memdebug.c:333)
==8061==    by 0x404B82D: Curl_close (url.c:485)
==8061==    by 0x405EC21: curl_easy_cleanup (easy.c:833)
==8061==    by 0x8049142: main (curl_server_push_test.c:326)
==8061==
== Info: Kill stream: Removed with partial response
== Info: Expire cleared
== Info: multi_done
== Info: free header_recvbuf!!
== Info: HTTP/2 DISCONNECT starts now
== Info: HTTP/2 DISCONNECT done
== Info: Closing connection 0
== Info: The cache now contains 0 members
=> Send SSL data, 5 bytes (0x5)
0000: .....
== Info: TLSv1.2 (OUT), TLS alert, Client hello (1):
=> Send SSL data, 2 bytes (0x2)
0000: ..

If I do it in this way, then no complain about "Invalid read"

curl_multi_remove_handle(multi_handle, easy);
curl_multi_remove_handle(multi_handle, easy_2);
curl_easy_cleanup(easy);
curl_easy_cleanup(easy_2);

Looks like curl_multi_remove_handle() doesn't fully remove the association between the easy handle and the connection object for long-pull request(or request with partial response). Somehow the "data" member in connection object still refer to the removed easy handle.

Attachment is simple test app to reproduce this issue. It use 2 easy handle to do long-pull request from "https://http2.golang.org/clockstream".

(Note that the test app is modified from "https://curl.haxx.se/libcurl/c/http2-serverpush.html" with server push disabled.)

I expected the following

Once easy handle is removed by curl_multi_remove_handle(), the multi-handle should not refer to this easy handle any more, so that I can cleanup this easy handle if required.

curl/libcurl version

curl-7.53.1

operating system

Ubuntu-12.04

@bagder bagder self-assigned this Mar 16, 2017
Owner
bagder commented Mar 21, 2017

Thanks for the lovely test case, I can reproduce this using it.

Owner
bagder commented Mar 21, 2017

I'll push this fix in a few minutes. Fixes the problem for me:

diff --git a/lib/multi.c b/lib/multi.c
index f16b594e0..6cdba393e 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -693,17 +693,17 @@ CURLMcode curl_multi_remove_handle(struct Curl_multi *multi,
   }
 
   if(data->easy_conn &&
      data->mstate > CURLM_STATE_DO &&
      data->mstate < CURLM_STATE_COMPLETED) {
+    /* Set connection owner so that the DONE function closes it.  We can
+       safely do this here since connection is killed. */
+    data->easy_conn->data = easy;
     /* If the handle is in a pipeline and has started sending off its
        request but not received its response yet, we need to close
        connection. */
     streamclose(data->easy_conn, "Removed with partial response");
-    /* Set connection owner so that the DONE function closes it.  We can
-       safely do this here since connection is killed. */
-    data->easy_conn->data = easy;
     easy_owns_conn = TRUE;
   }
 
   /* The timer must be shut down before data->multi is set to NULL,
      else the timenode will remain in the splay tree after
@bagder bagder added the crash label Mar 21, 2017
@bagder bagder added a commit that closed this issue Mar 21, 2017
@bagder bagder multi: fix streamclose() crash in debug mode
The code would refer to the wrong data pointer. Only debug builds do
this - for verbosity.

Reported-by: zelinchen@users.noreply.github.com
Fixes #1329
6e0f26c
@bagder bagder closed this in 6e0f26c Mar 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment