diff --git a/.github/workflows/validations.yaml b/.github/workflows/validations.yaml index 358cbadbf..95092a476 100644 --- a/.github/workflows/validations.yaml +++ b/.github/workflows/validations.yaml @@ -27,7 +27,7 @@ jobs: # enough; a 4 vCPU instance can't pull much more than that anyway. # note: s3-cache intentionally omitted -- PR runs are untrusted and must not write to the # shared cache backend that the trusted release workflow reads from (cache poisoning). - runs-on: &test-runner "runs-on=${{ github.run_id }}/cpu=4+8/ram=16+32/family=m5+m6+m7+m8+c5+c6+c7+c8/spot=price-capacity-optimized/volume=100gb" + runs-on: &test-runner "runs-on=${{ github.run_id }}/cpu=4+8/ram=16+32/family=m6+m7+m8+c6+c7+c8/spot=price-capacity-optimized/volume=100gb" permissions: contents: read steps: diff --git a/syft/pkg/cataloger/arch/parse_alpm_db_test.go b/syft/pkg/cataloger/arch/parse_alpm_db_test.go index 6b26209e8..c147fa330 100644 --- a/syft/pkg/cataloger/arch/parse_alpm_db_test.go +++ b/syft/pkg/cataloger/arch/parse_alpm_db_test.go @@ -492,12 +492,16 @@ func Test_parseMtree_malformedInput(t *testing.T) { // bytes still costs multiples of its own size in heap. Asserting the error would keep passing if the // line cap were removed, because the byte cap would still reject a large enough input eventually. func Test_parseMtree_lineCapBoundsHeap(t *testing.T) { - // newline-only lines are the worst case: minimum bytes per entry, so the most entries per byte - const payloadSize = 16 * intFile.MB - spec := bytes.Repeat([]byte{'\n'}, payloadSize) + // the property is linear in the cap, so it is measured at a small cap and scaled to the shipped + // one. Running it at 300k lines means an uncapped parse costing ~16GB, which under -race is most + // of a CI minute for a test with no concurrency in it. + const ( + lineCap = 4096 + payloadSize = 64 * lineCap + ) - // well inside the byte cap, so only the line cap can stop this - require.Less(t, int64(payloadSize), int64(maxMtreeSize)) + // newline-only lines are the worst case: minimum bytes per entry, so the most entries per byte + spec := bytes.Repeat([]byte{'\n'}, payloadSize) unbounded := testutils.MeasureAlloc(t, func() { // the same listing with the line cap lifted, to prove the fixture is really a bomb @@ -507,19 +511,29 @@ func Test_parseMtree_lineCapBoundsHeap(t *testing.T) { require.Greater(t, unbounded, uint64(payloadSize), "fixture did not actually cost more than its own size; it is no longer a bomb") - bounded := testutils.MeasureAlloc(t, func() { - _, err := parseMtreeWithLimits(gzipOf(t, spec), maxMtreeSize, maxMtreeLines) - require.ErrorIs(t, err, errTooManyMtreeLines) - }) + boundedAt := func(limit int) uint64 { + gz := gzipOf(t, spec) + return testutils.MeasureAlloc(t, func() { + _, err := parseMtreeWithLimits(gz, maxMtreeSize, limit) + require.ErrorIs(t, err, errTooManyMtreeLines) + }) + } + bounded := boundedAt(lineCap) - t.Logf("unbounded allocated %d bytes, bounded allocated %d bytes", unbounded, bounded) + // the marginal cost between two caps, so fixed setup (gzip reader, parser) does not get scaled + perLine := (boundedAt(2*lineCap) - bounded) / lineCap + atShippedCap := perLine * maxMtreeLines + t.Logf("unbounded allocated %d bytes, bounded allocated %d bytes (%d per line, ~%d at the shipped cap)", + unbounded, bounded, perLine, atShippedCap) - // measured at ~238MB against ~16.5GB uncapped, so the cap is doing its job, but note what it - // actually buys: 300k entries still cost a few hundred MB, and that is per concurrent cataloger. - // The budget is set to catch the cap being removed or raised by an order of magnitude, not to pin - // the exact figure. - assert.Less(t, bounded, uint64(512*intFile.MB), + assert.Less(t, bounded, unbounded/10, "the line cap has to stop the parse before it builds an entry per line") + + // measured at ~238MB for the shipped 300k lines, so the cap is doing its job, but note what it + // actually buys: that is still a few hundred MB, per concurrent cataloger. The budget is set to + // catch the cap being raised by an order of magnitude, not to pin the exact figure. + assert.Less(t, atShippedCap, uint64(512*intFile.MB), + "the shipped line cap allows more heap than budgeted") } // Test_parseMtree_sizeCapBoundsHeap is the byte-cap counterpart to the line-cap budget above. @@ -532,9 +546,12 @@ func Test_parseMtree_lineCapBoundsHeap(t *testing.T) { // be ordinary for the byte cap to be the thing under test, and there have to be few enough of them // that the line cap does not fire first. func Test_parseMtree_sizeCapBoundsHeap(t *testing.T) { + // same reasoning as the line cap: measured at a small byte cap, since the shipped 64MB cap means + // gzipping and parsing ~200MB of fixture, which is slow under -race and proves nothing more. const ( + sizeCap = intFile.MB lineLen = 1024 - numLines = 96 * 1024 // ~96MB across ~98k lines: past the 64MB cap, well under the 300k line cap + numLines = 2 * sizeCap / lineLen // twice the cap, and far under the line cap ) var raw bytes.Buffer @@ -542,7 +559,7 @@ func Test_parseMtree_sizeCapBoundsHeap(t *testing.T) { for range numLines { raw.Write(line) } - require.Greater(t, int64(raw.Len()), int64(maxMtreeSize), "fixture must exceed the byte cap") + require.Greater(t, int64(raw.Len()), int64(sizeCap), "fixture must exceed the byte cap") require.Less(t, numLines, maxMtreeLines, "and must not trip the line cap first") unbounded := testutils.MeasureAlloc(t, func() { @@ -554,11 +571,13 @@ func Test_parseMtree_sizeCapBoundsHeap(t *testing.T) { "fixture did not actually cost more than its own size; it is no longer a bomb") bounded := testutils.MeasureAlloc(t, func() { - _, err := parseMtreeWithLimits(gzipOf(t, raw.Bytes()), maxMtreeSize, maxMtreeLines) + _, err := parseMtreeWithLimits(gzipOf(t, raw.Bytes()), sizeCap, maxMtreeLines) require.ErrorIs(t, err, errMtreeTooLarge) }) t.Logf("unbounded allocated %d bytes, bounded allocated %d bytes", unbounded, bounded) - assert.Less(t, bounded, uint64(512*intFile.MB), + + // the old budget was 512MB against the 64MB cap, so 8x the cap + assert.Less(t, bounded, uint64(8*sizeCap), "the size cap has to stop the read at the cap, not after the member is drained") } diff --git a/syft/pkg/cataloger/debian/parse_deb_archive_test.go b/syft/pkg/cataloger/debian/parse_deb_archive_test.go index ad8c00ddf..8b916a476 100644 --- a/syft/pkg/cataloger/debian/parse_deb_archive_test.go +++ b/syft/pkg/cataloger/debian/parse_deb_archive_test.go @@ -373,9 +373,11 @@ func Test_processDataTar_capsCopyrightFiles(t *testing.T) { // than after the fact. The existing bounds test uses a payload small enough that buffering the whole // thing would pass anyway; this one is large enough that the difference shows up as bytes. func Test_decompressionStream_failsEarlyInsteadOfBuffering(t *testing.T) { - const expanded = 512 * intFile.MB + // 4x the 8MB budget below is plenty to tell bounded from unbounded. Going bigger only makes the + // fixture slower to build and inflate, which is most of this test's time under -race. + const expanded = 32 * intFile.MB - // well under 1% of input expanding to 512MB, so heap is what separates a streaming bound from a + // well under 1% of input expanding to 32MB, so heap is what separates a streaming bound from a // bound checked after the read completes var gz bytes.Buffer gw := gzip.NewWriter(&gz) diff --git a/syft/pkg/cataloger/golang/scan_binary_bomb_test.go b/syft/pkg/cataloger/golang/scan_binary_bomb_test.go index 6369bbda7..d11ffe7fa 100644 --- a/syft/pkg/cataloger/golang/scan_binary_bomb_test.go +++ b/syft/pkg/cataloger/golang/scan_binary_bomb_test.go @@ -20,19 +20,25 @@ import ( // Test_getBuildInfo_compressedSectionBomb covers the reason readBuildInfo exists: debug/buildinfo opens // ELF files with debug/elf itself, and elf.NewFile expands the section-name string table as it parses, // so an oversized compression header there is an unbounded allocation on a path elfutil.NewFile never -// sees. The fixture is a real zlib stream, so it delivers every byte its header promises. +// sees. The fixture is a real zlib stream, but it stops short of what its header promises: delivering +// the full 256MB only made it slower to build and inflate, mostly under -race. func Test_getBuildInfo_compressedSectionBomb(t *testing.T) { - const declared = 256 << 20 // comfortably over elfutil's bound, small enough to allocate in a test + const ( + declared = 256 << 20 // comfortably over elfutil's bound, which only reads the header + // debug/elf allocates the declared size up front today, so delivery barely matters. Twice the + // guarded budget keeps the fixture a bomb if it ever switches to reading in chunks. + delivered = 64 << 20 + ) - bomb := elfWithCompressedNameTable(t, declared) - t.Logf("%d byte fixture declares a %d byte section name table", len(bomb), declared) + bomb := elfWithCompressedNameTable(t, declared, delivered) + t.Logf("%d byte fixture declares a %d byte section name table and delivers %d", len(bomb), declared, delivered) // the unguarded path is the thing being defended against: prove the fixture really is a bomb unguarded := testutils.MeasureAlloc(t, func() { _, err := buildinfo.Read(bytes.NewReader(bomb)) t.Logf("buildinfo.Read err: %v", err) }) - assert.Greater(t, unguarded, uint64(declared), "fixture did not actually deliver the declared bytes") + assert.Greater(t, unguarded, uint64(delivered), "fixture did not actually deliver its bytes") guarded := testutils.MeasureAlloc(t, func() { _, err := getBuildInfo(bytes.NewReader(bomb)) @@ -47,17 +53,22 @@ func Test_getBuildInfo_compressedSectionBomb(t *testing.T) { // goversion opens the file itself and reads .symtab plus the string table it links, and those are // expanded lazily, so the section-name table bound getBuildInfo applies never reaches them. func Test_getCryptoInformation_compressedSymtabBomb(t *testing.T) { - const declared = 256 << 20 // comfortably over elfutil's bound, small enough to allocate in a test + const ( + declared = 256 << 20 // comfortably over elfutil's bound, which only reads the header + // debug/elf allocates the declared size up front today, so delivery barely matters. Twice the + // guarded budget keeps the fixture a bomb if it ever switches to reading in chunks. + delivered = 64 << 20 + ) - bomb := elfWithCompressedSymtab(t, declared) - t.Logf("%d byte fixture declares a %d byte symbol table", len(bomb), declared) + bomb := elfWithCompressedSymtab(t, declared, delivered) + t.Logf("%d byte fixture declares a %d byte symbol table and delivers %d", len(bomb), declared, delivered) // the unguarded path is the thing being defended against: prove the fixture really is a bomb unguarded := testutils.MeasureAlloc(t, func() { _, err := version.ReadExeFromReader(bytes.NewReader(bomb)) t.Logf("goversion err: %v", err) }) - assert.Greater(t, unguarded, uint64(declared), "fixture did not actually deliver the declared bytes") + assert.Greater(t, unguarded, uint64(delivered), "fixture did not actually deliver its bytes") guarded := testutils.MeasureAlloc(t, func() { _, err := getCryptoInformation(bytes.NewReader(bomb)) @@ -85,12 +96,12 @@ func Test_getCryptoInformation_passesThroughNonELF(t *testing.T) { } // elfWithCompressedSymtab builds a minimal ELF64 whose .symtab is SHF_COMPRESSED, declaring `declared` -// decompressed bytes and genuinely delivering them. The name table is left uncompressed so the file gets +// decompressed bytes and genuinely delivering `delivered` of them. The name table is left uncompressed so the file gets // past the bound getBuildInfo already applies, which is the point: this is the section that bound misses. -func elfWithCompressedSymtab(t *testing.T, declared uint64) []byte { +func elfWithCompressedSymtab(t *testing.T, declared, delivered uint64) []byte { t.Helper() - payload := make([]byte, declared) + payload := make([]byte, delivered) var compressed bytes.Buffer zw := zlib.NewWriter(&compressed) _, err := zw.Write(payload) @@ -150,12 +161,12 @@ func elfWithCompressedSymtab(t *testing.T, declared uint64) []byte { } // elfWithCompressedNameTable builds a minimal ELF64 whose only real section is a SHF_COMPRESSED .shstrtab -// declaring `declared` decompressed bytes and genuinely delivering them. Only the test that runs the +// declaring `declared` decompressed bytes and genuinely delivering `delivered` of them. Only the test that runs the // unguarded path needs delivery; use elfDeclaringNameTable everywhere else, since building this one costs -// `declared` bytes of allocation plus a zlib compress of them. -func elfWithCompressedNameTable(t *testing.T, declared uint64) []byte { +// `delivered` bytes of allocation plus a zlib compress of them. +func elfWithCompressedNameTable(t *testing.T, declared, delivered uint64) []byte { t.Helper() - return elfNameTableFixture(t, declared, true) + return elfNameTableFixture(t, declared, delivered) } // elfDeclaringNameTable declares `declared` bytes without delivering them. CheckSectionNameTable reads the @@ -163,19 +174,15 @@ func elfWithCompressedNameTable(t *testing.T, declared uint64) []byte { // bound itself never needs the stream to be real. func elfDeclaringNameTable(t *testing.T, declared uint64) []byte { t.Helper() - return elfNameTableFixture(t, declared, false) + return elfNameTableFixture(t, declared, 64) } -func elfNameTableFixture(t *testing.T, declared uint64, deliver bool) []byte { +func elfNameTableFixture(t *testing.T, declared, delivered uint64) []byte { t.Helper() // the decompressed name table only has to start with the section names; the rest is padding that // exists purely to make the declared size real - size := declared - if !deliver { - size = 64 - } - payload := make([]byte, size) + payload := make([]byte, delivered) copy(payload, "\x00.shstrtab\x00") var compressed bytes.Buffer diff --git a/syft/pkg/cataloger/snap/parse_kernel_changelog_test.go b/syft/pkg/cataloger/snap/parse_kernel_changelog_test.go index a1806a822..0e3c17c1a 100644 --- a/syft/pkg/cataloger/snap/parse_kernel_changelog_test.go +++ b/syft/pkg/cataloger/snap/parse_kernel_changelog_test.go @@ -318,7 +318,9 @@ func TestParseKernelChangelog(t *testing.T) { // thing holding this path, and it is incidental: switching to io.ReadAll or a bufio.Reader to pick up // long lines would remove it silently, which is what this test is here to catch. func TestParseKernelChangelog_gzipBombStaysBounded(t *testing.T) { - const expanded = 512 * 1024 * 1024 + // 4x the 8MB budget below is plenty to tell bounded from unbounded. Going bigger only makes the + // fixture slower to build and inflate, which is most of this test's time under -race. + const expanded = 32 * 1024 * 1024 // one enormous line with no newline, the worst case for a line scanner var gz bytes.Buffer