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 <wagoodman@users.noreply.github.com>

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
+	var source io.Reader = content
-	// bytes of skipped entries cost as much as kept ones. A zip or a plain tar seeks past them for free
+	if ca, ok := format.(archives.CompressedArchive); ok && ca.Compression != nil {
-	_, drains := format.(archives.CompressedArchive)
+		// a compressed tar decompresses everything to reach each header, including entries skipped whole
-	err := format.Extract(ctx, content, func(_ context.Context, f archives.FileInfo) error {
+		// and whatever an entry leaves unread, so the budget is charged for every byte the decoder
-		hdr, ok := r.entryHeader(f)
+		// produces rather than for what is kept. A zip or a plain tar seeks past what it skips
-		skip := !ok || exclusions.Excludes(hdr.Name)
+		rc, err := ca.OpenReader(content)
-		if !f.Mode().IsRegular() {
+		if err != nil {
-			if skip {
+			return err
-				return nil
-			}
-			return r.add(hdr, nil)
 		}
-		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 {
+		return r.add(hdr, entry)
-			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)
 	})
 	r.finish()
 	if errors.Is(err, ErrDiskLimitReached) {

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"}}

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

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
+// meteredReader charges the decompression budget for every byte read through it and stops once ctx is
-// every byte, so skipping an entry is not a way around the budget. A read error ends the drain quietly;
+// done, so nothing a decoder produces gets around the budget.
-// the archive reader reports a broken stream at the next entry.
+type meteredReader struct {
-func (r *Resolver) drain(ctx context.Context, content io.Reader) error {
+	ctx    context.Context
-	if r.chunk == nil {
+	reader io.Reader
-		r.chunk = make([]byte, copyChunkSize)
+	budget *Budget
+}
+
+func (m *meteredReader) Read(p []byte) (int, error) {
+	if err := m.ctx.Err(); err != nil {
+		return 0, err
 	}
-	for {
+	n, err := m.reader.Read(p)
-		if err := ctx.Err(); err != nil {
+	if n > 0 && !m.budget.take(int64(n)) {
-			return err
+		return 0, errBudgetSpent
-		}
-		n, err := content.Read(r.chunk)
-		if n > 0 && !r.budget.take(int64(n)) {
-			return errBudgetSpent
-		}
-		if err != nil {
-			return nil
-		}
 	}
+	return n, err
 }
 
 // write appends p to b: into memory while the memory limit admits it, otherwise into the spill file