fix: recover from panics while extracting an archive

sub-pipeline catalogers were already run under `runTaskSafely`, but extraction wasn't: the zip, tar and compressor decoders run on untrusted bytes, and a panic in one unwound the whole walk and failed `CreateSBOM` with "failed to run tasks".

- `processArchive` recovers and returns the panic (with its stack) as an error at the archive's coordinates, so it becomes an unknown and the sibling archives are still cataloged
- `Extract` now defers `Cleanup` on failure instead of calling it on each error path, so a panic no longer leaks the limiter charge or the spill file

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

internal/archive/extract.go

@@ -28,9 +28,16 @@ func Extract(ctx context.Context, r io.Reader, fileSystemID, archivePath string,
 	charge := limiter.charge()
 	resolver := newResolver(ctx, fileSystemID, archivePath, charge)
 
+	// deferred so that a panic in a decoder still gives back the charge and removes any spill file
+	succeeded := false
+	defer func() {
+		if !succeeded {
+			resolver.Cleanup()
+		}
+	}()
+
 	content, release, err := resolver.acquire(r)
 	if err != nil {
-		resolver.Cleanup()
 		return nil, err
 	}
 	// the archive's own bytes are needed only until they are extracted and digested
@@ -38,15 +45,14 @@ func Extract(ctx context.Context, r io.Reader, fileSystemID, archivePath string,
 
 	format := identifyFormat(ctx, archiveFileName(archivePath), content)
 	if format == nil {
-		resolver.Cleanup()
 		return nil, nil
 	}
 
 	if err := resolver.extract(ctx, format, content, exclusions); err != nil {
-		resolver.Cleanup()
 		return nil, fmt.Errorf("unable to extract archive %q: %w", archivePath, err)
 	}
 	resolver.Digests = digestsOf(ctx, content, archivePath)
+	succeeded = true
 	return resolver, nil
 }
 

internal/archive/extract_test.go

@@ -339,6 +339,34 @@ func TestExtract_heldContentWrittenToDiskStaysUntilCleanup(t *testing.T) {
 	assert.Zero(t, disk)
 }
 
+func TestExtract_aPanicStillReleasesEverything(t *testing.T) {
+	// a decoder panicking on hostile bytes must not leak the charge or the spill file
+	ctx, root := scanContext(t)
+	limiter := NewLimiter(Limits{MaxMemoryBytes: 0, MaxDiskBytes: -1})
+
+	require.Panics(t, func() {
+		_, _ = Extract(ctx, &panicsAfter{n: 4096}, "", "app.zip", limiter, nil)
+	})
+
+	mem, disk := limiter.InUse()
+	assert.Zero(t, mem)
+	assert.Zero(t, disk)
+	assert.Empty(t, filesIn(t, root))
+}
+
+// panicsAfter hands out n zero bytes, then panics.
+type panicsAfter struct{ n int }
+
+func (p *panicsAfter) Read(b []byte) (int, error) {
+	if p.n <= 0 {
+		panic("decoder blew up")
+	}
+	n := min(len(b), p.n)
+	clear(b[:n])
+	p.n -= n
+	return n, nil
+}
+
 func TestExtract_archiveBytesThatCannotBePlacedAreSkipped(t *testing.T) {
 	data := zipBytes(t, sampleFiles)
 	limiter := NewLimiter(Limits{MaxMemoryBytes: 0, MaxDiskBytes: 0})

internal/task/archive_tasks.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io"
 	"maps"
+	"runtime/debug"
 	"time"
 
 	"github.com/dustin/go-humanize"
@@ -142,7 +143,14 @@ func (c *archiveCataloger) catalog(ctx context.Context, resolver file.Resolver,
 	return errs
 }
 
-func (c *archiveCataloger) processArchive(ctx context.Context, parentResolver file.Resolver, candidate archiveCandidate, depth int, builder sbomsync.Builder) error {
+func (c *archiveCataloger) processArchive(ctx context.Context, parentResolver file.Resolver, candidate archiveCandidate, depth int, builder sbomsync.Builder) (err error) {
+	// the decoders run on untrusted bytes; a panic in one costs this archive, not the scan
+	defer func() {
+		if r := recover(); r != nil {
+			err = fmt.Errorf("panic while cataloging archive: %v at:\n%s", r, debug.Stack())
+		}
+	}()
+
 	location := candidate.location
 	content, err := parentResolver.FileContentsByLocation(location)
 	if err != nil {

internal/task/archive_tasks_test.go

@@ -63,6 +63,45 @@ func (d streamingDirResolver) FileContentsByLocation(loc file.Location) (io.Read
 	}{f, f}, nil
 }
 
+// panickingDirResolver is dirTestResolver whose reads of one file panic, standing in for a decoder
+// that panics on hostile bytes.
+type panickingDirResolver struct {
+	dirTestResolver
+	path string
+}
+
+func (d panickingDirResolver) FileContentsByLocation(loc file.Location) (io.ReadCloser, error) {
+	if loc.RealPath == d.path {
+		return io.NopCloser(panicReader{}), nil
+	}
+	return d.dirTestResolver.FileContentsByLocation(loc)
+}
+
+type panicReader struct{}
+
+func (panicReader) Read([]byte) (int, error) { panic("decoder blew up") }
+
+func Test_archiveCataloger_aPanicCostsOnlyThatArchive(t *testing.T) {
+	rootDir := t.TempDir()
+	for _, name := range []string{"bad.zip", "good.zip"} {
+		require.NoError(t, os.WriteFile(filepath.Join(rootDir, name), makeZip(t, map[string][]byte{"a.txt": []byte("a")}), 0o600))
+	}
+
+	var seen []string
+	tsk := newTestTask(t, cataloging.DefaultArchiveSearchConfig().WithMaxDepth(1), capturingTask(t, &seen, nil))
+
+	resolver := panickingDirResolver{dirTestResolver: dirTestResolver{dir: rootDir}, path: "/bad.zip"}
+	err := tsk.Execute(context.Background(), resolver, sbomsync.NewBuilder(newTestSBOM()))
+
+	// a coordinate error becomes an unknown; a plain error (or an escaped panic) would fail the scan
+	coordErrs, remaining := unknown.ExtractCoordinateErrors(err)
+	require.NoError(t, remaining)
+	require.Len(t, coordErrs, 1)
+	assert.Equal(t, "/bad.zip", coordErrs[0].Coordinates.RealPath)
+	assert.ErrorContains(t, coordErrs[0].Reason, "panic while cataloging archive: decoder blew up")
+	assert.Equal(t, []string{"/good.zip"}, seen, "the sibling is still cataloged")
+}
+
 func newTestTask(t *testing.T, cfg cataloging.ArchiveSearchConfig, subPipeline ...Task) Task {
 	t.Helper()
 	tsk := NewArchiveCatalogerTask(cfg, subPipeline, nil)