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

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
Closed

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

zelinchen opened this issue Mar 16, 2017 · 2 comments
Assignees
@bagder
Labels
crash

Comments

@zelinchen
Copy link

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

bagder commented Mar 21, 2017

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

@bagder
Copy link
Member

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 closed this as completed in 6e0f26c Mar 21, 2017
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bagder bagder

Labels
crash
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bagder @zelinchen