diff --git a/.golangci.yaml b/.golangci.yaml index 422a7448a..6b9bf74b0 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -32,6 +32,10 @@ linters: funlen: lines: 70 statements: 50 + goconst: + # the default of 3 turns every repeated literal in a lookup table or a test fixture into a finding. + # 5 keeps the ones worth hoisting without flagging data that reads better written out. + min-occurrences: 5 gocritic: enabled-checks: - deferInLoop @@ -54,6 +58,12 @@ linters: - linters: - revive text: "var-naming: avoid package names that conflict" + # cpegenerate is almost entirely lookup tables mapping artifact names to vendors, products, and + # maven group ids. The repeated strings are the data itself, so hoisting them to constants would + # replace a readable table with indirection for no gain. + - linters: + - goconst + path: syft/pkg/cataloger/internal/cpegenerate/ paths: - third_party$ - builtin$ diff --git a/.make/go.mod b/.make/go.mod index 787e6d0cc..b67a4d3a0 100644 --- a/.make/go.mod +++ b/.make/go.mod @@ -2,11 +2,11 @@ module github.com/anchore/syft/.make go 1.25.8 -require github.com/anchore/go-make v0.8.0 +require github.com/anchore/go-make v0.8.1 require ( github.com/bmatcuk/doublestar/v4 v4.10.0 // indirect github.com/goccy/go-yaml v1.19.2 // indirect - golang.org/x/mod v0.37.0 // indirect - golang.org/x/sys v0.46.0 // indirect + golang.org/x/mod v0.40.0 // indirect + golang.org/x/sys v0.47.0 // indirect ) diff --git a/.make/go.sum b/.make/go.sum index 72d002cfc..f8fd03437 100644 --- a/.make/go.sum +++ b/.make/go.sum @@ -1,10 +1,10 @@ -github.com/anchore/go-make v0.8.0 h1:3XAD4WNHIbqY4kwFyCQJ7kKiYLzSzjB0UwzDVwRqlUY= -github.com/anchore/go-make v0.8.0/go.mod h1:4M6TnArb5w693VyWsgr5dCWrk2BLNu/ed4JUcsrzS34= +github.com/anchore/go-make v0.8.1 h1:GAhPXR+9qVQhFB1pfVlHFOzxAt3dTDTUIA7Tp7nx9tg= +github.com/anchore/go-make v0.8.1/go.mod h1:xALbVsPsDoJnJxdkjPlxOLZV7GbAdT5Pv3gd0kP1Lz0= github.com/bmatcuk/doublestar/v4 v4.10.0 h1:zU9WiOla1YA122oLM6i4EXvGW62DvKZVxIe6TYWexEs= github.com/bmatcuk/doublestar/v4 v4.10.0/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc= github.com/goccy/go-yaml v1.19.2 h1:PmFC1S6h8ljIz6gMRBopkjP1TVT7xuwrButHID66PoM= github.com/goccy/go-yaml v1.19.2/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA= -golang.org/x/mod v0.37.0 h1:vF1DjpVEshcIqoEaauuHebaLk1O1forxjxBaVn884JQ= -golang.org/x/mod v0.37.0/go.mod h1:m8S8VeM9r4dzDwjrKO0a1sZP3YjeMamRRlD+fmR2Q/0= -golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= -golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/mod v0.40.0 h1:hUv+3cXcdRHz08UmSiOob7sadHig73uo5bkXxQ/tvUs= +golang.org/x/mod v0.40.0/go.mod h1:0/weTWkPWGBikyTWAX3dkjVztMmBA5hM0DH6BElSupE= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= diff --git a/cmd/syft/internal/commands/cataloger_info.go b/cmd/syft/internal/commands/cataloger_info.go index 57e0f2744..2a9db6280 100644 --- a/cmd/syft/internal/commands/cataloger_info.go +++ b/cmd/syft/internal/commands/cataloger_info.go @@ -349,8 +349,8 @@ func renderCatalogerInfoTable(_ *capabilities.Document, catalogers []capabilitie })), tablewriter.WithConfig(tablewriter.Config{ Row: tw.CellConfig{ - Formatting: tw.CellFormatting{MergeMode: tw.MergeHierarchical}, - Alignment: tw.CellAlignment{Global: tw.AlignLeft}, + Merging: tw.CellMerging{Mode: tw.MergeHierarchical}, + Alignment: tw.CellAlignment{Global: tw.AlignLeft}, }, }), ) diff --git a/internal/spillbuf/buffer_test.go b/internal/spillbuf/buffer_test.go index 445022df0..d71255f75 100644 --- a/internal/spillbuf/buffer_test.go +++ b/internal/spillbuf/buffer_test.go @@ -8,12 +8,12 @@ import ( "math/rand" "os" "path/filepath" - "runtime" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/anchore/syft/internal/testutils" "github.com/anchore/syft/internal/tmpdir" ) @@ -161,13 +161,9 @@ func TestSparsePlacementIsNotAnAllocationKnob(t *testing.T) { b := newTestBuffer(t, WithMemLimit(1<<20)) mustWrite(t, b, 0, repeat('A', 64)) - var before, after runtime.MemStats - runtime.GC() - runtime.ReadMemStats(&before) - mustWrite(t, b, far, repeat('B', 64)) - runtime.ReadMemStats(&after) - - allocated := after.TotalAlloc - before.TotalAlloc + allocated := testutils.MeasureAlloc(t, func() { + mustWrite(t, b, far, repeat('B', 64)) + }) t.Logf("writing 64 bytes at offset %d allocated %d bytes", far, allocated) assert.Less(t, allocated, uint64(1<<20), "a far placement must cost its own bytes, not its offset") @@ -714,15 +710,11 @@ func TestMemoryTierGrowsAmortized(t *testing.T) { ) b := newTestBuffer(t, WithMemLimit(limit)) - var before, after runtime.MemStats - runtime.GC() - runtime.ReadMemStats(&before) - for off := int64(0); off < limit; off += chunk { - mustWrite(t, b, off, repeat('A', chunk)) - } - runtime.ReadMemStats(&after) - - allocated := after.TotalAlloc - before.TotalAlloc + allocated := testutils.MeasureAlloc(t, func() { + for off := int64(0); off < limit; off += chunk { + mustWrite(t, b, off, repeat('A', chunk)) + } + }) t.Logf("filling a %d byte tier in %d byte chunks allocated %d bytes", limit, chunk, allocated) assert.Less(t, allocated, uint64(4*limit), "growth has to be amortized, not a fresh copy of the whole tier per write") diff --git a/internal/testutils/alloc.go b/internal/testutils/alloc.go new file mode 100644 index 000000000..c84200f10 --- /dev/null +++ b/internal/testutils/alloc.go @@ -0,0 +1,31 @@ +package testutils + +import ( + "runtime" + "testing" +) + +// MeasureAlloc reports the bytes allocated while fn ran. +// +// This exists for guards whose whole point is "a small input cannot make us reserve a large buffer". +// Only a byte count states that property: asserting that an error comes back would keep passing if the +// allocation were hoisted above the check, which is the bug these guards were written for. +// +// Two things to know when using it: +// +// - TotalAlloc is process-wide, so a test calling this must not call t.Parallel. Another test's +// allocations would land in the measurement. +// - a budget assertion is only meaningful if the fixture would really blow past it unguarded. Prefer +// measuring the unguarded path too and asserting it exceeds the declared size, so a fixture that +// quietly stops being a bomb fails instead of passing. +func MeasureAlloc(t testing.TB, fn func()) uint64 { + t.Helper() + + var before, after runtime.MemStats + runtime.GC() + runtime.ReadMemStats(&before) + fn() + runtime.ReadMemStats(&after) + + return after.TotalAlloc - before.TotalAlloc +} diff --git a/syft/format/internal/cyclonedxutil/helpers/property_encoder.go b/syft/format/internal/cyclonedxutil/helpers/property_encoder.go index dd442f6e3..2aeb8b6d0 100644 --- a/syft/format/internal/cyclonedxutil/helpers/property_encoder.go +++ b/syft/format/internal/cyclonedxutil/helpers/property_encoder.go @@ -93,7 +93,7 @@ func encode(out map[string]string, value reflect.Value, prefix string, fn FieldN typ := value.Type() switch typ.Kind() { - case reflect.Ptr: + case reflect.Pointer: if value.IsNil() { return } @@ -158,7 +158,7 @@ func fieldName(f reflect.StructField, prefix string, fn FieldName) (string, bool // Decode based on the given type, applies all values to hydrate a new instance func Decode(typ reflect.Type, values map[string]string, prefix string, fn FieldName) any { isPtr := false - for typ.Kind() == reflect.Ptr { + for typ.Kind() == reflect.Pointer { typ = typ.Elem() isPtr = true } @@ -188,7 +188,7 @@ func Decode(typ reflect.Type, values map[string]string, prefix string, fn FieldN func DecodeInto(obj any, values map[string]string, prefix string, fn FieldName) { value := reflect.ValueOf(obj) - for value.Type().Kind() == reflect.Ptr { + for value.Type().Kind() == reflect.Pointer { value = value.Elem() } @@ -205,7 +205,7 @@ func decode(vals map[string]string, value reflect.Value, prefix string, fn Field incoming, valid := vals[prefix] switch typ.Kind() { - case reflect.Ptr: + case reflect.Pointer: t := typ.Elem() v := value if v.IsNil() { @@ -269,13 +269,13 @@ func decode(vals map[string]string, value reflect.Value, prefix string, fn Field str := fmt.Sprintf("%s:%d", prefix, idx) // create new placeholder and decode values newType := t - if t.Kind() == reflect.Ptr { + if t.Kind() == reflect.Pointer { newType = t.Elem() } v := reflect.New(newType) if decode(vals, v.Elem(), str, fn) { // append to slice - if t.Kind() != reflect.Ptr { + if t.Kind() != reflect.Pointer { v = v.Elem() } slice = reflect.Append(slice, v) @@ -304,7 +304,7 @@ func decode(vals map[string]string, value reflect.Value, prefix string, fn Field keyVals[key] = after // create new placeholder and decode key newKeyType := keyType - if keyType.Kind() == reflect.Ptr { + if keyType.Kind() == reflect.Pointer { newKeyType = keyType.Elem() } k := reflect.New(newKeyType) @@ -312,18 +312,18 @@ func decode(vals map[string]string, value reflect.Value, prefix string, fn Field log.Debugf("unable to decode key for: %s", key) continue } - if keyType.Kind() != reflect.Ptr { + if keyType.Kind() != reflect.Pointer { k = k.Elem() } // create new placeholder and decode value newValueType := valueType - if valueType.Kind() == reflect.Ptr { + if valueType.Kind() == reflect.Pointer { newValueType = valueType.Elem() } v := reflect.New(newValueType) if decode(vals, v.Elem(), key, fn) { - if valueType.Kind() != reflect.Ptr { + if valueType.Kind() != reflect.Pointer { v = v.Elem() } @@ -367,7 +367,7 @@ func PtrToStruct(ptr any) any { return nil } switch v.Type().Kind() { - case reflect.Ptr: + case reflect.Pointer: return PtrToStruct(v.Elem().Interface()) case reflect.Interface: return PtrToStruct(v.Elem().Interface()) diff --git a/syft/format/internal/testutil/directory_input.go b/syft/format/internal/testutil/directory_input.go index 22bb91d63..8d4a23460 100644 --- a/syft/format/internal/testutil/directory_input.go +++ b/syft/format/internal/testutil/directory_input.go @@ -16,6 +16,7 @@ import ( "github.com/anchore/syft/syft/source/directorysource" ) +//nolint:goconst // fixture data: the repeated names and versions are what the golden files assert against, so they read better spelled out than behind constants func DirectoryInput(t testing.TB, dir string) sbom.SBOM { catalog := newDirectoryCatalog() @@ -97,6 +98,7 @@ func DirectoryInputWithAuthorField(t testing.TB) sbom.SBOM { } } +//nolint:goconst // fixture data: the repeated names and versions are what the golden files assert against, so they read better spelled out than behind constants func newDirectoryCatalog() *pkg.Collection { catalog := pkg.NewCollection() ctx := context.TODO() diff --git a/syft/format/internal/testutil/image_input.go b/syft/format/internal/testutil/image_input.go index f9ab7e06d..881c0952a 100644 --- a/syft/format/internal/testutil/image_input.go +++ b/syft/format/internal/testutil/image_input.go @@ -19,6 +19,7 @@ import ( "github.com/anchore/syft/syft/source/stereoscopesource" ) +//nolint:goconst // fixture data: the repeated names and versions are what the golden files assert against, so they read better spelled out than behind constants func ImageInput(t testing.TB, testImage string, options ...ImageOption) sbom.SBOM { t.Helper() catalog := pkg.NewCollection() @@ -95,6 +96,7 @@ func changeToDirectoryWithGoldenFixture(t testing.TB, testImage string) func() { return fn } +//nolint:goconst // fixture data: the repeated names and versions are what the golden files assert against, so they read better spelled out than behind constants func populateImageCatalog(catalog *pkg.Collection, img *image.Image) { // TODO: this helper function is coupled to the image-simple fixture, which seems like a bad idea _, ref1, _ := img.SquashedTree().File("/somefile-1.txt", filetree.FollowBasenameLinks) diff --git a/syft/internal/elfutil/elfutil_test.go b/syft/internal/elfutil/elfutil_test.go index fbb9b179d..8d45026a1 100644 --- a/syft/internal/elfutil/elfutil_test.go +++ b/syft/internal/elfutil/elfutil_test.go @@ -7,11 +7,12 @@ import ( "encoding/binary" "fmt" "io" - "runtime" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/anchore/syft/internal/testutils" ) // section describes one section to place in a fixture. The compression headers are deliberately @@ -543,13 +544,10 @@ func TestNewFile_BombDoesNotAllocate(t *testing.T) { {name: ".strtab", typ: elf.SHT_STRTAB}, }, buildOpts{}) - var before, after runtime.MemStats - runtime.GC() - runtime.ReadMemStats(&before) - - _, err := NewFile(bytes.NewReader(data)) - - runtime.ReadMemStats(&after) + var err error + allocated := testutils.MeasureAlloc(t, func() { + _, err = NewFile(bytes.NewReader(data)) + }) require.Error(t, err) assert.Contains(t, err.Error(), "over the") @@ -557,7 +555,7 @@ func TestNewFile_BombDoesNotAllocate(t *testing.T) { // the guard has to reject before the allocation, not after it. The margin is loose on purpose: this // is asserting "nothing near the declared size was allocated", not a precise budget. const margin = 32 * 1024 * 1024 - assert.Less(t, after.TotalAlloc-before.TotalAlloc, uint64(margin), + assert.Less(t, allocated, uint64(margin), "parsing allocated far more than the input warrants") } diff --git a/syft/pkg/cataloger/ai/parse_safetensors_test.go b/syft/pkg/cataloger/ai/parse_safetensors_test.go index 56d50151a..c210bfbe0 100644 --- a/syft/pkg/cataloger/ai/parse_safetensors_test.go +++ b/syft/pkg/cataloger/ai/parse_safetensors_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/anchore/syft/internal/testutils" "github.com/anchore/syft/syft/artifact" "github.com/anchore/syft/syft/file" "github.com/anchore/syft/syft/pkg" @@ -1200,3 +1201,60 @@ func TestDockerAIModelConfigMediaTypes(t *testing.T) { }) } } + +// TestReadSafeTensorsHeader_capBoundsHeap pins maxSafeTensorsHeaderSize as bytes rather than as an +// error string. The existing over-cap case omits the body precisely because the guard rejects before +// allocating, which means it would keep passing if the guard moved after the read. This one delivers +// the bytes, so moving the check shows up as heap. +func TestReadSafeTensorsHeader_capBoundsHeap(t *testing.T) { + const declared = maxSafeTensorsHeaderSize + 1 + + body := bytes.Repeat([]byte{'x'}, declared) + file := make([]byte, 8, 8+len(body)) + binary.LittleEndian.PutUint64(file, uint64(declared)) + file = append(file, body...) + + // the unguarded shape: honor the declared length without weighing it against a ceiling first. + // This is what readSafeTensorsHeader does once the cap is passed, so it is what the cap prevents. + unbounded := testutils.MeasureAlloc(t, func() { + b, err := io.ReadAll(io.LimitReader(bytes.NewReader(file[8:]), declared)) + require.NoError(t, err) + require.Len(t, b, declared) + }) + require.Greater(t, unbounded, uint64(declared), + "fixture did not actually cost more than its own size; it is no longer a bomb") + + bounded := testutils.MeasureAlloc(t, func() { + _, err := readSafeTensorsHeader(bytes.NewReader(file)) + require.ErrorContains(t, err, "exceeds maximum") + }) + + t.Logf("unbounded allocated %d bytes, bounded allocated %d bytes", unbounded, bounded) + assert.Less(t, bounded, uint64(8*1024*1024), + "the declared-size check has to reject before the body is read, not after") +} + +// TestCopyHeader_boundIsTheCallersLimitedReader pins where the GGUF bound actually lives. copyHeader +// io.Copy's whatever it is handed, so maxHeaderSize only holds because every caller wraps the reader +// first. A caller that stops doing that reads the whole file, and nothing in copyHeader would object. +// +// Bytes retained is asserted directly rather than through testutils.MeasureAlloc: the destination is a +// bytes.Buffer, whose doubling growth costs a multiple of what it holds and lands on a different power +// of two depending on the platform. buf.Len() is the same property without that noise. +func TestCopyHeader_boundIsTheCallersLimitedReader(t *testing.T) { + const payload = 2*maxHeaderSize + 1 + + file := make([]byte, 24, 24+payload) + binary.LittleEndian.PutUint32(file[0:4], ggufMagicNumber) + file = append(file, bytes.Repeat([]byte{'x'}, payload)...) + + var unbounded bytes.Buffer + require.NoError(t, copyHeader(&unbounded, bytes.NewReader(file))) + require.Equal(t, len(file), unbounded.Len(), + "an unwrapped reader is copied whole; copyHeader applies no limit of its own") + + var bounded bytes.Buffer + require.NoError(t, copyHeader(&bounded, &io.LimitedReader{R: bytes.NewReader(file), N: maxHeaderSize})) + assert.Equal(t, maxHeaderSize, bounded.Len(), + "a wrapped reader must cap the copy at maxHeaderSize") +} diff --git a/syft/pkg/cataloger/arch/parse_alpm_db_test.go b/syft/pkg/cataloger/arch/parse_alpm_db_test.go index cb591e7aa..6b26209e8 100644 --- a/syft/pkg/cataloger/arch/parse_alpm_db_test.go +++ b/syft/pkg/cataloger/arch/parse_alpm_db_test.go @@ -12,9 +12,11 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" intFile "github.com/anchore/syft/internal/file" + "github.com/anchore/syft/internal/testutils" "github.com/anchore/syft/syft/file" "github.com/anchore/syft/syft/pkg" ) @@ -484,3 +486,79 @@ func Test_parseMtree_malformedInput(t *testing.T) { require.ErrorContains(t, err, "token too long") }) } + +// Test_parseMtree_lineCapBoundsHeap pins the measurement that maxMtreeLines exists for. The byte cap +// alone does not bound this path: go-mtree keeps an entry per line, so a listing well inside 64MB of +// 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) + + // well inside the byte cap, so only the line cap can stop this + require.Less(t, int64(payloadSize), int64(maxMtreeSize)) + + unbounded := testutils.MeasureAlloc(t, func() { + // the same listing with the line cap lifted, to prove the fixture is really a bomb + _, err := parseMtreeWithLimits(gzipOf(t, spec), maxMtreeSize, payloadSize+1) + t.Logf("unbounded parse err: %v", err) + }) + 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) + }) + + t.Logf("unbounded allocated %d bytes, bounded allocated %d bytes", unbounded, bounded) + + // 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), + "the line cap has to stop the parse before it builds an entry per line") +} + +// Test_parseMtree_sizeCapBoundsHeap is the byte-cap counterpart to the line-cap budget above. +// Test_parseMtree_boundsDecompressedSize already covers the cap behaviorally, but with a spec small +// enough that buffering the whole listing would pass anyway. This one expands well past the cap, so a +// bound checked after the read instead of during it shows up as heap. +// +// The line length matters: go-mtree runs its own bufio.Scanner, which refuses a token past 64KB, so a +// single enormous line fails as "token too long" before the byte cap is ever consulted. Lines have to +// 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) { + const ( + lineLen = 1024 + numLines = 96 * 1024 // ~96MB across ~98k lines: past the 64MB cap, well under the 300k line cap + ) + + var raw bytes.Buffer + line := append(bytes.Repeat([]byte{'a'}, lineLen-1), '\n') + for range numLines { + raw.Write(line) + } + require.Greater(t, int64(raw.Len()), int64(maxMtreeSize), "fixture must exceed the byte cap") + require.Less(t, numLines, maxMtreeLines, "and must not trip the line cap first") + + unbounded := testutils.MeasureAlloc(t, func() { + // the same listing with the byte cap lifted past it, to prove the fixture is really a bomb + _, err := parseMtreeWithLimits(gzipOf(t, raw.Bytes()), int64(raw.Len())*2, maxMtreeLines) + t.Logf("unbounded parse err: %v", err) + }) + require.Greater(t, unbounded, uint64(raw.Len()), + "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) + 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 size cap has to stop the read at the cap, not after the member is drained") +} diff --git a/syft/pkg/cataloger/binary/classifiers.go b/syft/pkg/cataloger/binary/classifiers.go index 5fc3c7b80..17834ab0c 100644 --- a/syft/pkg/cataloger/binary/classifiers.go +++ b/syft/pkg/cataloger/binary/classifiers.go @@ -8,6 +8,9 @@ import ( "github.com/anchore/syft/syft/pkg/cataloger/internal/binutils" ) +// mysqlClusterPackage is the package name shared by every MySQL Cluster classifier below. +const mysqlClusterPackage = "mysql-cluster" + // in both binaries and shared libraries, the version pattern is [NUL]3.11.2[NUL] var pythonVersionTemplate = `(?m)\x00(?P{{ .version }}[-._a-zA-Z0-9]*)\x00` @@ -374,7 +377,7 @@ func DefaultClassifiers() []binutils.Classifier { Class: "mysqld-mysql-cluster-legacy-binary", EvidenceMatcher: m.FileContentsVersionMatcher( `cluster-gpl\x00[0-9]+(\.[0-9]+)?(\.[0-9]+)?\-ndb\-(?P[0-9]+(\.[0-9]+)?(\.[0-9]+)?)`), - Package: "mysql-cluster", + Package: mysqlClusterPackage, PURL: mustPURL("pkg:generic/mysql-cluster@version"), CPEs: []cpe.CPE{ cpe.Must("cpe:2.3:a:oracle:mysql_cluster:*:*:*:*:*:*:*:*", cpe.NVDDictionaryLookupSource), @@ -385,7 +388,7 @@ func DefaultClassifiers() []binutils.Classifier { Class: "mysqld-mysql-cluster-binary", EvidenceMatcher: m.FileContentsVersionMatcher( `/mysql-cluster-gpl-(?P[0-9]+(\.[0-9]+)?(\.[0-9]+)?(alpha[0-9]|beta[0-9]|rc[0-9])?)/`), - Package: "mysql-cluster", + Package: mysqlClusterPackage, PURL: mustPURL("pkg:generic/mysql-cluster@version"), CPEs: []cpe.CPE{ cpe.Must("cpe:2.3:a:oracle:mysql:*:*:*:*:*:*:*:*", cpe.NVDDictionaryLookupSource), @@ -412,7 +415,7 @@ func DefaultClassifiers() []binutils.Classifier { FileGlob: "**/ndbd", EvidenceMatcher: m.FileContentsVersionMatcher( `/mysql-cluster-gpl-(?P[0-9]+(\.[0-9]+)?(\.[0-9]+)?(alpha[0-9]|beta[0-9]|rc[0-9])?)/`), - Package: "mysql-cluster", + Package: mysqlClusterPackage, PURL: mustPURL("pkg:generic/mysql-cluster@version"), CPEs: []cpe.CPE{ cpe.Must("cpe:2.3:a:oracle:mysql_cluster:*:*:*:*:*:*:*:*", cpe.NVDDictionaryLookupSource), @@ -423,7 +426,7 @@ func DefaultClassifiers() []binutils.Classifier { FileGlob: "**/ndbmtd", EvidenceMatcher: m.FileContentsVersionMatcher( `/mysql-cluster-gpl-(?P[0-9]+(\.[0-9]+)?(\.[0-9]+)?(alpha[0-9]|beta[0-9]|rc[0-9])?)/`), - Package: "mysql-cluster", + Package: mysqlClusterPackage, PURL: mustPURL("pkg:generic/mysql-cluster@version"), CPEs: []cpe.CPE{ cpe.Must("cpe:2.3:a:oracle:mysql_cluster:*:*:*:*:*:*:*:*", cpe.NVDDictionaryLookupSource), @@ -434,7 +437,7 @@ func DefaultClassifiers() []binutils.Classifier { FileGlob: "**/ndb_mgmd", EvidenceMatcher: m.FileContentsVersionMatcher( `/mysql-cluster-gpl-(?P[0-9]+(\.[0-9]+)?(\.[0-9]+)?(alpha[0-9]|beta[0-9]|rc[0-9])?)/`), - Package: "mysql-cluster", + Package: mysqlClusterPackage, PURL: mustPURL("pkg:generic/mysql-cluster@version"), CPEs: []cpe.CPE{ cpe.Must("cpe:2.3:a:oracle:mysql_cluster:*:*:*:*:*:*:*:*", cpe.NVDDictionaryLookupSource), diff --git a/syft/pkg/cataloger/debian/parse_deb_archive_test.go b/syft/pkg/cataloger/debian/parse_deb_archive_test.go index f90e953d9..7afa0a455 100644 --- a/syft/pkg/cataloger/debian/parse_deb_archive_test.go +++ b/syft/pkg/cataloger/debian/parse_deb_archive_test.go @@ -16,6 +16,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + intFile "github.com/anchore/syft/internal/file" + "github.com/anchore/syft/internal/testutils" "github.com/anchore/syft/internal/unknown" "github.com/anchore/syft/syft/file" "github.com/anchore/syft/syft/pkg/cataloger/generic" @@ -366,3 +368,49 @@ func Test_processDataTar_capsCopyrightFiles(t *testing.T) { require.Error(t, err) } + +// Test_decompressionStream_failsEarlyInsteadOfBuffering pins that the bound is spent on read rather +// 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 + + // a few KB of input expanding to 512MB, so heap is what separates a streaming bound from a + // bound checked after the read completes + var gz bytes.Buffer + gw := gzip.NewWriter(&gz) + chunk := make([]byte, 32*1024) + for remaining := int64(expanded); remaining > 0; { + n, err := gw.Write(chunk[:min(remaining, int64(len(chunk)))]) + require.NoError(t, err) + remaining -= int64(n) + } + require.NoError(t, gw.Close()) + require.Less(t, gz.Len(), 1024*1024, "compressed payload should be tiny relative to what it expands to") + + // both arms use io.ReadAll on purpose. The stream itself never holds much whatever the cap says, + // so copying to io.Discard measures nothing; what the cap actually protects is a consumer that + // buffers, and the real one (a tar reader building entries) does. + unbounded := testutils.MeasureAlloc(t, func() { + rc, err := decompressionStream(context.Background(), bytes.NewReader(gz.Bytes()), "control.tar.gz", expanded*2) + require.NoError(t, err) + defer rc.Close() + b, err := io.ReadAll(rc) + require.NoError(t, err) + require.Len(t, b, expanded) + }) + require.Greater(t, unbounded, uint64(expanded), + "fixture did not actually cost more than its own size; it is no longer a bomb") + + bounded := testutils.MeasureAlloc(t, func() { + rc, err := decompressionStream(context.Background(), bytes.NewReader(gz.Bytes()), "control.tar.gz", 64*1024) + require.NoError(t, err) + defer rc.Close() + _, err = io.ReadAll(rc) + require.ErrorIs(t, err, errDecompressedTooLarge) + }) + + t.Logf("unbounded allocated %d bytes, bounded allocated %d bytes", unbounded, bounded) + assert.Less(t, bounded, uint64(8*intFile.MB), + "a bounded stream must stop at the cap, not buffer the whole member and check afterwards") +} diff --git a/syft/pkg/cataloger/golang/scan_binary_bomb_test.go b/syft/pkg/cataloger/golang/scan_binary_bomb_test.go index 73420896c..6369bbda7 100644 --- a/syft/pkg/cataloger/golang/scan_binary_bomb_test.go +++ b/syft/pkg/cataloger/golang/scan_binary_bomb_test.go @@ -7,13 +7,13 @@ import ( "debug/elf" "encoding/binary" "os" - "runtime" "testing" "github.com/kastenhq/goversion/version" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/anchore/syft/internal/testutils" "github.com/anchore/syft/syft/internal/elfutil" ) @@ -28,13 +28,13 @@ func Test_getBuildInfo_compressedSectionBomb(t *testing.T) { t.Logf("%d byte fixture declares a %d byte section name table", len(bomb), declared) // the unguarded path is the thing being defended against: prove the fixture really is a bomb - unguarded := measureAlloc(t, func() { + 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") - guarded := measureAlloc(t, func() { + guarded := testutils.MeasureAlloc(t, func() { _, err := getBuildInfo(bytes.NewReader(bomb)) require.Error(t, err) assert.ErrorIs(t, err, elfutil.ErrDeclaredSizeExceeded) @@ -53,13 +53,13 @@ func Test_getCryptoInformation_compressedSymtabBomb(t *testing.T) { t.Logf("%d byte fixture declares a %d byte symbol table", len(bomb), declared) // the unguarded path is the thing being defended against: prove the fixture really is a bomb - unguarded := measureAlloc(t, func() { + 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") - guarded := measureAlloc(t, func() { + guarded := testutils.MeasureAlloc(t, func() { _, err := getCryptoInformation(bytes.NewReader(bomb)) require.ErrorIs(t, err, elfutil.ErrDeclaredSizeExceeded) }) @@ -149,18 +149,6 @@ func elfWithCompressedSymtab(t *testing.T, declared uint64) []byte { return buf.Bytes() } -// measureAlloc reports the bytes allocated while fn ran. TotalAlloc is process-wide, so a test using this -// must not call t.Parallel: another test's allocations would land in the measurement. -func measureAlloc(t *testing.T, fn func()) uint64 { - t.Helper() - var before, after runtime.MemStats - runtime.GC() - runtime.ReadMemStats(&before) - fn() - runtime.ReadMemStats(&after) - return after.TotalAlloc - before.TotalAlloc -} - // 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 // unguarded path needs delivery; use elfDeclaringNameTable everywhere else, since building this one costs diff --git a/syft/pkg/cataloger/golang/upx_bounds_regression_test.go b/syft/pkg/cataloger/golang/upx_bounds_regression_test.go index d4e63b488..22beeba32 100644 --- a/syft/pkg/cataloger/golang/upx_bounds_regression_test.go +++ b/syft/pkg/cataloger/golang/upx_bounds_regression_test.go @@ -15,6 +15,7 @@ import ( "github.com/stretchr/testify/require" intFile "github.com/anchore/syft/internal/file" + "github.com/anchore/syft/internal/testutils" "github.com/anchore/syft/internal/tmpdir" "github.com/anchore/syft/internal/unknown" "github.com/anchore/syft/syft/file" @@ -112,7 +113,7 @@ func TestDecompressUPX_SparsePlacementIsNotAnAllocationKnob(t *testing.T) { require.ErrorIs(t, err, errUPXPartial) require.NotNil(t, out) - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { _, _ = getBuildInfo(out) }) diff --git a/syft/pkg/cataloger/golang/upx_security_test.go b/syft/pkg/cataloger/golang/upx_security_test.go index 36981dc72..f4d4f105d 100644 --- a/syft/pkg/cataloger/golang/upx_security_test.go +++ b/syft/pkg/cataloger/golang/upx_security_test.go @@ -17,6 +17,7 @@ import ( intFile "github.com/anchore/syft/internal/file" "github.com/anchore/syft/internal/spillbuf" + "github.com/anchore/syft/internal/testutils" "github.com/anchore/syft/internal/tmpdir" "github.com/anchore/syft/internal/unknown" "github.com/anchore/syft/syft/file" @@ -602,7 +603,7 @@ func TestDecompressUPX_RefusingATinyClaimIsCheap(t *testing.T) { tiny := padTo(buildUPXFile(t, 1<<30, 1<<30, [][]byte{bytes.Repeat([]byte("A"), 32)}, nil), 128) - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { _, err := unpackIn(t, t.TempDir(), tiny) require.Error(t, err, "a 128 byte file may not claim a gigabyte") }) @@ -629,7 +630,7 @@ func TestDecompressUPX_OutputIsNeverResident(t *testing.T) { require.LessOrEqual(t, len(fixture), inputLen, "the fixture must stay small enough for the ratio to bind") var out unpackedContents - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { var err error out, err = unpackIn(t, t.TempDir(), fixture) require.NoError(t, err) @@ -702,7 +703,7 @@ func TestDecompressUPX_CompressedSizeCannotOutrunTheInput(t *testing.T) { b[8] = 14 // b_method = LZMA data := padTo(append(buildUPXHeader(4096, 4096), b...), 128) - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { _, err := unpackIn(t, t.TempDir(), data) require.Error(t, err) assert.ErrorIs(t, err, errUPXImplausibleHeader, "no block was readable, so nothing was packed") @@ -842,7 +843,7 @@ func TestScanReader_MaliciousUPXRejected(t *testing.T) { // assert on an already-finished test if it ever fired. var build *extendedBuildInfo var err error - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { build, err = scanReader(ctx, file.NewLocation("/malicious"), bytes.NewReader(data), false) }) assert.Nil(t, build) @@ -873,7 +874,7 @@ func TestCopyUnfiltered_WindowIsNotResident(t *testing.T) { const size = 16 * intFile.MB block := bytes.Repeat([]byte{0xE8, cto8, 0x11, 0x22, 0x33, 0x0F}, size/6+1)[:size] - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { require.NoError(t, copyUnfiltered(io.Discard, bytes.NewReader(block), size, cto8)) }) diff --git a/syft/pkg/cataloger/internal/dotnet/bundle/bundle_elf_test.go b/syft/pkg/cataloger/internal/dotnet/bundle/bundle_elf_test.go index 8521b8425..168edbf9a 100644 --- a/syft/pkg/cataloger/internal/dotnet/bundle/bundle_elf_test.go +++ b/syft/pkg/cataloger/internal/dotnet/bundle/bundle_elf_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" intFile "github.com/anchore/syft/internal/file" + "github.com/anchore/syft/internal/testutils" ) // readSeekCloser adapts a *bytes.Reader to the unionreader.UnionReader interface (adds Close). @@ -122,7 +123,7 @@ func TestExtractDepsJSONFromELFBundle_MalformedFileszDoesNotOverAllocate(t *test var content string var err error - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { content, err = ExtractDepsJSONFromELFBundle(r) }) require.NoError(t, err) diff --git a/syft/pkg/cataloger/internal/dotnet/bundle/bundle_test.go b/syft/pkg/cataloger/internal/dotnet/bundle/bundle_test.go index 70d2afedc..a3146d218 100644 --- a/syft/pkg/cataloger/internal/dotnet/bundle/bundle_test.go +++ b/syft/pkg/cataloger/internal/dotnet/bundle/bundle_test.go @@ -5,7 +5,6 @@ import ( "encoding/binary" "io" "math" - "runtime" "testing" "github.com/stretchr/testify/assert" @@ -185,21 +184,6 @@ func (r *unbackedSizeReader) ReadAt(p []byte, off int64) (int, error) { func (r *unbackedSizeReader) Close() error { return nil } -// measureAlloc reports the bytes allocated while fn runs. Only a byte count states "a small file cannot -// make us reserve a large buffer"; asserting an error comes back would keep passing if the allocation -// were hoisted above the check. -func measureAlloc(t *testing.T, fn func()) uint64 { - t.Helper() - - var before, after runtime.MemStats - runtime.GC() - runtime.ReadMemStats(&before) - fn() - runtime.ReadMemStats(&after) - - return after.TotalAlloc - before.TotalAlloc -} - func TestFindBundleHeaderOffset_ShortReadStillSearchesWhatWasRead(t *testing.T) { data := fileWithSignatureAt(8192, 64, 0x1234) diff --git a/syft/pkg/cataloger/internal/dotnet/pe/bundle_test.go b/syft/pkg/cataloger/internal/dotnet/pe/bundle_test.go index 4a9f4e536..c57f94b83 100644 --- a/syft/pkg/cataloger/internal/dotnet/pe/bundle_test.go +++ b/syft/pkg/cataloger/internal/dotnet/pe/bundle_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" intFile "github.com/anchore/syft/internal/file" + "github.com/anchore/syft/internal/testutils" ) func Test_extractDepsJSONFromBundle_Versions(t *testing.T) { @@ -109,7 +110,7 @@ func TestExtractDepsJSONFromBundle_MalformedSectionSizesDoNotOverAllocate(t *tes var content string var err error - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { content, err = extractDepsJSONFromBundle(r, sections) }) require.NoError(t, err) diff --git a/syft/pkg/cataloger/internal/dotnet/pe/pe_resource_test.go b/syft/pkg/cataloger/internal/dotnet/pe/pe_resource_test.go index bb074e3e2..305f43a92 100644 --- a/syft/pkg/cataloger/internal/dotnet/pe/pe_resource_test.go +++ b/syft/pkg/cataloger/internal/dotnet/pe/pe_resource_test.go @@ -4,7 +4,6 @@ import ( "bytes" "encoding/binary" "fmt" - "runtime" "testing" "time" @@ -12,6 +11,7 @@ import ( "github.com/stretchr/testify/require" intFile "github.com/anchore/syft/internal/file" + "github.com/anchore/syft/internal/testutils" ) const ( @@ -42,21 +42,6 @@ func boundWalk(data []byte) *resourceWalk { return newResourceWalk(bytes.NewReader(data), testSectionRVA) } -// measureAlloc reports the bytes allocated while fn runs. The property these guards exist for is "a small -// section cannot make us reserve a large buffer", and only a byte count states that: asserting an error -// comes back would keep passing if the allocation were hoisted above the check. -func measureAlloc(t *testing.T, fn func()) uint64 { - t.Helper() - - var before, after runtime.MemStats - runtime.GC() - runtime.ReadMemStats(&before) - fn() - runtime.ReadMemStats(&after) - - return after.TotalAlloc - before.TotalAlloc -} - func TestParseResourceDataEntry_SizePastSectionIsRejected(t *testing.T) { // OffsetToData and Size are user-controlled uint32s. A data entry may only describe bytes its section // actually holds, otherwise Size drives the allocation. @@ -150,7 +135,7 @@ func TestParseResourceDirectory_AliasedEntriesCannotAmplifyWork(t *testing.T) { var err error var timedOut bool - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { done := make(chan error, 1) go func() { done <- parseResourceDirectoryAt(testSectionRVA, w) }() select { @@ -360,7 +345,7 @@ func TestParseResourceDirectory_NestedOffsetsCannotUnderflow(t *testing.T) { w := boundWalk(buf) var err error - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { err = parseResourceDirectoryAt(testSectionRVA, w) }) @@ -441,7 +426,7 @@ func TestProcessResourceEntry_HostileNameLengthIsRejected(t *testing.T) { w := boundWalk(buf) - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { _ = processResourceEntry(entry, w) }) diff --git a/syft/pkg/cataloger/internal/dotnet/pe/pe_rva_test.go b/syft/pkg/cataloger/internal/dotnet/pe/pe_rva_test.go index fbccb6eb3..f3b1044ae 100644 --- a/syft/pkg/cataloger/internal/dotnet/pe/pe_rva_test.go +++ b/syft/pkg/cataloger/internal/dotnet/pe/pe_rva_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" intFile "github.com/anchore/syft/internal/file" + "github.com/anchore/syft/internal/testutils" ) func TestReadDataFromRVA_BogusSizeDoesNotOverAllocate(t *testing.T) { @@ -21,7 +22,7 @@ func TestReadDataFromRVA_BogusSizeDoesNotOverAllocate(t *testing.T) { r := &readSizeRecorder{Reader: bytes.NewReader(make([]byte, fileSize))} var err error - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { _, err = readDataFromRVA(r, 0x1000, 0xFFFFFFFF, sections) }) @@ -73,7 +74,7 @@ func TestReadDataFromRVA_SizePastTheAbsoluteCapIsRejected(t *testing.T) { r := &readSizeRecorder{Reader: bytes.NewReader(make([]byte, size+1))} var err error - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { _, err = readDataFromRVA(r, 0x1000, size, sections) }) diff --git a/syft/pkg/cataloger/internal/pkgtest/metadata_tracker.go b/syft/pkg/cataloger/internal/pkgtest/metadata_tracker.go index bed764b35..b4447d429 100644 --- a/syft/pkg/cataloger/internal/pkgtest/metadata_tracker.go +++ b/syft/pkg/cataloger/internal/pkgtest/metadata_tracker.go @@ -334,7 +334,7 @@ func getMetadataTypeName(metadata any) string { } // handle pointers - if t.Kind() == reflect.Ptr { + if t.Kind() == reflect.Pointer { t = t.Elem() } @@ -407,7 +407,7 @@ func dereferenceToStruct(v any) reflect.Value { } val := reflect.ValueOf(v) - if val.Kind() == reflect.Ptr { + if val.Kind() == reflect.Pointer { if val.IsNil() { return reflect.Value{} } @@ -436,7 +436,7 @@ func hasPopulatedDigest(fileRecord reflect.Value) bool { // check if digest is a pointer and not nil, or a non-zero value switch digestField.Kind() { - case reflect.Ptr: + case reflect.Pointer: return !digestField.IsNil() case reflect.String: return digestField.String() != "" diff --git a/syft/pkg/cataloger/java/graalvm_native_image_cataloger_test.go b/syft/pkg/cataloger/java/graalvm_native_image_cataloger_test.go index 67f0fb212..00fd15bd8 100644 --- a/syft/pkg/cataloger/java/graalvm_native_image_cataloger_test.go +++ b/syft/pkg/cataloger/java/graalvm_native_image_cataloger_test.go @@ -10,13 +10,13 @@ import ( "io" "os" "path" - "runtime" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" intFile "github.com/anchore/syft/internal/file" + "github.com/anchore/syft/internal/testutils" "github.com/anchore/syft/syft/artifact" "github.com/anchore/syft/syft/cpe" "github.com/anchore/syft/syft/internal/unionreader" @@ -299,20 +299,6 @@ func buildMinimalPE64(exportRVA, exportSize uint32, totalSize int) []byte { return buf } -// measureAlloc reports the bytes allocated while fn runs. TotalAlloc is process-wide, so a test using -// this must not call t.Parallel. -func measureAlloc(t *testing.T, fn func()) uint64 { - t.Helper() - - var before, after runtime.MemStats - runtime.GC() - runtime.ReadMemStats(&before) - fn() - runtime.ReadMemStats(&after) - - return after.TotalAlloc - before.TotalAlloc -} - // readSizeRecorder records the largest single read requested of it. A small allocation is not enough on // its own; no single read may exceed the file size either, whatever the headers claim. type readSizeRecorder struct { @@ -342,7 +328,7 @@ func TestNewPE_ExportDirectorySizeIsNotAllocatedUpFront(t *testing.T) { r := &readSizeRecorder{Reader: bytes.NewReader(data)} var err error - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { _, err = newPE("oversized-export-dir.exe", r) }) @@ -501,7 +487,7 @@ func TestDecompressSbom_RejectsStreamExpandingPastTheLimit(t *testing.T) { binary.LittleEndian.PutUint64(dataBuf[lengthStart:], lengthStart) var decompressErr error - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { _, _, decompressErr = decompressSbom(dataBuf, 0, lengthStart) }) err = decompressErr diff --git a/syft/pkg/cataloger/java/graalvm_native_image_pe_test.go b/syft/pkg/cataloger/java/graalvm_native_image_pe_test.go index dbe4ca989..20375934f 100644 --- a/syft/pkg/cataloger/java/graalvm_native_image_pe_test.go +++ b/syft/pkg/cataloger/java/graalvm_native_image_pe_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/require" intFile "github.com/anchore/syft/internal/file" + "github.com/anchore/syft/internal/testutils" "github.com/anchore/syft/syft/artifact" "github.com/anchore/syft/syft/pkg" ) @@ -344,7 +345,7 @@ func TestReadExportDirectory_DoesNotReadThePhantomBytes(t *testing.T) { rec := &readSizeRecorder{Reader: bytes.NewReader(make([]byte, realSize))} dir := pe.DataDirectory{VirtualAddress: 0, Size: nativeImageMaxExportDirectorySize} - allocated := measureAlloc(t, func() { + allocated := testutils.MeasureAlloc(t, func() { _, err := readExportDirectory(rec, dir) // a Size exactly at the cap must fall through to the remaining-bytes check, not be capped away; // pins the `>` (not `>=`) in the cap comparison diff --git a/syft/pkg/cataloger/java/internal/maven/resolver.go b/syft/pkg/cataloger/java/internal/maven/resolver.go index 56a92db20..b7d3d8ced 100644 --- a/syft/pkg/cataloger/java/internal/maven/resolver.go +++ b/syft/pkg/cataloger/java/internal/maven/resolver.go @@ -205,7 +205,7 @@ func (r *Resolver) resolveProjectProperty(ctx context.Context, resolutionContext pomValue = pomValue.Field(fieldNum) pomValueType = pomValue.Type() - if pomValueType.Kind() == reflect.Ptr { + if pomValueType.Kind() == reflect.Pointer { // we were recursing down the nested structs, but one of the steps // we need to take is a nil pointer, so give up if pomValue.IsNil() { diff --git a/syft/pkg/cataloger/php/interpreter_cataloger.go b/syft/pkg/cataloger/php/interpreter_cataloger.go index 4c1619eb3..80ae69d21 100644 --- a/syft/pkg/cataloger/php/interpreter_cataloger.go +++ b/syft/pkg/cataloger/php/interpreter_cataloger.go @@ -16,6 +16,9 @@ import ( "github.com/anchore/syft/syft/pkg/cataloger/internal/binutils" ) +// phpCPEName is the vendor and product used for every PHP interpreter CPE below. +const phpCPEName = "php" + type interpreterCataloger struct { name string extensionsGlob string @@ -49,8 +52,8 @@ func NewInterpreterCataloger() pkg.Cataloger { //nolint:funlen { Attributes: cpe.Attributes{ Part: "a", - Vendor: "php", - Product: "php", + Vendor: phpCPEName, + Product: phpCPEName, }, Source: cpe.NVDDictionaryLookupSource, }, @@ -71,8 +74,8 @@ func NewInterpreterCataloger() pkg.Cataloger { //nolint:funlen { Attributes: cpe.Attributes{ Part: "a", - Vendor: "php", - Product: "php", + Vendor: phpCPEName, + Product: phpCPEName, }, Source: cpe.NVDDictionaryLookupSource, }, @@ -86,15 +89,15 @@ func NewInterpreterCataloger() pkg.Cataloger { //nolint:funlen Package: "libphp", PURL: packageurl.PackageURL{ Type: packageurl.TypeGeneric, - Name: "php", + Name: phpCPEName, // the version will be filled in dynamically }, CPEs: []cpe.CPE{ { Attributes: cpe.Attributes{ Part: "a", - Vendor: "php", - Product: "php", + Vendor: phpCPEName, + Product: phpCPEName, }, Source: cpe.NVDDictionaryLookupSource, }, diff --git a/syft/pkg/cataloger/snap/parse_kernel_changelog_test.go b/syft/pkg/cataloger/snap/parse_kernel_changelog_test.go index df0c15767..063161d19 100644 --- a/syft/pkg/cataloger/snap/parse_kernel_changelog_test.go +++ b/syft/pkg/cataloger/snap/parse_kernel_changelog_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/anchore/syft/internal/testutils" "github.com/anchore/syft/syft/file" "github.com/anchore/syft/syft/pkg" "github.com/anchore/syft/syft/pkg/cataloger/generic" @@ -310,3 +311,47 @@ func TestParseKernelChangelog(t *testing.T) { }) } } + +// TestParseKernelChangelog_gzipBombStaysBounded pins a bound nothing in this file states out loud. +// parseKernelChangelog reads the decompressed changelog through a bufio.Scanner, and Scanner refuses a +// token past MaxScanTokenSize, so a gzip member expanding to gigabytes costs 64KB. That is the only +// 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 + + // one enormous line with no newline, the worst case for a line scanner + var gz bytes.Buffer + gw := gzip.NewWriter(&gz) + chunk := bytes.Repeat([]byte{'a'}, 32*1024) + for remaining := expanded; remaining > 0; { + n, err := gw.Write(chunk[:min(remaining, len(chunk))]) + require.NoError(t, err) + remaining -= n + } + require.NoError(t, gw.Close()) + require.Less(t, gz.Len(), 1024*1024, "compressed payload should be tiny relative to what it expands to") + + // prove the fixture is a bomb: the same stream read without a line bound costs its full size + unbounded := testutils.MeasureAlloc(t, func() { + zr, err := gzip.NewReader(bytes.NewReader(gz.Bytes())) + require.NoError(t, err) + defer zr.Close() + b, err := io.ReadAll(zr) + require.NoError(t, err) + require.Len(t, b, expanded) + }) + require.Greater(t, unbounded, uint64(expanded), + "fixture did not actually cost more than its own size; it is no longer a bomb") + + bounded := testutils.MeasureAlloc(t, func() { + _, _, err := parseKernelChangelog(context.Background(), nil, nil, + file.NewLocationReadCloser(file.NewLocation("/changelog.gz"), io.NopCloser(bytes.NewReader(gz.Bytes())))) + // the scanner refuses the oversized line, so this fails rather than parsing a version out of it + require.Error(t, err) + }) + + t.Logf("unbounded allocated %d bytes, bounded allocated %d bytes", unbounded, bounded) + assert.Less(t, bounded, uint64(8*1024*1024), + "the changelog scan must stay bounded by the line limit, not by what the member expands to") +} diff --git a/test/rules/rules.go b/test/rules/rules.go index 27ea103e5..e001a12bd 100644 --- a/test/rules/rules.go +++ b/test/rules/rules.go @@ -38,6 +38,41 @@ func noUnboundedReads(m dsl.Matcher) { Report("do not use unbounded io.Copy to in-memory buffer; wrap the source reader with io.LimitReader") } +// nolint:unused +func noOverflowingBoundsCheck(m dsl.Matcher) { + // an offset+size bounds check on unsigned operands wraps rather than saturating, so a large + // offset out of a file header can produce a small sum and pass a check it should have failed. + // Written as a subtraction against the limit, there is nothing to wrap. + m.Match( + `$offset + $size > $limit`, + `$offset + $size >= $limit`, + `$offset + $size < $limit`, + `$offset + $size <= $limit`, + ). + Where((m["offset"].Type.OfKind("unsigned") || m["size"].Type.OfKind("unsigned")) && + // a constant addend cannot be attacker-controlled, which is the common safe shape of + // stepping a loop cursor (`pos+4 <= uint32(len(buf))`) + !m["offset"].Const && !m["size"].Const && + // same 32-bit floor as noUnboundedAllocations: a uint16 sum is a nibble or a count widened for + // the comparison, not an offset that can carry a file-controlled value up to the wrap point + m["offset"].Type.Size >= 4). + Report("bounds check can overflow: $offset + $size wraps on unsigned operands. write it as a subtraction against the limit instead, e.g. `$offset > $limit || $size > $limit-$offset`") +} + +// nolint:unused +func noSwallowedEOF(m dsl.Matcher) { + // treating EOF as success hands the caller a zero value it cannot distinguish from real data. In a + // loop that advances by however much was read, that is not just bad data but a cursor that never + // moves, so a truncated record spins forever. + m.Match( + `if errors.Is($err, io.EOF) { return nil }`, + `if errors.Is($err, io.EOF) { return nil, nil }`, + `if $err == io.EOF { return nil }`, + `if $err == io.EOF { return nil, nil }`, + ). + Report("EOF returned as success: the caller gets a zero value it cannot tell from real data. return the error, or return an explicit sentinel the caller checks") +} + // nolint:unused func noDirectTempFiles(m dsl.Matcher) { // catalogers must use tmpdir.FromContext(ctx) instead of creating temp files/dirs directly,