From c9b4f9c085feaae323f7c9b368653b7674c7d7cb Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Thu, 24 Sep 2026 11:28:26 -0400 Subject: [PATCH] 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 --- internal/archive/extract.go | 12 ++++++--- internal/archive/extract_test.go | 28 +++++++++++++++++++++ internal/task/archive_tasks.go | 10 +++++++- internal/task/archive_tasks_test.go | 39 +++++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 4 deletions(-) diff --git a/internal/archive/extract.go b/internal/archive/extract.go index 90031b0a9..60aec3cf0 100644 --- a/internal/archive/extract.go +++ b/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 } diff --git a/internal/archive/extract_test.go b/internal/archive/extract_test.go index 75227bdd6..d587a52a5 100644 --- a/internal/archive/extract_test.go +++ b/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}) diff --git a/internal/task/archive_tasks.go b/internal/task/archive_tasks.go index 34e548b97..cb1ce21db 100644 --- a/internal/task/archive_tasks.go +++ b/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 { diff --git a/internal/task/archive_tasks_test.go b/internal/task/archive_tasks_test.go index 1e9a6b71f..c57d1df27 100644 --- a/internal/task/archive_tasks_test.go +++ b/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)