From db18c4af70dba7b170a8ebe58a2086efd93b0678 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 21 Sep 2026 05:20:46 +0000 Subject: [PATCH 1/2] [BUG] Fix a data race on the curl operation's last result code Finish() and ~HttpOperation() stored into last_curl_result_ after waiting on the completion promise, while the IO thread read the same member from IsRetryable() as it drained curl messages. Nothing ordered the two, so ThreadSanitizer reported a write of size 4 racing a read of size 4. The store was writing back a value the IO thread had already published: Cleanup() sets the promise from last_curl_result_, and the future orders that write ahead of the waiter. Dropping it leaves the member with a single writer and removes the race rather than making it benign. Cleanup() is the only place the promise is set, it is reached only from the branch that will not retry, and it detaches the handle by clearing CURLOPT_PRIVATE, so no later attempt can produce a newer code for the waiter to overwrite. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 +++ ext/src/http/client/curl/http_operation_curl.cc | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e203290157..3c8cc41172 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,9 @@ Increment the: to compile standalone on newer standard library implementations. [#4574](https://github.com/open-telemetry/opentelemetry-cpp/pull/4574) +* [BUG] Fix a data race on the curl operation's last result code + [#4618](https://github.com/open-telemetry/opentelemetry-cpp/pull/4618) + ## [1.29.0] 2026-09-13 * [RELEASE] Bump main branch to 1.29.0-dev (#4259) diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 8d399d5f2a..ef0f96dc4c 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -507,8 +507,8 @@ HttpOperation::~HttpOperation() { if (HttpOperationAccessor::GetThreadId(*async_data_) != std::this_thread::get_id()) { - async_data_->result_future.wait(); - last_curl_result_ = async_data_->result_future.get(); + // The IO thread stored this into the member before setting the promise. + static_cast(async_data_->result_future.get()); } } break; @@ -532,8 +532,8 @@ void HttpOperation::Finish() // We should not wait in callback from Cleanup() if (HttpOperationAccessor::GetThreadId(*async_data_) != std::this_thread::get_id()) { - async_data_->result_future.wait(); - last_curl_result_ = async_data_->result_future.get(); + // The IO thread stored this into the member before setting the promise. + static_cast(async_data_->result_future.get()); } } } From 5798bceb021c0156da6c684a50ff6088d6f007d5 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Wed, 23 Sep 2026 13:29:15 +0000 Subject: [PATCH 2/2] [CHORE] Name the writer in the comment at both wait sites The comment said the IO thread stores the result before the promise is set, without saying where, so the invariant could not be checked from the wait. PerformCurlMessage() does the store at its top and calls Cleanup() at its end, and Cleanup() is what sets the promise. Naming both puts the check one hop away. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_operation_curl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index ef0f96dc4c..e2c8dfad4f 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -507,7 +507,7 @@ HttpOperation::~HttpOperation() { if (HttpOperationAccessor::GetThreadId(*async_data_) != std::this_thread::get_id()) { - // The IO thread stored this into the member before setting the promise. + // PerformCurlMessage() stores this, then calls the Cleanup() that sets the promise. static_cast(async_data_->result_future.get()); } } @@ -532,7 +532,7 @@ void HttpOperation::Finish() // We should not wait in callback from Cleanup() if (HttpOperationAccessor::GetThreadId(*async_data_) != std::this_thread::get_id()) { - // The IO thread stored this into the member before setting the promise. + // PerformCurlMessage() stores this, then calls the Cleanup() that sets the promise. static_cast(async_data_->result_future.get()); } }