Repository navigation
[BUG] The curl client mutates and frees easy handle resources before removing the handle from the multi handle #4391
Description
Describe your environment main at f6e4818, libcurl 8.14.1. This is a code reading report, not a crash report. I have not seen any of it misbehave at runtime, and I say where that matters below.
Three places in the curl client touch an easy handle, or free something the handle still points at, before the handle has been taken out of the multi handle, and the CURLMcode that would say whether the removal worked is dropped on the floor.
Teardown resets the handle before it is removed
HttpOperation::Cleanup() clears CURLOPT_PRIVATE and resets the handle, and only then hands it over to be removed:
if (curl_resource_.easy_handle != nullptr)
{
curl_easy_setopt(curl_resource_.easy_handle, CURLOPT_PRIVATE, NULL);
curl_easy_reset(curl_resource_.easy_handle);
}
session->GetHttpClient().ScheduleRemoveSession(...);curl_multi_remove_handle does not run until the IO thread gets to doRemoveSessions in http_client_curl.cc. On a cancel the transfer can still be active when that curl_easy_setopt runs, and curl_easy_setopt says:
Changing options with curl_easy_setopt() while a transfer is still in progress may cause undefined and undesired behavior.
The header list is freed before the removal too
if (nullptr != removing_handle.second.headers_chunk)
{
curl_slist_free_all(removing_handle.second.headers_chunk);
}
curl_multi_remove_handle(multi_handle_, removing_handle.second.easy_handle);
curl_easy_cleanup(removing_handle.second.easy_handle);When this option is passed to curl_easy_setopt, libcurl does not copy the entire list so you must keep it around until you no longer use this handle for a transfer before you call curl_slist_free_all on the list.
At the curl_slist_free_all the handle is still attached, so the list is freed while it is arguably still in use for the transfer. Swapping that call and the curl_multi_remove_handle below it costs nothing.
The removal result is not checked
That curl_multi_remove_handle ignores its CURLMcode and the curl_easy_cleanup after it cleans the handle up regardless. If the removal ever fails, that frees a handle the multi stack still knows about. doRetrySessions() has the same shape:
auto easy_handle = operation->GetCurlEasyHandle();
curl_multi_remove_handle(multi_handle_, easy_handle);
curl_multi_add_handle(multi_handle_, easy_handle);No null check, and neither return value is read. GetCurlEasyHandle() returns null once Cleanup() has moved curl_resource_ out, and passing null to those two is harmless, so the concrete risk here is small. It is the silence I would rather not keep: a session can leave pending_to_retry_sessions_ whether or not it was actually re-armed.
What is the expected behavior? The IO thread removes the handle from the multi handle, checks the result, and only then resets it, frees the header list, and cleans it up. A failed removal is visible rather than assumed away.
What is the actual behavior? Options are set and memory is freed while the handle may still be attached, and a failed removal looks exactly like a successful one.
Additional context No reproduction. Reordering the free and the removal and checking the two return codes is small and self contained, and I am happy to send it if you want it, but it is your call whether the retry path belongs in the same change or somewhere else.
Found while working on #4375, which is on the same file but is a different problem.
Activity
thc1006 commentedon Aug 10, 2026
Two things I had wrong when I first worked out what the fix should be, both worth writing down before anyone codes it.
A handle that was never added still reports
CURLM_OK. I assumed the removal would have to know whether the handle had made it into the multi handle, since a cancel can arrive beforedoAddSessionsruns, and that meant carrying an attached-or-not bit throughHttpCurlEasyResource. It does not. Fromlib/multi.c:So
CURLM_BAD_EASY_HANDLEonly comes back for a handle that is bogus or that belongs to a different multi handle. Remove unconditionally, check the code, and the never-added case takes care of itself with no extra state.Clearing
CURLOPT_PRIVATEinCleanup()is load bearing, not tidy up. It reads like leftover hygiene next to thecurl_easy_resetbeside it, andcurl_easy_resetreally is pointless when the next thing to happen iscurl_easy_cleanup. The other line is not.http_client_curl.cc:520reads the value back to decide whether aCURLMSG_DONEstill belongs to anyone:Delete the assignment and a message the multi handle had already buffered runs
PerformCurlMessageon an operation that finished, which dispatches a second round of terminal events into the handler. That is the doubling #4360 is about, arriving from a new direction. Moving the clear onto the IO thread does not help either, since the loop drains messages before it reachesdoRemoveSessions.What I ended up with is an
is_cleaned_check at the top ofPerformCurlMessage, which keeps the behaviour without writing to a handle from a thread that does not own it. Being straight about its evidence: I instrumented that branch and ran the whole suite, and it fires zero times, so nothing here exercises it. It is in because it replaces something the code deliberately did, not because I caught it working.The rest is the ordering you would expect.
Cleanup()hands the resource over untouched,doRemoveSessionsremoves, checks theCURLMcode, and only then frees the list and the handle. A failed removal leaves both alone and logs, since leaking one easy handle is the better half of that trade.It is written and verified, and I am sitting on it rather than opening it, because
Cleanup()is also touched by #4392 and by the #4390 work, and three pull requests editing one function is how the last batch of mine ended up waiting a fortnight. Happy to send it whenever the two ahead of it land, or sooner if you would rather see it now.[BUG] Remove a curl easy handle before releasing what it points at
[BUG] Remove a curl easy handle before releasing what it points at
[BUG] Remove a curl easy handle before releasing what it points at
[BUG] Remove a curl easy handle before releasing what it points at
[BUG] Remove a curl easy handle before releasing what it points at
thc1006 commentedon Aug 14, 2026
Correcting the first half of what I wrote above, because anyone implementing from it would build the wrong thing.
I said a handle the multi handle never took reports
CURLM_OK, so the removal doesn't need to know whether the handle was ever added. That's true of the libcurl I read and tested it on, and it isn't true in general. The prologue ofcurl_multi_remove_handlecarried a second condition for two minor releases:if(!GOOD_EASY_HANDLE(data))CURLM_OKif(!GOOD_EASY_HANDLE(data) || !multi->num_easy)CURLM_BAD_EASY_HANDLEif(!GOOD_EASY_HANDLE(data))CURLM_OK!multi->num_easyonly holds while the multi handle is empty, so on those two the same call answers differently depending on whether anything else is in flight. A busy client agrees with what I wrote. A quiet one, or a shutdown, doesn't.cmake/curl.cmakecallsfind_package(CURL QUIET)with no lower bound, so both sides of that are in range.The version window isn't needed to reach the same branch either. On 8.14.1
curl_multi_remove_handle(NULL, easy)gives meCURLM_BAD_HANDLE, and that's what a client whosecurl_multi_init()returned null hands to every removal it ever makes.So the attached-or-not bit I talked myself out of is needed, and #4405 carries it. The client records an easy handle when
curl_multi_add_handleaccepts it, and one that was never accepted is freed without asking the multi handle anything, which takes both libcurl's answer and the version it came from out of the decision.The second half of that comment, about
CURLOPT_PRIVATEbeing load bearing rather than tidy up, still holds.[BUG] Remove a curl easy handle before releasing what it points at
[BUG] Remove a curl easy handle before releasing what it points at
[BUG] Remove a curl easy handle before releasing what it points at