[BUGFIX] Stop a curl request after its gzip step fails - #4457
Open
thc1006 wants to merge 1 commit into
Open
Conversation
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 open-telemetry#4360. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006
marked this pull request as ready for review
August 19, 2026 20:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch stops a curl request whose gzip step failed, instead of reporting the failure to the handler and then sending the request anyway.
Session::SendRequestcompresses the body, and on failure it dispatchesCreateFailed, marks the session inactive, and then carries straight on to build the operation and send. There is noreturn. So the caller is told the request failed and the request goes out regardless.What actually goes on the wire
The part I had not expected is that the body being sent is not the caller's data.
deflateInPlace()compresses into the caller's own buffer. It has to write into that buffer to find out whether the result will fit, so by the time it reports that it will not, the buffer has already been written. Its own comment says when that happens:The caller only resizes the body when the compression succeeded, so on failure the body keeps its original length and loses its contents. No
Content-Encodingheader is set on that path either. The peer receives bytes that are neither the payload nor a gzip stream, with nothing to say which it should expect.I measured it on this branch before changing anything, for bodies of 1, 500 and 5000 bytes of high entropy data:
About
GzipIncompressibleDataThat case changes here, and it is worth being explicit rather than quiet about it.
It required a response, on the reading that data which will not compress is simply sent uncompressed, and what it checked was that the body kept its original size. That assertion holds and the conclusion does not: the size is unchanged and the contents are not. So the behaviour it has been pinning is a request that goes out with a body the compression step overwrote.
It now requires
CreateFailedand no request at the server. If you would rather incompressible data still be sent, that is the alternative below rather than a smaller version of this change.About keeping the payload
Refusing to send is the narrow reading of "do not send after reporting a failure". The other reading is that incompressible data should still go out, uncompressed, which is what the old case describes and what a user with high entropy telemetry would want.
That needs the original bytes to survive the attempt, either by compressing into a separate buffer or by copying the body first. Both cost a copy on the compression path, and it changes what a user gets rather than repairing what the code already promises, so I have left it in #4360 rather than deciding it here. Say the word if you would prefer it and I will do that instead.
Tests
AFailedCompressionStopsTheRequestsends a one byte body with gzip enabled and requiresCreateFailed, and then requires the server to have received nothing.The failure is arranged rather than injected.
deflateInPlace()is given the body's own size as its output budget, so a one byte body cannot hold a gzip header and the deflate reportsZ_BUF_ERROR. There is no allocator hook and no timing involved. The case asserts that the failure was reached before it checks anything else, so a change that made such a body compressible would fail the case rather than quietly leave it testing nothing.The whole
curl_http_testsuite passes, 29 of 29, withWITH_OTLP_HTTP_COMPRESSION=ON. The case is compiled in either way and skips at runtime when compression is off, because a case an#ifdefremoves still gets a CTest entry and reports a pass without running.@lalitb noted on #4448 that this one can proceed independently of the transport design discussion. It does not change the structure of the curl client and does not pre-commit anything there.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes