test: make gzip decompression-bomb regression test actually bite#7655
test: make gzip decompression-bomb regression test actually bite#7655anxkhn wants to merge 1 commit into
Conversation
| assert.NotNil(t, err) | ||
|
|
||
| runtime.ReadMemStats(&m2) | ||
| allocated := m2.TotalAlloc - m1.TotalAlloc |
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>
|
Good point, |
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)