Skip to content

Commit f4a41eb

Browse files
committed
[BUG] Clear a detached easy handle before freeing it
curl_easy_cleanup documents that it can reach the application: "Occasionally you may get your progress callback or header callback called from within curl_easy_cleanup (if previously set for the handle using curl_easy_setopt)", for protocols that need a command and response before disconnecting. By then the event handler is gone. The only shared_ptr to it is captured in the completion callback that SendRequest builds, Cleanup swaps that out and runs it, and it is released when Cleanup returns. The write, header and read callbacks all dispatch through the raw pointer the operation keeps, so the one libcurl says it may call is the one with nothing behind it. Clearing the handle first takes the callbacks and their data off it, so freeing it cannot reach anything the client has let go of, and the header list is freed after that because libcurl does not copy it and the clear is what stops it being read. This runs on the background thread with the handle detached, which is what the version removed from Cleanup earlier on this branch could not say: that one ran on whichever thread cancelled, against a handle the multi handle still held. Measured on 8.14.1 over HTTP, neither curl_multi_remove_handle nor curl_easy_cleanup reaches a callback once the transfer has finished, and removing a handle from a live transfer reaches the progress callback once. So nothing here is what stops a failure today over HTTP. What it stops is the call libcurl reserves the right to make. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
1 parent 36bd6e6 commit f4a41eb

1 file changed

Lines changed: 13 additions & 4 deletions

File tree

‎ext/src/http/client/curl/http_client_curl.cc‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,21 +49,30 @@ namespace curl
4949

5050
namespace
5151
{
52-
// The easy handle goes first. libcurl does not copy the header list, so it is in use for as long
53-
// as anything is still transferring on the handle that points at it.
52+
// Only ever called with the handle detached, either because it has been given back or because
53+
// the multi handle that held it has been cleaned up.
54+
//
55+
// The reset clears the callbacks and the pointers they are given, so that freeing the handle
56+
// cannot reach the operation or the event handler the caller has let go of by then. The header
57+
// list goes next, because libcurl does not copy it and the reset is what stops it being read.
5458
void ReleaseCurlResource(HttpCurlEasyResource &resource) noexcept
5559
{
5660
if (nullptr != resource.easy_handle)
5761
{
58-
curl_easy_cleanup(resource.easy_handle);
59-
resource.easy_handle = nullptr;
62+
curl_easy_reset(resource.easy_handle);
6063
}
6164

6265
if (nullptr != resource.headers_chunk)
6366
{
6467
curl_slist_free_all(resource.headers_chunk);
6568
resource.headers_chunk = nullptr;
6669
}
70+
71+
if (nullptr != resource.easy_handle)
72+
{
73+
curl_easy_cleanup(resource.easy_handle);
74+
resource.easy_handle = nullptr;
75+
}
6776
}
6877
} // namespace
6978

0 commit comments

Comments
 (0)