Skip to content

[BUG] Send one request per curl session - #4431

Open
thc1006 wants to merge 3 commits into
open-telemetry:mainfrom
thc1006:bugfix/session-is-single-use-4396
Open

thc1006 wants to merge 3 commits into
open-telemetry:mainfrom
thc1006:bugfix/session-is-single-use-4396

Conversation

@thc1006

@thc1006 thc1006 commented Aug 14, 2026

Copy link
Copy Markdown
Member

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 in CURLOPT_PRIVATE. The message loop reads that back and resolves it to whichever operation the session owns by then:

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

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 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 unique_ptr::reset destroys the object whose Cleanup() is two frames up, and Cleanup() reads it again on the way out.

What this does

A session carries one request:

  • the first SendRequest consumes it, and a second reports CreateFailed to 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:

before after
ASecondRequestFromInsideTheResponseIsRefused exit 1, heap-use-after-free in unique_ptr<AsyncData>::_M_ptr() passes
AddressSanitizer reports 1 0
whole curl_http_test under ASan 30 of 30, no sanitizer output

The 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.cc has exactly one CreateSession and exactly one SendRequest;
  • exporters/elasticsearch/src/es_log_record_exporter.cc has one CreateSession and two SendRequest, with the #else between them, so they are the two arms of one conditional rather than two sends;
  • exporters/zipkin/src/zipkin_exporter.cc has neither, so it does not reach this path at all.

No case in curl_http_test sends 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.h interface, only on the curl Session that implements it. Saying it in the interface would bind every implementation, including the one in examples/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_PRIVATE would 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

result
curl_http_test under ASan with detect_leaks=1 30 of 30, no sanitizer output
same, OTELCPP_WITH_OTLP_RETRY_PREVIEW=ON 30 of 30, no sanitizer output
bazel test //ext/test/http:curl_http_test passes
otlp_http_exporter_test, log record, metric, all under ASan 55, 16, 22, all pass
clang-format 18.1.8 clean

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

Every number above re-measured on the current head 97350255 against f77c1a5c, 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 the OTELCPP_ 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 with heap-use-after-free, and with it restored it passes.

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

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.52%. Comparing base (ad74ba2) to head (c97f5d1).

Additional details and impacted files

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
...ntelemetry/ext/http/client/curl/http_client_curl.h 95.13% <100.00%> (+0.13%) ⬆️
ext/src/http/client/curl/http_client_curl.cc 93.20% <100.00%> (+0.07%) ⬆️

... 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.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
@thc1006
thc1006 marked this pull request as ready for review September 22, 2026 18:00
@thc1006
thc1006 requested a review from a team as a code owner September 22, 2026 18:00
@thc1006

thc1006 commented Sep 22, 2026

Copy link
Copy Markdown
Member Author

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 Session.

The rule is the one @owent stated on #4448, that a Session was never meant to take a second request. Today a second SendRequest() replaces the operation the first one is still running, while the first easy handle keeps calling back through pointers into the object that was replaced. With this change the second call reports CreateFailed to its own handler and sends nothing.

Checked today against f77c1a5c:

  • 0 commits behind main, merges clean
  • CI: 76 checks, none failing
  • curl_http_test under AddressSanitizer with leak detection: 30 of 30, no sanitizer output
  • both new cases run rather than skip

No overlap with #4618. That one is in http_operation_curl, this is in http_client_curl, and the changelog is the only file they share.

Two decisions I left alone. The rule lives on the curl Session and not in ext/http/client/http_client.h, because putting it in the interface would bind examples/custom_http_client as well. And if keeping reuse is preferable, giving each operation an identity of its own is a different change that I think deserves its own issue rather than being folded in here; the description sketches what it would take.

Happy to change either, and there is no hurry from my side.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Reusing a curl Session destroys the HttpOperation that is still running its own completion

3 participants