fix: don't fail the whole SBOM when a cataloger panics on one file (#5342)

* fix(generic): recover from parser and processor panics

- a panic while parsing one file becomes an unknown for that location instead of failing the whole SBOM
- a panic in a package processor is logged as a warning and the unprocessed results are kept
- stacks go to the trace log, not into the unknown text

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

* fix(unknown): keep errors that unwrap to nil

`sync.PanicError` with a non-error panic value (e.g. `panic("boom")`) unwraps to nil, and was being dropped entirely from the error graph instead of being kept as a leaf.

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

* fix(task): don't put the stack trace in task panic errors

Task panics are still fatal, but the error now reads `panic in task "<name>": <value>` and the stack goes to the debug log.

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

* test(generic): cover chained processors when a later one panics

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

* fix(file): recover from panics in the executable and file digest catalogers

A panic while reading one file is now recorded as an unknown for that file instead of failing the whole SBOM, the same as generic catalogers.

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

---------

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

internal/task/executor.go

@@ -51,7 +51,8 @@ func runTaskSafely(ctx context.Context, t Task, resolver file.Resolver, s sbomsy
 	// handle individual cataloger panics
 	defer func() {
 		if e := recover(); e != nil {
-			err = fmt.Errorf("%v at:\n%s", e, string(debug.Stack()))
+			log.WithFields("task", t.Name()).Debugf("task panic stack:\n%s", debug.Stack())
+			err = fmt.Errorf("panic in task %q: %v", t.Name(), e)
 		}
 	}()
 

internal/task/executor_test.go

@@ -8,8 +8,12 @@ import (
 	"github.com/wagoodman/go-progress"
 
 	"github.com/anchore/syft/internal/sbomsync"
+	"github.com/anchore/syft/syft/artifact"
 	"github.com/anchore/syft/syft/event/monitor"
 	"github.com/anchore/syft/syft/file"
+	"github.com/anchore/syft/syft/pkg"
+	"github.com/anchore/syft/syft/pkg/cataloger/generic"
+	"github.com/anchore/syft/syft/sbom"
 )
 
 func Test_TaskExecutor_PanicHandling(t *testing.T) {
@@ -21,6 +25,20 @@ func Test_TaskExecutor_PanicHandling(t *testing.T) {
 		Manual: progress.NewManual(-1),
 	})
 
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "executor_test.go")
+	require.EqualError(t, err, `panic in task "panicking-cataloger": something bad happened`)
+}
+
+func Test_RunTask_GenericParserPanicBecomesUnknown(t *testing.T) {
+	c := generic.NewCataloger("panicking-cataloger").
+		WithParserByPath(func(context.Context, file.Resolver, *generic.Environment, file.LocationReadCloser) ([]pkg.Package, []artifact.Relationship, error) {
+			panic("boom")
+		}, "executor_test.go")
+	s := &sbom.SBOM{}
+
+	err := RunTask(context.Background(), NewPackageTask(DefaultCatalogingFactoryConfig(), c), file.NewMockResolverForPaths("executor_test.go"), sbomsync.NewBuilder(s), &monitor.TaskProgress{
+		Manual: progress.NewManual(-1),
+	})
+
+	require.NoError(t, err)
+	require.Len(t, s.Artifacts.Unknowns, 1)
 }

internal/unknown/coordinate_error.go

@@ -183,6 +183,10 @@ func visitErrors(err error, fn func(error) error) error {
 	// unwrap singly wrapped errors
 	if e, ok := err.(interface{ Unwrap() error }); ok {
 		wrapped := e.Unwrap()
+		if wrapped == nil {
+			// nothing is wrapped (e.g. a sync.PanicError with a non-error value), so this is a leaf
+			return fn(err)
+		}
 		got := visitErrors(wrapped, fn)
 		if got == nil {
 			return nil

internal/unknown/coordinate_error_test.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 
+	"github.com/anchore/go-sync"
 	"github.com/anchore/syft/syft/file"
 )
 
@@ -234,3 +235,11 @@ func Test_Append(t *testing.T) {
 	e3 = Appendf(e2, file.NewLocation("l3"), "%s", "e3")
 	require.Equal(t, "l1: e1\nl3: e3", e3.Error())
 }
+
+func Test_ExtractCoordinateErrors_keepsNilUnwrap(t *testing.T) {
+	// sync.PanicError unwraps to nil when the panic value is not an error
+	err := sync.PanicError{Value: "x"}
+	unknowns, remaining := ExtractCoordinateErrors(err)
+	require.Empty(t, unknowns)
+	require.Equal(t, err, remaining)
+}

syft/file/cataloger/executable/cataloger.go

@@ -7,6 +7,7 @@ import (
 	"debug/macho"
 	"encoding/binary"
 	"fmt"
+	"runtime/debug"
 	"sort"
 
 	"github.com/bmatcuk/doublestar/v4"
@@ -89,7 +90,7 @@ func (i *Cataloger) CatalogCtx(ctx context.Context, resolver file.Resolver) (map
 	return results, errs
 }
 
-func processExecutableLocation(loc file.Location, resolver file.Resolver) (*file.Executable, error) {
+func processExecutableLocation(loc file.Location, resolver file.Resolver) (exec *file.Executable, err error) {
 	reader, err := resolver.FileContentsByLocation(loc)
 	if err != nil {
 		log.WithFields("error", err, "path", loc.RealPath).Debug("unable to get file contents")
@@ -97,6 +98,15 @@ func processExecutableLocation(loc file.Location, resolver file.Resolver) (*file
 	}
 	defer internal.CloseAndLogError(reader, loc.RealPath)
 
+	defer func() {
+		if r := recover(); r != nil {
+			// a malformed binary must not take down the whole SBOM; the caller records this as an unknown
+			log.WithFields("path", loc.RealPath, "panic", r).Debug("recovered from panic while reading executable")
+			log.Tracef("executable panic stack:\n%s", debug.Stack())
+			exec, err = nil, fmt.Errorf("recovered from panic while reading executable: %v", r)
+		}
+	}()
+
 	uReader, err := unionreader.GetUnionReader(reader)
 	if err != nil {
 		log.WithFields("error", err, "path", loc.RealPath).Debug("unable to get union reader")

syft/file/cataloger/executable/cataloger_test.go

@@ -0,0 +1,40 @@
+package executable
+
+import (
+	"context"
+	"io"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/anchore/syft/internal/unknown"
+	"github.com/anchore/syft/syft/file"
+)
+
+type panicReader struct{}
+
+func (panicReader) Read([]byte) (int, error) { panic("boom") }
+
+type panickingContentsResolver struct {
+	*file.MockResolver
+}
+
+func (r panickingContentsResolver) FilesByMIMEType(...string) ([]file.Location, error) {
+	return r.FilesByPath("cataloger_test.go")
+}
+
+func (r panickingContentsResolver) FileContentsByLocation(file.Location) (io.ReadCloser, error) {
+	return io.NopCloser(panicReader{}), nil
+}
+
+func TestCatalogCtx_panicBecomesUnknown(t *testing.T) {
+	resolver := panickingContentsResolver{file.NewMockResolverForPaths("cataloger_test.go")}
+
+	results, err := NewCataloger(DefaultConfig()).CatalogCtx(context.Background(), resolver)
+
+	require.Empty(t, results)
+	unknowns, remaining := unknown.ExtractCoordinateErrors(err)
+	require.NoError(t, remaining)
+	require.Len(t, unknowns, 1)
+	require.ErrorContains(t, unknowns[0].Reason, "recovered from panic while reading executable: boom")
+}

syft/file/cataloger/filedigest/cataloger.go

@@ -5,6 +5,7 @@ import (
 	"crypto"
 	"errors"
 	"fmt"
+	"runtime/debug"
 
 	"github.com/dustin/go-humanize"
 
@@ -87,7 +88,7 @@ func (i *Cataloger) Catalog(ctx context.Context, resolver file.Resolver, coordin
 	return results, err
 }
 
-func (i *Cataloger) catalogLocation(ctx context.Context, resolver file.Resolver, location file.Location) ([]file.Digest, error) {
+func (i *Cataloger) catalogLocation(ctx context.Context, resolver file.Resolver, location file.Location) (digests []file.Digest, err error) {
 	meta, err := resolver.FileMetadataByLocation(location)
 	if err != nil {
 		return nil, err
@@ -104,7 +105,16 @@ func (i *Cataloger) catalogLocation(ctx context.Context, resolver file.Resolver,
 	}
 	defer internal.CloseAndLogError(contentReader, location.AccessPath)
 
-	digests, err := intFile.NewDigestsFromFile(ctx, contentReader, i.hashes)
+	defer func() {
+		if r := recover(); r != nil {
+			// the caller records this as an unknown for the location instead of failing the whole SBOM
+			log.WithFields("path", location.RealPath, "panic", r).Debug("recovered from panic while digesting file")
+			log.Tracef("file digest panic stack:\n%s", debug.Stack())
+			digests, err = nil, fmt.Errorf("recovered from panic while digesting file: %v", r)
+		}
+	}()
+
+	digests, err = intFile.NewDigestsFromFile(ctx, contentReader, i.hashes)
 	if err != nil {
 		return nil, internal.ErrPath{Context: "digests-cataloger", Path: location.RealPath, Err: err}
 	}

syft/file/cataloger/filedigest/cataloger_test.go

@@ -15,6 +15,7 @@ import (
 	stereoscopeFile "github.com/anchore/stereoscope/pkg/file"
 	"github.com/anchore/stereoscope/pkg/imagetest"
 	intFile "github.com/anchore/syft/internal/file"
+	"github.com/anchore/syft/internal/unknown"
 	"github.com/anchore/syft/syft/file"
 	"github.com/anchore/syft/syft/source"
 	"github.com/anchore/syft/syft/source/directorysource"
@@ -202,3 +203,27 @@ func TestFileDigestCataloger_GivenCoordinates(t *testing.T) {
 	}
 
 }
+
+type panicReader struct{}
+
+func (panicReader) Read([]byte) (int, error) { panic("boom") }
+
+type panickingContentsResolver struct {
+	*file.MockResolver
+}
+
+func (r panickingContentsResolver) FileContentsByLocation(file.Location) (io.ReadCloser, error) {
+	return io.NopCloser(panicReader{}), nil
+}
+
+func TestDigestsCataloger_panicBecomesUnknown(t *testing.T) {
+	resolver := panickingContentsResolver{file.NewMockResolverForPaths("cataloger_test.go")}
+
+	results, err := NewCataloger([]crypto.Hash{crypto.SHA256}).Catalog(context.Background(), resolver, file.NewLocation("cataloger_test.go").Coordinates)
+
+	require.Empty(t, results)
+	unknowns, remaining := unknown.ExtractCoordinateErrors(err)
+	require.NoError(t, remaining)
+	require.Len(t, unknowns, 1)
+	require.ErrorContains(t, unknowns[0].Reason, "recovered from panic while digesting file: boom")
+}

syft/pkg/cataloger/generic/cataloger.go

@@ -2,6 +2,8 @@ package generic
 
 import (
 	"context"
+	"fmt"
+	"runtime/debug"
 
 	"github.com/anchore/go-logger"
 	"github.com/anchore/go-sync"
@@ -221,12 +223,24 @@ func (c *Cataloger) Catalog(ctx context.Context, resolver file.Resolver) ([]pkg.
 
 func (c *Cataloger) process(ctx context.Context, resolver file.Resolver, pkgs []pkg.Package, rels []artifact.Relationship, err error) ([]pkg.Package, []artifact.Relationship, error) {
 	for _, p := range c.processors {
-		pkgs, rels, err = p.process(ctx, resolver, pkgs, rels, err)
+		pkgs, rels, err = c.processSafely(ctx, p, resolver, pkgs, rels, err)
 	}
 	return pkgs, rels, err
 }
 
-func invokeParser(ctx context.Context, resolver file.Resolver, location file.Location, logger logger.Logger, parser Parser, env *Environment) ([]pkg.Package, []artifact.Relationship, error) {
+func (c *Cataloger) processSafely(ctx context.Context, p processExecutor, resolver file.Resolver, pkgs []pkg.Package, rels []artifact.Relationship, err error) (outPkgs []pkg.Package, outRels []artifact.Relationship, outErr error) {
+	defer func() {
+		if r := recover(); r != nil {
+			// keep the unprocessed results; there is no single location to attach an unknown to
+			log.WithFields("cataloger", c.upstreamCataloger, "panic", r).Warn("recovered from panic in package processor")
+			log.Tracef("processor panic stack:\n%s", debug.Stack())
+			outPkgs, outRels, outErr = pkgs, rels, err
+		}
+	}()
+	return p.process(ctx, resolver, pkgs, rels, err)
+}
+
+func invokeParser(ctx context.Context, resolver file.Resolver, location file.Location, logger logger.Logger, parser Parser, env *Environment) (pkgs []pkg.Package, rels []artifact.Relationship, err error) {
 	contentReader, err := resolver.FileContentsByLocation(location)
 	if err != nil {
 		logger.WithFields("location", location.RealPath, "error", err).Debug("unable to fetch contents")
@@ -234,6 +248,15 @@ func invokeParser(ctx context.Context, resolver file.Resolver, location file.Loc
 	}
 	defer internal.CloseAndLogError(contentReader, location.AccessPath)
 
+	defer func() {
+		if r := recover(); r != nil {
+			// a malformed file must not take down the whole SBOM; the caller records this as an unknown
+			logger.WithFields("location", location.RealPath, "panic", r).Debug("recovered from panic while parsing file")
+			logger.Tracef("parser panic stack:\n%s", debug.Stack())
+			pkgs, rels, err = nil, nil, fmt.Errorf("recovered from panic while parsing file: %v", r)
+		}
+	}()
+
 	discoveredPackages, discoveredRelationships, err := parser(ctx, resolver, env, file.NewLocationReadCloser(location, contentReader))
 	if err != nil {
 		// these errors are propagated up, and are likely to be coordinate errors

syft/pkg/cataloger/generic/cataloger_test.go

@@ -10,7 +10,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"github.com/anchore/go-sync"
 	"github.com/anchore/syft/internal/unknown"
 	"github.com/anchore/syft/syft/artifact"
 	"github.com/anchore/syft/syft/file"
@@ -190,12 +189,122 @@ func TestClosesFileOnParserPanic(t *testing.T) {
 
 	_, _, err := c.Catalog(ctx, resolver)
 	require.Error(t, err)
-	var panicErr sync.PanicError
-	require.ErrorAs(t, err, &panicErr)
-	assert.Equal(t, "panic!", panicErr.Value)
+	var coordErr *unknown.CoordinateError
+	require.ErrorAs(t, err, &coordErr)
+	assert.ErrorContains(t, err, "recovered from panic while parsing file: panic!")
 	require.True(t, spy.closed)
 }
 
+func TestParserPanicBecomesUnknown(t *testing.T) {
+	tests := []struct {
+		name    string
+		doPanic func()
+		wantErr string
+	}{
+		{
+			name:    "string panic",
+			doPanic: func() { panic("boom") },
+			wantErr: "boom",
+		},
+		{
+			name: "runtime panic",
+			doPanic: func() {
+				var m map[string]int
+				m["x"] = 1
+			},
+			wantErr: "assignment to entry in nil map",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			parser := func(_ context.Context, _ file.Resolver, _ *Environment, reader file.LocationReadCloser) ([]pkg.Package, []artifact.Relationship, error) {
+				if reader.Path() == "testdata/another-path.txt" {
+					tt.doPanic()
+				}
+				return []pkg.Package{{Name: "good", Locations: file.NewLocationSet(reader.Location)}}, nil, nil
+			}
+
+			resolver := file.NewMockResolverForPaths("testdata/a-path.txt", "testdata/another-path.txt")
+			c := NewCataloger("unit-test-cataloger").
+				WithParserByPath(parser, "testdata/a-path.txt", "testdata/another-path.txt")
+
+			pkgs, _, err := c.Catalog(context.Background(), resolver)
+
+			require.Len(t, pkgs, 1)
+			assert.Equal(t, "good", pkgs[0].Name)
+
+			unknowns, remaining := unknown.ExtractCoordinateErrors(err)
+			require.NoError(t, remaining)
+			require.Len(t, unknowns, 1)
+			assert.Equal(t, "testdata/another-path.txt", unknowns[0].Coordinates.RealPath)
+			assert.ErrorContains(t, unknowns[0].Reason, tt.wantErr)
+		})
+	}
+}
+
+func TestProcessorPanicKeepsResults(t *testing.T) {
+	parser := func(_ context.Context, _ file.Resolver, _ *Environment, reader file.LocationReadCloser) ([]pkg.Package, []artifact.Relationship, error) {
+		return []pkg.Package{{Name: "good", Locations: file.NewLocationSet(reader.Location)}}, nil, nil
+	}
+
+	rename := func(pkgs []pkg.Package, rels []artifact.Relationship, err error) ([]pkg.Package, []artifact.Relationship, error) {
+		for i := range pkgs {
+			pkgs[i].Name = "processed"
+		}
+		return pkgs, rels, err
+	}
+
+	tests := []struct {
+		name      string
+		cataloger *Cataloger
+		want      string
+	}{
+		{
+			name: "processor",
+			cataloger: NewCataloger("unit-test-cataloger").
+				WithParserByPath(parser, "testdata/a-path.txt").
+				WithProcessors(func([]pkg.Package, []artifact.Relationship, error) ([]pkg.Package, []artifact.Relationship, error) {
+					panic("boom")
+				}),
+			want: "good",
+		},
+		{
+			// output from earlier processors survives a later processor panicking
+			name: "chained processors",
+			cataloger: NewCataloger("unit-test-cataloger").
+				WithParserByPath(parser, "testdata/a-path.txt").
+				WithProcessors(rename, func([]pkg.Package, []artifact.Relationship, error) ([]pkg.Package, []artifact.Relationship, error) {
+					panic("boom")
+				}),
+			want: "processed",
+		},
+		{
+			name: "resolving processor",
+			cataloger: NewCataloger("unit-test-cataloger").
+				WithParserByPath(parser, "testdata/a-path.txt").
+				WithResolvingProcessors(func(context.Context, file.Resolver, []pkg.Package, []artifact.Relationship, error) ([]pkg.Package, []artifact.Relationship, error) {
+					var m map[string]int
+					m["x"] = 1
+					return nil, nil, nil
+				}),
+			want: "good",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			resolver := file.NewMockResolverForPaths("testdata/a-path.txt")
+
+			pkgs, _, err := tt.cataloger.Catalog(context.Background(), resolver)
+
+			require.NoError(t, err)
+			require.Len(t, pkgs, 1)
+			assert.Equal(t, tt.want, pkgs[0].Name)
+		})
+	}
+}
+
 func Test_CatalogerWithParserByMediaType(t *testing.T) {
 	allParsedPaths := make(map[string]bool)
 	parser := func(_ context.Context, resolver file.Resolver, env *Environment, reader file.LocationReadCloser) ([]pkg.Package, []artifact.Relationship, error) {