diff --git a/internal/task/executor.go b/internal/task/executor.go index 6cee1242f..610f0a963 100644 --- a/internal/task/executor.go +++ b/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) } }() diff --git a/internal/task/executor_test.go b/internal/task/executor_test.go index 527066cd0..af165b4be 100644 --- a/internal/task/executor_test.go +++ b/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) } diff --git a/internal/unknown/coordinate_error.go b/internal/unknown/coordinate_error.go index caa0a160e..2db6caf7e 100644 --- a/internal/unknown/coordinate_error.go +++ b/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 diff --git a/internal/unknown/coordinate_error_test.go b/internal/unknown/coordinate_error_test.go index 27c53a5ff..e863e6359 100644 --- a/internal/unknown/coordinate_error_test.go +++ b/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) +} diff --git a/syft/file/cataloger/executable/cataloger.go b/syft/file/cataloger/executable/cataloger.go index f6cdb5e65..552698f48 100644 --- a/syft/file/cataloger/executable/cataloger.go +++ b/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") diff --git a/syft/file/cataloger/executable/cataloger_test.go b/syft/file/cataloger/executable/cataloger_test.go new file mode 100644 index 000000000..7b670170c --- /dev/null +++ b/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") +} diff --git a/syft/file/cataloger/filedigest/cataloger.go b/syft/file/cataloger/filedigest/cataloger.go index 9ca6e7867..3340cc5cb 100644 --- a/syft/file/cataloger/filedigest/cataloger.go +++ b/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} } diff --git a/syft/file/cataloger/filedigest/cataloger_test.go b/syft/file/cataloger/filedigest/cataloger_test.go index 00e72a7d1..6188bd066 100644 --- a/syft/file/cataloger/filedigest/cataloger_test.go +++ b/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") +} diff --git a/syft/pkg/cataloger/generic/cataloger.go b/syft/pkg/cataloger/generic/cataloger.go index d357d0e07..ec4a5384a 100644 --- a/syft/pkg/cataloger/generic/cataloger.go +++ b/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 diff --git a/syft/pkg/cataloger/generic/cataloger_test.go b/syft/pkg/cataloger/generic/cataloger_test.go index 2209a3fc9..93f7813d0 100644 --- a/syft/pkg/cataloger/generic/cataloger_test.go +++ b/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) {