[BUG] Remove a curl easy handle before releasing what it points at by thc1006 · Pull Request #4405 · open-telemetry/opentelemetry-cpp

thc1006 · 2026-08-11T05:45:29Z

What was wrong, and one thing I had wrong about it

The issue has three parts: Cleanup() writes to an easy handle that may still be transferring, doRemoveSessions() frees the header list before it takes the handle out of the multi handle, and neither the removal nor the retry path reads the CURLMcode it gets back. The first two are ordering and they're fixed by moving two calls.

The third one I got wrong in my own comment on the issue, where I said a handle the multi handle never took reports CURLM_OK, so nothing has to remember which handles were added. That's true of the libcurl I tested on and it isn't true in general. The prologue of curl_multi_remove_handle picked up a second condition for two minor releases:

libcurl	prologue	never added, multi handle empty
7.81.0, 8.5.0, 8.6.0, 8.7.1, 8.8.0, 8.9.1	`if(!GOOD_EASY_HANDLE(data))`	`CURLM_OK`
8.10.1, 8.11.1	`if(!GOOD_EASY_HANDLE(data) \|\| !multi->num_easy)`	`CURLM_BAD_EASY_HANDLE`
8.12.0, 8.14.1	`if(!GOOD_EASY_HANDLE(data))`	`CURLM_OK`

find_package(CURL) here has no lower bound, so both are supported. And the version window isn't even needed to reach the same branch: I measured curl_multi_remove_handle(NULL, easy) returning CURLM_BAD_HANDLE on 8.14.1, which is what a client whose curl_multi_init() failed hands to every removal.

What made that reading dangerous is what the failure branch did with it. pending_to_remove_session_handles_ is swapped into a local, so a continue past a record ends the life of that record at the end of the pass. The easy handle and its header list go with it, and so does the shared_ptr<Session> the same pass was releasing, while the handle still carries that session in CURLOPT_PRIVATE. The message loop reads it straight back:

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

What this does now

An easy handle is recorded when curl_multi_add_handle accepts it, and unrecorded when curl_multi_remove_handle gives it back. A handle that was never accepted is freed without asking the multi handle anything, so neither libcurl's answer nor the version it came from decides the path.
A removal libcurl refuses keeps the whole record: handle, header list and the session it names. Those are freed after curl_multi_cleanup detaches them, in resetMultiHandle() and in the destructor. No retry loop, because a refusal here doesn't become acceptance later, and I didn't want a busy path in the IO loop.
The queue holds one record per easy handle rather than one per session. A session that starts another request hands over a second handle while the first is still queued, and the map keyed by session dropped the first one.
The destructor releases what the background thread never got to. That path frees nothing today: a handle handed over after the last pass measured 5594 bytes in 7 allocations under LeakSanitizer.
doRetrySessions() goes through the same accounting and reports a re-arm libcurl won't accept, which is the silence the issue asks about.

Evidence

Fail before, pass after, against dd2ca474 (this branch before the change) with the same two scenarios written against the members that existed there:

	before	after
`ARefusedRemovalKeepsTheHandleAndTheSessionItNames`	fails, the session is released while the handle still names it	passes
`ASecondHandleFromTheSameSessionDoesNotDisplaceTheFirst`	fails, 1 record queued where 2 were handed over	passes
LeakSanitizer on those two	11171 bytes in 14 allocations	0

Each case was then checked against the mutation it's meant to catch, on an otherwise clean tree:

mutation	case that fails
a refused removal drops the record again	`ARefusedRemovalKeepsTheHandleAndTheSessionItNames`
ask libcurl even about a handle it was never given	`AHandleTheMultiHandleNeverTookIsFreedNotStranded`
drop this session's queued handles on a new add	`ASecondHandleFromTheSameSessionDoesNotDisplaceTheFirst`
forget to unrecord what the removal gave back	`TheAttachmentLedgerIsEmptyOnceARequestFinishes`
forget to record what the add accepted	nothing

That last row is honest rather than tidy. With the record never written, every release takes the never-added path and frees a handle the multi handle still holds, and I could not get that to show as anything: a small probe says curl_easy_cleanup detaches the handle itself on the way out on 8.14.1, so the state afterwards is identical either way. The half of the accounting that's covered is the half that skips a removal it doesn't need. The half that isn't is the documented call order, which the same probe suggests only bites while a transfer is actually running, and I don't have a way to hold one open and free underneath it from a test.

One case fixed that isn't about any of this

The valgrind job went red on RepeatedCallerThreadCancelsAreClean, which came in with #4392 and has nothing to do with the accounting here. It sums Cancelled and Response over twenty attempts against a closed port and asks for at least one each, and there's a third way such an attempt ends: the connection fails before the cancel arrives, and the cancel is then correctly a no-op. Instrumented, the silent attempts run Created, Connecting, ConnectFailed and stop there.

Pinned to one core, fifteen runs each:

	pinned to one core	unpinned
main at `3fb1d317`	15 of 15 fail, 3 to 7 cancels of 20	15 of 15 pass
#4395 at `66ab56ca`	15 of 15 fail, 3 to 7	12 of 12 pass
this branch before the change	15 of 15 fail, 3 to 7	12 of 12 pass
this branch after it	15 of 15 fail, 4 to 7	12 of 12 pass

Same on all four, so it isn't the change, it's a case that pinned one of two legitimate outcomes. It's now asking each attempt to end somewhere the handler is told about rather than to end the same way twenty times, and pinned to one core that goes from 15 of 15 failing to 0 of 15. The connection failure is counted apart from terminal_count_, which two other cases pin to an exact value, and outside the existing chain, because ACancelBeforeTheResponseReportsCancelled cancels from ConnectFailed and needs that branch to keep firing.

It's a separate commit. It rides along because it's the job this PR makes red and it's a case I added, but it splits out cleanly if you'd rather have it on its own.

Checks

	result
`curl_http_test`	31 of 31
same, `WITH_OTLP_RETRY_PREVIEW=ON`	31 of 31
same, ASan with `detect_leaks=1`	31 of 31, no sanitizer output
`bazel test //ext/test/http:curl_http_test`	passes
clang-tidy 22, CI header filters	0 warnings before, 0 after, with a positive control that fires
include-what-you-use 0.26, `WITH_STL=CXX14`	clean, after taking the include changes it asked for
clang-format 18.1.8	idempotent on all three files

Codecov Report

❌ Patch coverage is 94.38202% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.65%. Comparing base (60c3d11) to head (18bb79d).

Files with missing lines	Patch %	Lines
ext/src/http/client/curl/http_client_curl.cc	94.05%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4405      +/-   ##
==========================================
+ Coverage   82.61%   82.65%   +0.04%     
==========================================
  Files         511      511              
  Lines       20132    20187      +55     
==========================================
+ Hits        16631    16684      +53     
- Misses       3501     3503       +2

Files with missing lines	Coverage Δ
...ntelemetry/ext/http/client/curl/http_client_curl.h	`95.00% <ø> (ø)`
...lemetry/ext/http/client/curl/http_operation_curl.h	`91.31% <ø> (ø)`
ext/src/http/client/curl/http_operation_curl.cc	`60.60% <100.00%> (ø)`
ext/src/http/client/curl/http_client_curl.cc	`91.01% <94.05%> (+0.68%)`	⬆️

🚀 New features to boost your workflow:

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

thc1006 · 2026-08-11T17:10:43Z

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.

…_read Two corrections to the change before it, both found reviewing it rather than reported. The wait cannot be interrupted. wakeupBackgroundThread reaches the worker through the multi handle, and the whole point of that branch is that there is not one, so the destructor cannot cut the wait short and the previous commit saying teardown was unchanged is wrong. Destroying a client whose worker was inside the wait measured 168 ms, five runs out of five. Taking the same wait in 16 ms slices and rechecking is_shutdown_ leaves the retry rate where it was and brings that to 10 ms, five runs out of five. And curl_multi_info_read was still called with the handle libcurl had refused to give. curl_multi_poll and curl_multi_wait cannot see one, since they sit inside the branch that only runs when perform succeeded. curl_multi_add_handle and curl_multi_remove_handle can, but they are in doAddSessions and doRemoveSessions, which open-telemetry#4395 and open-telemetry#4405 rewrite, so they are named in the description rather than changed here. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

Teardown touched the easy handle in the wrong order and on the wrong thread. Cleanup cleared CURLOPT_PRIVATE and called curl_easy_reset on it, from whichever thread ran the cancel, while the transfer could still be active. doRemoveSessions then freed the header list before curl_multi_remove_handle and cleaned the handle up whatever that call returned. libcurl is explicit on all three: changing options while a transfer is in progress may have undefined behaviour, the header list has to outlive the handle that points at it, and a handle has to leave the multi handle before it can be cleaned up. Cleanup now hands the resource over untouched. doRemoveSessions removes it, checks the CURLMcode, and only then frees the list and the handle. A failed removal leaves both alone: leaking one easy handle is better than freeing one the multi stack may still own. A handle that was never added reports CURLM_OK, so no separate bookkeeping is needed to tell the two apart. Clearing CURLOPT_PRIVATE was load bearing rather than tidy up. The IO loop reads it back to decide whether a CURLMSG_DONE belongs to a session that has already gone, so PerformCurlMessage checks is_cleaned_ instead. That keeps the behaviour without writing to a handle from a thread that does not own it. Part of open-telemetry#4391. The retry path still calls curl_multi_remove_handle and curl_multi_add_handle without checking either result, so the issue stays open. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

The comments carried the reasoning that found the bug as well as the rule the code follows. The rule is what a reader needs; the rest belongs in the pull request. Each block now states its constraint and stops, and the two member comments in the installed headers follow the one line trailing form the file already uses next to them. No code changes. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

Handing the easy handle over untouched leaves CURLOPT_PRIVATE and the write, header, read and progress callback data pointing at the session and its operation. Clearing them was what made that safe before, and this branch stopped clearing them on purpose, because the cancelling thread does not own a handle the multi handle may still be driving. The owner then has to outlive the handle, and on the abort path it did not. ScheduleAbortSession takes the session out of sessions_, so doRemoveSessions cannot find it to hold one, and doAbortSessions kept the only remaining shared_ptr in a local map that died on return. The handle could reach curl_multi_remove_handle and curl_easy_cleanup with those pointers dangling, and curl_easy_cleanup is documented as able to run the progress and header callbacks. pending_to_remove_sessions_ already exists for this: doRemoveSessions swaps it out and holds it until the handle is freed. The abort path hands the session to it now. 26 tests pass, 3 of 3, and under AddressSanitizer with leak detection 2 of 2 with no leaks and no errors. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

curl_multi_remove_handle reports whether that call succeeded, not whether the multi handle was holding the easy handle. libcurl 8.10 and 8.11 refuse a handle the multi handle does not hold, and a multi handle that failed to initialize refuses every one, so reading the refusal as ownership dropped the record: the easy handle, the header list it points at and the session it names went with it, while the message loop still reads that session back out of CURLOPT_PRIVATE. The client now records an easy handle when curl_multi_add_handle accepts it, asks for no removal of one that was never added, and keeps the whole record, session included, when libcurl will not give a handle back. Kept handles are freed once curl_multi_cleanup has detached them. The queue holds one record per easy handle rather than one per session, so a session that starts another request no longer displaces the handle its previous request left behind, and the destructor releases what the background thread did not get to. The retry path goes through the same accounting, and reports a re-arm that libcurl would not accept rather than dropping the session from the queue in silence. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

RepeatedCallerThreadCancelsAreClean summed Cancelled and Response over twenty attempts against a closed port and asked for at least one each. There is a third way such an attempt ends: the connection fails before the cancel arrives, and the cancel is then correctly a no-op. Where the caller and the IO thread share a core that is the usual outcome, and the case fails. Measured with the run pinned to one core, fifteen runs each: 15 of 15 fail on main at 3fb1d31, on open-telemetry#4395 at 66ab56c, and on this branch either side of the change it carries, all with the same 3 to 7 cancels out of 20. Unpinned all four pass. The failure the valgrind job hit is that, not anything about the handle accounting. The connection failure is counted apart from terminal_count_, which two other cases pin to an exact value, and it is taken before the existing chain rather than inside it, because ACancelBeforeTheResponseReportsCancelled cancels from ConnectFailed and needs that branch to keep firing. The case now asks each attempt to end somewhere the handler is told about rather than to end the same way twenty times, which is what it was after. Pinned to one core it goes from 15 of 15 failing to 0 of 15. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

A queued easy handle still names its session through CURLOPT_PRIVATE, and the message loop reads that back and dereferences it. Removing a handle from an active transfer also runs the progress callback once, measured on 8.14.1, and that callback dereferences the operation the session owns. So the session has to outlive the record, and the record was relying on finding it again when it was drained, in sessions_ or in a second queue that CleanupSession filled. Neither survives the ordinary ordering. Instrumented over the suite, one or two sessions per run reach CleanupSession with a record of theirs in flight, no record for them visible because the background thread has already swapped the queue out, and the session inactive because its completion callback has run. Neither branch keeps it, so its last reference goes while the handle it names is still being released. The record now takes the session in the same critical section that publishes it. CleanupSession has nothing left to decide about a queued handle, and the queue that existed only to carry sessions found later is gone. The abort path gives its record a session directly, because scheduling an abort takes that session out of sessions_ before the handle is queued, and nothing drains in between: both steps belong to the background thread, one after the other. A handle recorded against a multi handle that gets replaced is not offered to the replacement, because curl_multi_cleanup detaches what it held and the ledger is emptied in the same step. That is what a generation on the record would be for, and there is a case for it now. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

The guard added earlier on this branch returns before the message is read, and the caller then asked IsRetryable() anyway. That predicate reads response_code_, last_curl_result_ and retry_attempts_, none of which the guard updates, so for an operation that has been cleaned up it answers with whatever the message before it left behind. A retryable status from that earlier message puts a session back in the retry queue while the easy handle it names is on its way out of the client, and doRetrySessions offers a moved-out handle to the multi handle without reading either return code. PerformCurlMessage now answers whether it has rewound the operation for another attempt, which is the only thing that makes the push correct, and the predicate is read once rather than twice with a Cleanup possible in between. The guard's comment claimed more than the flag carries. is_cleaned_ is exchanged at the top of Cleanup, ahead of the terminal event, the hand over of the easy handle, the completion callback and the promise, so reading it true says only that some thread has entered Cleanup. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

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>

…_read Two corrections to the change before it, both found reviewing it rather than reported. The wait cannot be interrupted. wakeupBackgroundThread reaches the worker through the multi handle, and the whole point of that branch is that there is not one, so the destructor cannot cut the wait short and the previous commit saying teardown was unchanged is wrong. Destroying a client whose worker was inside the wait measured 168 ms, five runs out of five. Taking the same wait in 16 ms slices and rechecking is_shutdown_ leaves the retry rate where it was and brings that to 10 ms, five runs out of five. And curl_multi_info_read was still called with the handle libcurl had refused to give. curl_multi_poll and curl_multi_wait cannot see one, since they sit inside the branch that only runs when perform succeeded. curl_multi_add_handle and curl_multi_remove_handle can, but they are in doAddSessions and doRemoveSessions, which open-telemetry#4395 and open-telemetry#4405 rewrite, so they are named in the description rather than changed here. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

thc1006 requested a review from a team as a code owner August 11, 2026 05:45

thc1006 mentioned this pull request Aug 11, 2026

[BUG] Check what curl_slist_append and curl_multi_init return #4406

Draft

3 tasks

thc1006 force-pushed the bugfix/remove-before-release-4391 branch from dd5df05 to 44b53fe Compare August 11, 2026 06:52

thc1006 force-pushed the bugfix/remove-before-release-4391 branch from 44b53fe to e625eef Compare August 11, 2026 18:01

thc1006 mentioned this pull request Aug 13, 2026

[BUG] Complete a curl operation that is never scheduled #4395

Draft

3 tasks

thc1006 force-pushed the bugfix/remove-before-release-4391 branch from 5a7e671 to 707ce77 Compare August 13, 2026 17:38

thc1006 marked this pull request as draft August 13, 2026 17:45

thc1006 force-pushed the bugfix/remove-before-release-4391 branch from 5ef4bf6 to 1e12bd8 Compare August 14, 2026 05:49

thc1006 force-pushed the bugfix/remove-before-release-4391 branch 2 times, most recently from a2f59a9 to 7714cb9 Compare August 14, 2026 09:17

thc1006 mentioned this pull request Aug 14, 2026

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

Open

thc1006 added 8 commits August 15, 2026 08:47

thc1006 force-pushed the bugfix/remove-before-release-4391 branch from f4a41eb to 18bb79d Compare August 15, 2026 08:48

Navigation Menu

Uh oh!

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

[BUG] Remove a curl easy handle before releasing what it points at#4405
thc1006 wants to merge 8 commits into
open-telemetry:mainfrom
thc1006:bugfix/remove-before-release-4391

thc1006 commented Aug 11, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Aug 11, 2026 •

edited

Loading

Uh oh!

thc1006 commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

thc1006 commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was wrong, and one thing I had wrong about it

What this does now

Evidence

One case fixed that isn't about any of this

Checks

Related

Uh oh!

codecov Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thc1006 commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

thc1006 commented Aug 11, 2026 •

edited

Loading

codecov Bot commented Aug 11, 2026 •

edited

Loading