Conversation
SendRequest replaces the operation the session owns. The easy handle of the first request holds that operation in every pointer libcurl calls back through, and holds the session in CURLOPT_PRIVATE, which the message loop reads back and resolves to whichever operation the session owns by then. So a second request takes delivery of the first one's completion. From a handler that sends again out of OnResponse it is worse than a mix up. Cleanup takes the completion callback and runs it while the operation it belongs to is still on the stack, so the reset destroys the object whose Cleanup is two frames up, and Cleanup reads it again on the way out. AddressSanitizer reports heap-use-after-free in unique_ptr<AsyncData>::_M_ptr, and the case that provokes it is the reproduction from the issue. A session now carries one request. A second SendRequest reports CreateFailed to its own handler and sends nothing, which is what a send that cannot be made already reports, and CreateRequest leaves the request a send is reading from where it is, since libcurl does not copy the body or the header list. Every caller here already works that way: the OTLP HTTP client and the Elasticsearch exporter each make a session, send once and let it go, and no case in the suite sends twice on one session. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4431 +/- ##
==========================================
- Coverage 86.53% 86.52% -0.00%
==========================================
Files 525 525
Lines 20475 20481 +6
==========================================
+ Hits 17715 17719 +4
- Misses 2760 2762 +2
🚀 New features to boost your workflow:
|
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> # Conflicts: # CHANGELOG.md
|
Un-drafted, so this is ready whenever there is time for it. It is small: 125 added lines, nothing removed, one rule on the curl The rule is the one @owent stated on #4448, that a Checked today against
No overlap with #4618. That one is in Two decisions I left alone. The rule lives on the curl Happy to change either, and there is no hurry from my side. |
Fixes #4396.
What happens today
Session::SendRequest()replaces the operation the session owns. The easy handle of the first request holds that operation in every pointer libcurl calls back through, and holds the session inCURLOPT_PRIVATE. The message loop reads that back and resolves it to whichever operation the session owns by then:So a second request takes delivery of the first one's completion, and the first one's handle keeps calling back into an operation nobody owns.
From a handler that sends again out of
OnResponseit is worse than a mix up.Cleanup()takes the completion callback and runs it while the operation it belongs to is still on the stack, so theunique_ptr::resetdestroys the object whoseCleanup()is two frames up, andCleanup()reads it again on the way out.What this does
A session carries one request:
SendRequestconsumes it, and a second reportsCreateFailedto its own handler and sends nothing, which is what a send that cannot be made already reports on this class;CreateRequest()after that leaves the request the send is reading from where it is, because libcurl does not copy the body or the header list.Evidence
The case is the reproduction from the issue, a handler that sends again from
OnResponse. Same binary, same command, either side of the change:ASecondRequestFromInsideTheResponseIsRefusedheap-use-after-freeinunique_ptr<AsyncData>::_M_ptr()curl_http_testunder ASanThe second case,
ASessionSendsOneRequest, is the same rule without the re-entrancy: send, wait, send again, and check that the second is refused and the backing request was not replaced under the first.Is anything relying on reuse
Not here. Both callers make a session, send once and let it go:
exporters/otlp/src/otlp_http_client.cchas exactly oneCreateSessionand exactly oneSendRequest;exporters/elasticsearch/src/es_log_record_exporter.cchas oneCreateSessionand twoSendRequest, with the#elsebetween them, so they are the two arms of one conditional rather than two sends;exporters/zipkin/src/zipkin_exporter.cchas neither, so it does not reach this path at all.No case in
curl_http_testsends twice on one session either, and the three OTLP HTTP exporter test binaries pass under ASan unchanged: 55, 16 and 22, with no sanitizer output.What I did not do
I have not put the rule in the
ext/http/client/http_client.hinterface, only on the curlSessionthat implements it. Saying it in the interface would bind every implementation, including the one inexamples/custom_http_client, and that reads like your call rather than mine. Say the word and I will move it.The alternative is to keep reuse and give each operation an identity of its own:
CURLOPT_PRIVATEwould carry the operation rather than the session, the pending records would own the exact operation, its request and its handler, and the message loop would resolve the operation that owns the handle rather than the one the session owns now. That is a rewrite of how this client owns things, it conflicts with every open change to the same file, and nothing in this repository asks for reuse. If you want it, it wants its own issue.Checks
curl_http_testunder ASan withdetect_leaks=1OTELCPP_WITH_OTLP_RETRY_PREVIEW=ONbazel test //ext/test/http:curl_http_testotlp_http_exporter_test, log record, metric, all under ASanFor significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changesEvery number above re-measured on the current head
97350255againstf77c1a5c, 2026-09-22. Two things changed since this was written in August and are corrected here: the counts were 28 and are 30, and the retry option gained theOTELCPP_prefix. The call site citations were line numbers that had drifted by seven lines, so they are counts now, which is what the argument rests on anyway. The before and after rows were re-run rather than carried over: with the one-shot guard removed from this head the case still exits 1 withheap-use-after-free, and with it restored it passes.