[BUG] Remove a curl easy handle before releasing what it points at

[thc1006]

Part of #4391, not a full fix, so no closing keyword. The issue also names the retry path at doRetrySessions(), where curl_multi_remove_handle and curl_multi_add_handle are still unchecked. That belongs with the retry queue work rather than here, so #4391 should stay open until it is done.

Changes

Teardown touches the easy handle in the wrong order, and part of it on the wrong thread. Three things, and libcurl is explicit about each one.

HttpOperation::Cleanup() clears CURLOPT_PRIVATE and calls curl_easy_reset() on the handle. It runs on whichever thread cancelled, and the transfer can still be live inside curl_multi_perform() on the IO thread. curl_easy_setopt: "Changing options with curl_easy_setopt() while a transfer is still in progress may cause undefined and undesired behavior."

HttpClient::doRemoveSessions() frees the header list before it removes the handle. CURLOPT_HTTPHEADER: "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."

And it calls curl_easy_cleanup() whatever curl_multi_remove_handle() returned, because the return is dropped. curl_easy_cleanup: "To close an easy handle that has been used with the multi interface, make sure to first call curl_multi_remove_handle to remove it from the multi handle before it is closed."

So:

• Cleanup() hands the resource over untouched. The next thing that happens to the handle is curl_easy_cleanup(), so resetting it first bought nothing even when it was safe.
• doRemoveSessions() removes the handle first, checks the CURLMcode, and only then frees the list and the handle. A failed removal leaves both alone and logs. Leaking one easy handle is the better half of that trade: freeing one the multi stack may still own corrupts whatever holds it.
• A handle that was never added reports CURLM_OK from curl_multi_remove_handle() (lib/multi.c: if(!data->multi) return CURLM_OK;), so nothing has to track which handles were scheduled and which were not.

Clearing CURLOPT_PRIVATE was load bearing rather than tidying up. The IO loop reads it back to decide whether a buffered CURLMSG_DONE still belongs to anyone, so PerformCurlMessage() now checks is_cleaned_ instead. Same de-duplication, no write to a handle from a thread that does not own it.

On tests

I have not added one, and I would rather say why than tick the box on something thin.

Two of the three parts are ordering inside teardown with nothing observable from outside. Whether curl_multi_remove_handle() runs before or after curl_slist_free_all(), and whether Cleanup() touched the handle on its way out, are not reachable through the public API.

The third, the is_cleaned_ guard, is observable in principle, and I measured that the current suite does not discriminate it. Neutralising the guard and rebuilding leaves all 23 cases green, twice over. The case that does exercise the property it preserves is ACancelAfterTheResponseReportsOneOutcome on #4392, which this PR sits behind.

What I ran instead is the whole suite under AddressSanitizer with detect_leaks=1, because "leave both alone when the removal fails" is the part of this change that could plausibly leak.

If you want a dedicated case for the guard, say so and I will build one around a buffered CURLMSG_DONE. I did not want to ship a test I had not first watched fail.

Checks

	result
curl_http_test, OTELCPP_MAINTAINER_MODE=ON	23 of 23, 0 warnings
same suite under ASan with detect_leaks=1	23 of 23, no sanitizer output
ASan really linked	33 __asan_ symbols in the binary
clang-format 18.1.8	clean on both changed files

Related

This is the last of the four open against these two files. #4392 and #4395 both touch SendAsync() and Cleanup(), and #4394 touches resetMultiHandle(). Nothing here overlaps #4394. It does overlap the other two in Cleanup(), so I will rebase this one after they land rather than the other way round, and I have kept it to the smallest change that closes #4391 so that rebase stays mechanical.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.36%. Comparing base (a31138b) to head (e625eef).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ext/src/http/client/curl/http_client_curl.cc	69.24%	4 Missing ⚠️
ext/src/http/client/curl/http_operation_curl.cc	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4405      +/-   ##
==========================================
- Coverage   82.37%   82.36%   -0.00%     
==========================================
  Files         502      502              
  Lines       19883    19890       +7     
==========================================
+ Hits        16377    16381       +4     
- Misses       3506     3509       +3     

Files with missing lines	Coverage Δ
ext/src/http/client/curl/http_operation_curl.cc	60.30% <50.00%> (-0.29%)	⬇️
ext/src/http/client/curl/http_client_curl.cc	89.62% <69.24%> (-0.70%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[thc1006]

Where the five uncovered patch lines are, measured with the same all-options-abiv2-preview configuration ci/do_ci.sh code.coverage uses rather than read off the Codecov page.

One is the guard this change is about. PerformCurlMessage returns early when is_cleaned_ is set, for the message the multi handle had already buffered when the operation was torn down. Reaching it needs a CURLMSG_DONE to arrive for an operation that has been cleaned up in the same pass, which is the window the change exists to close. Nothing in the suite creates it today, and the case that would has to cancel a transfer that is already in flight and then land inside one loop iteration, so it is a timing case rather than a deterministic one.

One is a null easy handle in doRemoveSessions. Every path that schedules a removal today moves a live resource in, so a null handle in that map is the defensive half of the check rather than a state the client produces.

Three are the curl_multi_remove_handle failure branch. That call returns CURLM_OK for a handle that was never added, which is the whole reason the change can remove unconditionally, so the failure needs a handle owned by a different multi handle or a null multi handle. Both are reachable through the HttpClientTestPeer friend that #4394 added, and neither happens on its own.

I would rather say that plainly than add cases that pin defensive branches into place with test-only seams. If you want any of the three covered, say which and I will send them.