From 36d417d52a8adde5e4775a0a63829e1cdc5d7bed Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Wed, 21 Oct 2026 11:45:34 -0400 Subject: [PATCH] fix: charge the decompression budget at the decoder, not per entry Draining skipped entries still left a hole: a tar entry whose header doesn't read as a regular file (a socket or directory per its mode bits, say) can carry a body, and the tar reader decompresses it to reach the next header without anything being charged or ctx being checked. - compressed tars are now read through a reader that charges the budget for every byte the decoder produces and checks ctx per read, so no skip path inside the tar reader gets around it - entry content from that stream isn't charged a second time, and the per-entry drain is gone - zip and plain tar are unchanged Signed-off-by: Alex Goodman --- internal/archive/extract.go | 41 +++++++++++++++----------------- internal/archive/extract_test.go | 28 ++++++++++++++++++++++ internal/archive/resolver.go | 1 + internal/archive/storage.go | 35 +++++++++++++-------------- 4 files changed, 65 insertions(+), 40 deletions(-) diff --git a/internal/archive/extract.go b/internal/archive/extract.go index feee70cc4..60cd93fd3 100644 --- a/internal/archive/extract.go +++ b/internal/archive/extract.go @@ -11,6 +11,7 @@ import ( "github.com/mholt/archives" + "github.com/anchore/syft/internal" intFile "github.com/anchore/syft/internal/file" "github.com/anchore/syft/internal/log" "github.com/anchore/syft/syft/file" @@ -139,21 +140,26 @@ func (r *Resolver) extract(ctx context.Context, format archives.Extractor, conte if _, err := content.Seek(0, io.SeekStart); err != nil { return err } - // a compressed tar decompresses whatever an entry leaves unread to reach the next header, so there the - // bytes of skipped entries cost as much as kept ones. A zip or a plain tar seeks past them for free - _, drains := format.(archives.CompressedArchive) - err := format.Extract(ctx, content, func(_ context.Context, f archives.FileInfo) error { - hdr, ok := r.entryHeader(f) - skip := !ok || exclusions.Excludes(hdr.Name) - if !f.Mode().IsRegular() { - if skip { - return nil - } - return r.add(hdr, nil) + var source io.Reader = content + if ca, ok := format.(archives.CompressedArchive); ok && ca.Compression != nil { + // a compressed tar decompresses everything to reach each header, including entries skipped whole + // and whatever an entry leaves unread, so the budget is charged for every byte the decoder + // produces rather than for what is kept. A zip or a plain tar seeks past what it skips + rc, err := ca.OpenReader(content) + if err != nil { + return err } - if skip && !drains { + defer internal.CloseAndLogError(rc, "decompressor") + source, format, r.metered = &meteredReader{ctx: ctx, reader: rc, budget: r.budget}, ca.Extraction, true + } + err := format.Extract(ctx, source, func(_ context.Context, f archives.FileInfo) error { + hdr, ok := r.entryHeader(f) + if !ok || exclusions.Excludes(hdr.Name) { return nil } + if !f.Mode().IsRegular() { + return r.add(hdr, nil) + } entry, err := f.Open() if err != nil { log.WithFields("entry", hdr.Name, "error", err).Trace("unable to open archive entry, skipping it") @@ -164,16 +170,7 @@ func (r *Resolver) extract(ctx context.Context, format archives.Extractor, conte log.WithFields("entry", hdr.Name, "error", err).Trace("unable to close archive entry") } }() - if !skip { - if err := r.add(hdr, entry); err != nil { - return err - } - } - if !drains { - return nil - } - // what add left unread (the rest of an entry over the size cap, or one skipped whole) is charged here - return r.drain(ctx, entry) + return r.add(hdr, entry) }) r.finish() if errors.Is(err, ErrDiskLimitReached) { diff --git a/internal/archive/extract_test.go b/internal/archive/extract_test.go index 5726ebc3b..17b1e3fbd 100644 --- a/internal/archive/extract_test.go +++ b/internal/archive/extract_test.go @@ -314,6 +314,34 @@ func TestResolver_extract_skippedCompressedTarBytesSpendTheBudget(t *testing.T) } } +// a body behind a header that doesn't read as a regular file (here a socket per its mode bits) is never +// opened, but the tar reader still decompresses it to reach the next header +func TestResolver_extract_bodiesOfNonRegularTarEntriesSpendTheBudget(t *testing.T) { + var buf bytes.Buffer + gw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gw) + body := strings.Repeat("\x00", 4<<20) + require.NoError(t, tw.WriteHeader(&tar.Header{Name: "sock", Typeflag: tar.TypeReg, Mode: 0o140644, Size: int64(len(body))})) + _, err := tw.Write([]byte(body)) + require.NoError(t, err) + require.NoError(t, tw.WriteHeader(&tar.Header{Name: "after.txt", Typeflag: tar.TypeReg, Mode: 0o644, Size: 5})) + _, err = tw.Write([]byte("after")) + require.NoError(t, err) + require.NoError(t, tw.Close()) + require.NoError(t, gw.Close()) + + r, _ := resolverIn(t, memCharge(1<<30)) + r.budget = NewBudget(1 << 20) + content := bytes.NewReader(buf.Bytes()) + format, err := identifyFormat(context.Background(), "test.tar.gz", content) + require.NoError(t, err) + + require.NoError(t, r.extract(context.Background(), format, content, nil)) + assert.ErrorIs(t, r.Truncated, errBudgetSpent) + assert.Empty(t, storedNames(t, r)) + assert.Zero(t, r.budget.remaining) +} + // a zip or a plain tar seeks past what it skips, so an excluded entry there costs nothing func TestResolver_extract_skippedSeekableBytesAreFree(t *testing.T) { entries := []testEntry{{name: "bomb.bin", body: strings.Repeat("\x00", 4<<20)}, {name: "after.txt", body: "after"}} diff --git a/internal/archive/resolver.go b/internal/archive/resolver.go index fbd499937..0a6b1a699 100644 --- a/internal/archive/resolver.go +++ b/internal/archive/resolver.go @@ -43,6 +43,7 @@ type Resolver struct { archiveCoordinates file.Coordinates charge *charge budget *Budget + metered bool // the budget is charged as the archive is decoded (see meteredReader), not in put // content is held in memory while the memory limit admits it and in one spill file once it does // not. The file is created on first use, so an archive that stays in memory never touches the diff --git a/internal/archive/storage.go b/internal/archive/storage.go index 842ad8d01..c654d0f3c 100644 --- a/internal/archive/storage.go +++ b/internal/archive/storage.go @@ -44,7 +44,8 @@ func (r *Resolver) put(b *blob, content io.Reader) error { for { n, readErr := content.Read(r.chunk) if n > 0 { - if !r.budget.take(int64(n)) { + // content read through a meteredReader was charged as it was decoded + if !r.metered && !r.budget.take(int64(n)) { return errBudgetSpent } if err := r.write(b, r.chunk[:n]); err != nil { @@ -62,25 +63,23 @@ func (r *Resolver) put(b *blob, content io.Reader) error { return nil } -// drain reads the rest of a streaming entry without keeping it, charging the decompression budget for -// every byte, so skipping an entry is not a way around the budget. A read error ends the drain quietly; -// the archive reader reports a broken stream at the next entry. -func (r *Resolver) drain(ctx context.Context, content io.Reader) error { - if r.chunk == nil { - r.chunk = make([]byte, copyChunkSize) +// meteredReader charges the decompression budget for every byte read through it and stops once ctx is +// done, so nothing a decoder produces gets around the budget. +type meteredReader struct { + ctx context.Context + reader io.Reader + budget *Budget +} + +func (m *meteredReader) Read(p []byte) (int, error) { + if err := m.ctx.Err(); err != nil { + return 0, err } - for { - if err := ctx.Err(); err != nil { - return err - } - n, err := content.Read(r.chunk) - if n > 0 && !r.budget.take(int64(n)) { - return errBudgetSpent - } - if err != nil { - return nil - } + n, err := m.reader.Read(p) + if n > 0 && !m.budget.take(int64(n)) { + return 0, errBudgetSpent } + return n, err } // write appends p to b: into memory while the memory limit admits it, otherwise into the spill file