From 8ce027d0cd93b3d6d7380650b1047580c1439863 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:57:36 +0000 Subject: [PATCH 1/5] [BUGFIX] Stop a curl request after its gzip step fails The compression failure was reported to the handler and then ignored. Execution carried on past the branch that reports it, built the operation and sent the request. What went out was not the caller's payload. deflateInPlace() compresses into the caller's own buffer, and it writes into that buffer before it can know the result will not fit, so a failure leaves the body at its original length with its contents replaced by a partial deflate. No Content-Encoding header is set on that path either, so the peer is handed bytes that are neither the payload nor a gzip stream and nothing tells it which to expect. The branch now ends the request. The reason string is taken before deflateEnd() releases what zs.msg points at, the session is marked inactive before the handler runs because the handler is application code that can drop the last reference to the session, and nothing touches the session after it returns. GzipIncompressibleData changes with this. It required a response, on the reading that data which will not compress is sent uncompressed, and the assertion it rested on was that the body kept its original size. The size is not the whole story: the contents have been overwritten by then. Measured on this branch for bodies of 1, 500 and 5000 bytes, the size is unchanged in every case and the contents differ in every case. Keeping the payload instead, so that incompressible data can still be sent, means compressing into a separate buffer or copying the body before the attempt. That costs a copy on the compression path and is a decision rather than a repair, so it stays with #4360. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 + ext/src/http/client/curl/http_client_curl.cc | 19 ++++- ext/test/http/curl_http_test.cc | 76 +++++++++++++++++--- 3 files changed, 86 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 805c832e5..2917b1a63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ Increment the: * [CONFIGURATION] Add a configuration builder for the host resource detector [#4451](https://github.com/open-telemetry/opentelemetry-cpp/issues/4451) +* [BUGFIX] Stop a curl request whose gzip step failed, instead of sending + a body the failed compression had already overwritten + [#4360](https://github.com/open-telemetry/opentelemetry-cpp/issues/4360) * [CONFIGURATION] Build the configured resource detectors in SdkBuilder, apply the `detection.attributes` include/exclude filter to the detected attributes, and merge the resource per the resource SDK specification. diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 87f2c123a..c08f3a557 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -184,12 +184,25 @@ void Session::SendRequest( if (stream != Z_OK) { + // Taken before deflateEnd(), which releases what zs.msg points at. + const std::string reason = (nullptr != zs.msg) ? zs.msg : ""; + deflateEnd(&zs); + + // Before the handler runs, because the handler is application code: it can start another + // request on this session or drop the last reference to it, and nothing below this point + // may touch the session afterwards. + is_session_active_.store(false, std::memory_order_release); + if (callback) { - callback->OnEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, - zs.msg ? zs.msg : ""); + callback->OnEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, reason); } - is_session_active_.store(false, std::memory_order_release); + + // The caller has been told this request failed, so it does not go out. The body has also + // been partly rewritten in place by the deflate that failed and carries no Content-Encoding + // header, so what would have been sent is neither what the caller asked for nor readable as + // gzip at the other end. + return; } deflateEnd(&zs); diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 90142962d..2898320b1 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -984,6 +984,54 @@ struct GzipEventHandler : public CustomEventHandler std::string reason_; }; +// A request whose compression step fails is reported as a failed create and then must not be +// sent. It used to be reported and sent anyway, with a body the failed deflate had already +// partly rewritten and no Content-Encoding header to describe it. +// +// The failure is arranged rather than injected. deflateInPlace() is given the body's own size as +// its output budget, and its comment says it fails when that budget is smaller than the header +// plus one byte, so a body of one byte cannot fit a gzip header and the deflate reports +// Z_BUF_ERROR. No allocator hook and no timing are involved. +TEST_F(BasicCurlHttpTests, AFailedCompressionStopsTheRequest) +{ +# ifdef ENABLE_OTLP_COMPRESSION_PREVIEW + received_requests_.clear(); + auto session_manager = std::make_shared()->Create(); + ASSERT_TRUE(session_manager != nullptr); + + auto session = session_manager->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("post/"); + request->SetMethod(http_client::Method::Post); + + http_client::Body body(1, static_cast('a')); + request->SetBody(body); + request->AddHeader("Content-Type", "text/plain"); + request->SetCompression(opentelemetry::ext::http::client::Compression::kGzip); + + auto handler = std::make_shared(); + session->SendRequest(handler); + + // Asserted rather than assumed: a change that made this body compressible would otherwise leave + // the case passing while testing nothing. + ASSERT_TRUE(handler->is_called_) << "the compression did not fail, so nothing was tested"; + ASSERT_EQ(handler->state_, http_client::SessionState::CreateFailed); + + session->FinishSession(); + session_manager->FinishAllSessions(); + + // Long enough that a request which was going to be sent has been. The server records every + // request it receives, including one whose body is unreadable. + std::this_thread::sleep_for(std::chrono::milliseconds{500}); + + std::unique_lock lk1(mtx_requests); + EXPECT_TRUE(received_requests_.empty()) + << "a request the caller was told had failed was sent anyway"; +# else + GTEST_SKIP() << "gzip is not compiled in, so there is no compression step to fail"; +# endif // ENABLE_OTLP_COMPRESSION_PREVIEW +} + TEST_F(BasicCurlHttpTests, GzipCompressibleData) { received_requests_.clear(); @@ -1074,19 +1122,29 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData) request->SetCompression(opentelemetry::ext::http::client::Compression::kGzip); auto handler = std::make_shared(); session->SendRequest(handler); - ASSERT_TRUE(waitForRequests(30, 1)); - session->FinishSession(); - ASSERT_TRUE(handler->is_called_); - ASSERT_EQ(handler->state_, http_client::SessionState::Response); - ASSERT_TRUE(handler->reason_.empty()); - auto http_request = - dynamic_cast(request.get()); - ASSERT_TRUE(http_request != nullptr); - ASSERT_EQ(http_request->body_.size(), original_size); + // This used to require a response, on the reading that data which will not compress is sent + // uncompressed. The size assertion below is what that rested on, and the size is not the whole + // story: deflateInPlace() writes into the caller's buffer before it reports that the result + // would not fit, so the body keeps its length while its contents are no longer the caller's + // bytes. What went out was neither the payload nor a gzip stream, and carried no + // Content-Encoding header to describe itself either way. + // + // Refusing to send it is the narrow reading of "do not send after reporting a failure". Keeping + // the payload instead, by compressing into a separate buffer or by restoring the original on + // failure, would let incompressible data still be sent and costs a copy. That is a decision + // rather than a repair, so it is #4360 rather than this change. + ASSERT_TRUE(handler->is_called_); + ASSERT_EQ(handler->state_, http_client::SessionState::CreateFailed); + session->FinishSession(); session_manager->CancelAllSessions(); session_manager->FinishAllSessions(); + + std::this_thread::sleep_for(std::chrono::milliseconds{500}); + std::unique_lock lk1(mtx_requests); + EXPECT_TRUE(received_requests_.empty()) + << "a body the compression step had already overwritten was sent anyway"; } #endif // ENABLE_OTLP_COMPRESSION_PREVIEW From 4070f277bd38cd8d874bbf1961be4cf81c2f23fc Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 24 Aug 2026 02:19:52 +0000 Subject: [PATCH 2/5] [TEST] Drop a variable the rewritten assertion no longer uses GzipIncompressibleData used to assert that the body kept its original size and concluded from that it had been sent uncompressed. The assertion is gone, because the size is unchanged while the contents are not, and the variable it read was left behind. Maintainer mode builds with -Wall -Wextra -Werror and rejects it. The case also needs for the uint8_t it casts to, which include-what-you-use asked for. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 2898320b1..c529de91a 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include "gtest/gtest.h" #ifdef ENABLE_OTLP_RETRY_PREVIEW @@ -1115,7 +1116,6 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData) 63, 35, 21, 121, 152, 22, 242, 199, 106, 217, 199, 211, 206, 165, 88, 77, 112, 108, 193, 122, 8, 193, 74, 91, 50, 6, 156, 185, 165, 15, 92, 116, 3, 18, 244, 165, 191, 2, 183, 9, 164, 116, 75, 127}; - const auto original_size = body.size(); request->SetBody(body); request->AddHeader("Content-Type", "text/plain"); From e7aef6832939647e012fbf1cd5015767e53961af Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 24 Aug 2026 02:55:50 +0000 Subject: [PATCH 3/5] [TEST] Build the one byte body the way the rest of the file does The case filled its body with an explicit uint8_t, which made the file need a fixed-width integer header. IWYU asks for there and modernize-deprecated-headers, which this repo enables, rejects that header and wants ; the file cannot satisfy both while the type is spelled out. GzipIncompressibleData a few lines below already sizes its body with http_client::Body body(original_size), and only the length matters here: one byte is too small to hold a gzip header, which is what makes the deflate fail. Sizing the body the same way needs no element type and no cast, and leaves no fixed-width type in the file. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index c529de91a..a98746855 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -2,7 +2,6 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include #include "gtest/gtest.h" #ifdef ENABLE_OTLP_RETRY_PREVIEW @@ -1005,7 +1004,7 @@ TEST_F(BasicCurlHttpTests, AFailedCompressionStopsTheRequest) request->SetUri("post/"); request->SetMethod(http_client::Method::Post); - http_client::Body body(1, static_cast('a')); + http_client::Body body(1); request->SetBody(body); request->AddHeader("Content-Type", "text/plain"); request->SetCompression(opentelemetry::ext::http::client::Compression::kGzip); From 124156537a39c5a1fa54a404e08d92b0e2d7c35e Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 24 Aug 2026 03:14:07 +0000 Subject: [PATCH 4/5] [TEST] Drop a guard inside the case that can never be reached The case carried its own ENABLE_OTLP_COMPRESSION_PREVIEW check with a GTEST_SKIP for the other direction. The file already wraps this whole region, GzipEventHandler and the two other gzip cases included, in that same macro, so when it is undefined the case does not exist and the skip cannot run. The two cases beside it have no such guard. Bazel is where the difference shows: it does not define the macro, and the test binary there registers 26 cases where the CMake build with compression on registers 29. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index a98746855..2dbccbf53 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -994,7 +994,6 @@ struct GzipEventHandler : public CustomEventHandler // Z_BUF_ERROR. No allocator hook and no timing are involved. TEST_F(BasicCurlHttpTests, AFailedCompressionStopsTheRequest) { -# ifdef ENABLE_OTLP_COMPRESSION_PREVIEW received_requests_.clear(); auto session_manager = std::make_shared()->Create(); ASSERT_TRUE(session_manager != nullptr); @@ -1027,9 +1026,6 @@ TEST_F(BasicCurlHttpTests, AFailedCompressionStopsTheRequest) std::unique_lock lk1(mtx_requests); EXPECT_TRUE(received_requests_.empty()) << "a request the caller was told had failed was sent anyway"; -# else - GTEST_SKIP() << "gzip is not compiled in, so there is no compression step to fail"; -# endif // ENABLE_OTLP_COMPRESSION_PREVIEW } TEST_F(BasicCurlHttpTests, GzipCompressibleData) From fbb938708d3146d006c4aba392fd2ec6a321e121 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:29:34 +0000 Subject: [PATCH 5/5] [CHORE] Shorten the comments to what the code does not already say The comments on the failure path explained how the code came to be shaped that way. That belongs in this PR and in the commit history, not beside the code, and it left nine lines of comment against seven of code. What is left is the two things a reader cannot get from the code: why the reason is copied before deflateEnd(), and why nothing may touch the session after the handler runs. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 13 +++++------ ext/test/http/curl_http_test.cc | 23 +++++--------------- 2 files changed, 10 insertions(+), 26 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index c08f3a557..d900c74b1 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -184,13 +184,12 @@ void Session::SendRequest( if (stream != Z_OK) { - // Taken before deflateEnd(), which releases what zs.msg points at. + // zs.msg points into the stream deflateEnd() releases. const std::string reason = (nullptr != zs.msg) ? zs.msg : ""; deflateEnd(&zs); - // Before the handler runs, because the handler is application code: it can start another - // request on this session or drop the last reference to it, and nothing below this point - // may touch the session afterwards. + // The handler may start another request on this session or drop the last reference to it, + // so nothing below this point may touch the session. is_session_active_.store(false, std::memory_order_release); if (callback) @@ -198,10 +197,8 @@ void Session::SendRequest( callback->OnEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, reason); } - // The caller has been told this request failed, so it does not go out. The body has also - // been partly rewritten in place by the deflate that failed and carries no Content-Encoding - // header, so what would have been sent is neither what the caller asked for nor readable as - // gzip at the other end. + // deflateInPlace() rewrote part of the body before reporting that it would not fit, and no + // Content-Encoding header describes what is left. The request does not go out. return; } diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 2dbccbf53..5646a9b4c 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -984,14 +984,10 @@ struct GzipEventHandler : public CustomEventHandler std::string reason_; }; -// A request whose compression step fails is reported as a failed create and then must not be -// sent. It used to be reported and sent anyway, with a body the failed deflate had already -// partly rewritten and no Content-Encoding header to describe it. +// A request whose compression step fails reports CreateFailed and is not sent. // -// The failure is arranged rather than injected. deflateInPlace() is given the body's own size as -// its output budget, and its comment says it fails when that budget is smaller than the header -// plus one byte, so a body of one byte cannot fit a gzip header and the deflate reports -// Z_BUF_ERROR. No allocator hook and no timing are involved. +// The failure is arranged, not injected: deflateInPlace() gets the body's own size as its output +// budget, and one byte cannot hold a gzip header, so it returns Z_BUF_ERROR. TEST_F(BasicCurlHttpTests, AFailedCompressionStopsTheRequest) { received_requests_.clear(); @@ -1118,17 +1114,8 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData) auto handler = std::make_shared(); session->SendRequest(handler); - // This used to require a response, on the reading that data which will not compress is sent - // uncompressed. The size assertion below is what that rested on, and the size is not the whole - // story: deflateInPlace() writes into the caller's buffer before it reports that the result - // would not fit, so the body keeps its length while its contents are no longer the caller's - // bytes. What went out was neither the payload nor a gzip stream, and carried no - // Content-Encoding header to describe itself either way. - // - // Refusing to send it is the narrow reading of "do not send after reporting a failure". Keeping - // the payload instead, by compressing into a separate buffer or by restoring the original on - // failure, would let incompressible data still be sent and costs a copy. That is a decision - // rather than a repair, so it is #4360 rather than this change. + // deflateInPlace() overwrites part of the caller's buffer before reporting that the result will + // not fit, so no payload survives to send uncompressed. Whether to keep one is #4360. ASSERT_TRUE(handler->is_called_); ASSERT_EQ(handler->state_, http_client::SessionState::CreateFailed);