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

Double-free after curl_easy_pause called from curl_multi_closed socket callback #4563

Closed
monkeygroover opened this issue Nov 6, 2019 · 7 comments
Assignees
Labels

Comments

@monkeygroover
Copy link

@monkeygroover monkeygroover commented Nov 6, 2019

As reported on the mailing list:

We are in the process of upgrading an existing application from curl 7.51.0 and have discovered a double free issue, we are not sure if this is an unintentional consequence of a curl change, or just that we have been handling this wrong all along.

Some background; we have a socket callback function that contains the following code (the intention is to ensure we correctly handle transfers left if we are paused when the socket closes). It is being invoked from curl_multi_closed when we see the issue.

 if (what == CURL_POLL_REMOVE) {
     http::Transfer *t;
     curl_easy_getinfo(e, CURLINFO_PRIVATE, &t);
     assert(t);
     if (!t->finished) {
         // Make sure paused transfers complete
         curl_easy_pause(e, CURLPAUSE_CONT);
     }
     ...

This has apparently been working fine for several years, however the following change causes us a problem 26d3d23

The addition of the call to Curl_updatesocket(data) in curl_easy_pause results in Curl_hash_destroy being triggered, but immediately after the socket callback completes we hit this line:

curl/lib/multi.c

Line 2455 in 26d3d23

sh_delentry(&multi->sockhash, s);

which also results in a call to Curl_hash_destroy and we see a double free. So, should we be doing this differently? or was this an unexpected side effect of the change?

We were testing with curl 7.65.3. I don't see any changes in the later version that look like they would change this, but I will be trying again against latest as soon as I get a chance.

I'm also going to work on a minimal repro, although that may take a bit of work to untangle from the codebase :)

thank you

curl/libcurl version

7.65.3

operating system

Ubuntu 18.04.03

@bagder bagder added the crash label Nov 6, 2019
@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Nov 6, 2019

version 7.65.3 I presume...

@monkeygroover

This comment has been minimized.

Copy link
Author

@monkeygroover monkeygroover commented Nov 6, 2019

yeah sorry! will correct

@monkeygroover

This comment has been minimized.

Copy link
Author

@monkeygroover monkeygroover commented Nov 6, 2019

Confirmed still seeing the same issue against 7.67.0. I'll work on a repro, cheers.

@monkeygroover

This comment has been minimized.

Copy link
Author

@monkeygroover monkeygroover commented Nov 7, 2019

It's proving quite difficult to prise out just the pertinent code from our codebase to make a repro I can share, but some more info for now:

Part (but maybe not all) of the problem seems to be because the addition of Curl_updatesocket in curl_easy_pause causes a recursive call back to our socket handler

i.e. in the problematic scenario we have we see some activity on a socket and call curl_multi_socket_action and it results in the following chain of calls:

curl_multi_socket_action ->
multi_socket ->
multi_runsingle ->
multi_done ->
Curl_disconnect ->
conn_shutdown ->
Curl_closesocket ->
curl_multi_closed ->

which calls into our socket callback with what == CURL_POLL_REMOVE

which (in our case) then calls curl_easy_pause

subsequently Curl_updatesocket calls singlesocket

which calls into our socket callback again recursively

this second callback invocation also calls curl_easy_pause and I see the value for entry->users in singlesocket end up decremented twice (in singlesocket) and it becomes negative in our case (this obviously seems not ideal :) )

I say "but maybe not all" because even if I change the code to stop this recursion, i'm still seeing corruption somewhere else which I'm still digging into. (update it's the sh_delentry in Curl_multi_closed as the above described stack unwinds)

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Nov 8, 2019

Something about this use

Let's discuss the initial approach: "the intention is to ensure we correctly handle transfers left if we are paused when the socket closes"

The fact that libcurl tells your callback to stop monitoring the socket is in no way saying that the socket is or will be closed! It just tells your application that you can stop monitoring that socket for now - and if you pause a transfer on a specific socket is seems likely that you can get this message. Therefore, your pausing is itself a trigger for this callback to get called with CURL_POLL_REMOVE and you want to deal with that by immediately unpausing that transfer?

A proposed fix

I believe updating the socket after the "done" phase has been entered is unnecessary so we can avoid the recursive callback by a fix like this. I'd be very interested to hear how this works for you:

diff --git a/lib/easy.c b/lib/easy.c
index 001648d49..fc5eceb6a 100644
--- a/lib/easy.c
+++ b/lib/easy.c
@@ -1025,13 +1025,14 @@ CURLcode curl_easy_pause(struct Curl_easy *data, int action)
     Curl_expire(data, 0, EXPIRE_RUN_NOW); /* get this handle going again */
     if(data->multi)
       Curl_update_timer(data->multi);
   }
 
-  /* This transfer may have been moved in or out of the bundle, update
-     the corresponding socket callback, if used */
-  Curl_updatesocket(data);
+  if(!data->state.done)
+    /* This transfer may have been moved in or out of the bundle, update the
+       corresponding socket callback, if used */
+    Curl_updatesocket(data);
 
   return result;
 }
@bagder bagder self-assigned this Nov 8, 2019
bagder added a commit that referenced this issue Nov 8, 2019
... avoids unnecesary recursive risk when the transfer is already done.

Reported-by: Richard Bowker
Fixes #4563
@monkeygroover

This comment has been minimized.

Copy link
Author

@monkeygroover monkeygroover commented Nov 8, 2019

Yes, I only did a quick test but that does appear to solve the issue we were seeing. I'd like to run a more thorough set of tests next week just to double check before confirming for sure.

Thank you.

@monkeygroover

This comment has been minimized.

Copy link
Author

@monkeygroover monkeygroover commented Nov 9, 2019

Everything looks good from my side. I'm no longer seeing any issues after applying the proposed patch. Thank you very much!

@bagder bagder closed this in 32747aa Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.