[BUG] The curl client mutates and frees easy handle resources before removing the handle from the multi handle

[thc1006]

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);   // :556
        curl_easy_reset(curl_resource_.easy_handle);                           // :557
      }
      session->GetHttpClient().ScheduleRemoveSession(...);                     // :559

curl_multi_remove_handle does not run until the IO thread gets to doRemoveSessions, at http_client_curl.cc:819. On a cancel the transfer can still be active at :556, 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);             // :816
      }

      curl_multi_remove_handle(multi_handle_, removing_handle.second.easy_handle);  // :819
      curl_easy_cleanup(removing_handle.second.easy_handle);                        // :820

CURLOPT_HTTPHEADER:

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 :816 the handle is still attached, so the list is freed while it is arguably still in use for the transfer. Swapping :816 and :819 costs nothing.

The removal result is not checked

:819 ignores its CURLMcode and :820 cleans the handle up regardless. If the removal ever fails, that frees a handle the multi stack still knows about. The retry path has the same shape at :862:

      auto easy_handle = operation->GetCurlEasyHandle();
      curl_multi_remove_handle(multi_handle_, easy_handle);                    // :863
      curl_multi_add_handle(multi_handle_, easy_handle);                       // :864

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_ at :865 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 :816 and :819 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.

[thc1006]

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 before doAddSessions runs, and that meant carrying an attached-or-not bit through HttpCurlEasyResource. It does not. From lib/multi.c:

/* Prevent users from trying to remove same easy handle more than once */
if(!data->multi)
  return CURLM_OK; /* it is already removed so let's say it is fine! */

/* Prevent users from trying to remove an easy handle from the wrong multi */
if(data->multi != multi)
  return CURLM_BAD_EASY_HANDLE;

So CURLM_BAD_EASY_HANDLE only 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_PRIVATE in Cleanup() is load bearing, not tidy up. It reads like leftover hygiene next to the curl_easy_reset beside it, and curl_easy_reset really is pointless when the next thing to happen is curl_easy_cleanup. The other line is not. http_client_curl.cc:520 reads the value back to decide whether a CURLMSG_DONE still belongs to anyone:

curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, &session);
const auto operation = (nullptr != session) ? session->GetOperation().get() : nullptr;

// If it's already moved into pending_to_remove_session_handles_, we just ignore this
// message.
if (operation)

Delete the assignment and a message the multi handle had already buffered runs PerformCurlMessage on 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 reaches doRemoveSessions.

What I ended up with is an is_cleaned_ check at the top of PerformCurlMessage, 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, doRemoveSessions removes, checks the CURLMcode, 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.