{{ message }}
test: make gzip decompression-bomb regression test actually bite#7655
Open
anxkhn wants to merge 1 commit into
Open
test: make gzip decompression-bomb regression test actually bite#7655anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
krunaljain
reviewed
Jun 30, 2026
There was a problem hiding this comment.
TotalAlloc is a process-wide counter. Is this assertion robust if other goroutines allocate during the call, or if the package's tests run with in parallel?
TestParseProtoReader_GzipDecompressionBomb only asserted that an error was returned, which passes whether or not the io.LimitReader cap on gzip output is present. The compressed input is already bounded by an outer LimitReader and a truncated bomb is rejected by the deferred size check regardless, so neither the returned error nor the bytes read from the source distinguish the two cases. The only real signal is how much is decompressed into memory. Replace the test with a white-box TestDecompressRequest_GzipDecompressionBomb that calls decompressRequest directly and asserts the buffered body never exceeds maxSize+1. This is deterministic and parallel-safe, unlike the process-wide, monotonic runtime.MemStats.TotalAlloc, which concurrent goroutines or parallel tests would inflate. Removing the cap inflates the bomb to ~4 MB in the buffer, blowing past maxSize+1 and failing the test. Also add TestParseProtoReader_Gzip covering the under-cap decode and over-cap rejection through the public API, which had no gzip coverage. Fixes cortexproject#7581. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Author
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
2 participants
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.
What this PR does:
TestParseProtoReader_GzipDecompressionBombwas added in #7515 to guard theio.LimitReader(gzReader, maxSize+1)cap on gzip decompression, but it onlyasserted
assert.NotNil(t, err). That passes whether or not the cap is present:decompressRequest's deferredlen(body) > maxSizecheck rejects the oversizedbody regardless, so the test never exercised the new wrapper and could silently
go to a no-op if the cap were removed.
This makes the test bite by asserting decompression stays bounded: with a tiny
gzip payload that inflates to 32 MB, the call must allocate under ~1 MB, which
only holds when the inner cap stops decompression early. Removing the cap blows
past the bound and fails the test (verified: ~16.8 MB allocated vs the 1 MB
bound).
It also adds
TestParseProtoReader_Gzipcovering the under-cap decode (passes)and over-cap rejection, since the
TestParseProtoReadertable had no gzip caseat all.
Test-only change; no production code, config, or flags touched.
Which issue(s) this PR fixes:
Fixes #7581
Checklist
CHANGELOG.mdupdated (n/a, not user-facing)docs/configuration/v1-guarantees.mdupdated (n/a, no flags)