Skip to content

test: make gzip decompression-bomb regression test actually bite#7655

Open
anxkhn wants to merge 1 commit into
cortexproject:masterfrom
anxkhn:loop/cortex__002
Open

test: make gzip decompression-bomb regression test actually bite#7655
anxkhn wants to merge 1 commit into
cortexproject:masterfrom
anxkhn:loop/cortex__002

Conversation

@anxkhn

@anxkhn anxkhn commented Jun 29, 2026

Copy link
Copy Markdown

What this PR does:

TestParseProtoReader_GzipDecompressionBomb was added in #7515 to guard the
io.LimitReader(gzReader, maxSize+1) cap on gzip decompression, but it only
asserted assert.NotNil(t, err). That passes whether or not the cap is present:
decompressRequest's deferred len(body) > maxSize check rejects the oversized
body 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_Gzip covering the under-cap decode (passes)
and over-cap rejection, since the TestParseProtoReader table had no gzip case
at all.

Test-only change; no production code, config, or flags touched.

AI assistance: this change was prepared with help from an AI coding assistant.
I reviewed every line, validated the regression behavior locally, and can
explain the design (per GENAI_POLICY.md).

Which issue(s) this PR fixes:
Fixes #7581

Checklist

  • Tests updated
  • Documentation added (n/a, test-only)
  • CHANGELOG.md updated (n/a, not user-facing)
  • docs/configuration/v1-guarantees.md updated (n/a, no flags)

Comment thread pkg/util/http_test.go Outdated
assert.NotNil(t, err)

runtime.ReadMemStats(&m2)
allocated := m2.TotalAlloc - m1.TotalAlloc

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@anxkhn anxkhn force-pushed the loop/cortex__002 branch from a697c7f to 78dafe4 Compare July 1, 2026 10:41
@anxkhn

anxkhn commented Jul 1, 2026

Copy link
Copy Markdown
Author

Good point, TotalAlloc is process-wide and monotonic, so a parallel test or a background goroutine allocating during the call could inflate the delta and make the bound meaningless. Digging in, the source-read count doesn't work either: an outer io.LimitReader already caps the compressed input to maxSize+1, so bytes read from source are nearly identical with or without the inner cap (4096 vs 4097) and both paths error. The only signal that reflects the guard is how much gets decompressed into memory. I replaced the memory heuristic with a white-box TestDecompressRequest_GzipDecompressionBomb that calls decompressRequest directly and asserts len(body) <= maxSize+1. It is deterministic and parallel-safe (just a local slice length, no runtime stats) and still bites: dropping the io.LimitReader(gzReader, maxSize+1) cap inflates the buffer to ~4 MB and fails with 4204111 is not less than or equal to 4097.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

V04 gzip-cap regression test in pkg/util/http_test.go passes whether or not the new decompressed-size cap is in place

2 participants