From 03fdc8e31f7c4f254ce3e42709eebb5f3350794b Mon Sep 17 00:00:00 2001 From: Will Murphy Date: Mon, 29 Apr 2024 11:04:09 -0400 Subject: [PATCH 1/5] always close ELF cataloger file handles The elf-binary-package-cataloger does its own file IO to account for the possibility of a logical ELF package being broken across multiple physical files. However, this casued it to skip the normal invocation pattern in the generic cataloger code that prevented file leaks. Ensure this cataloger always closes its file handles. Signed-off-by: Will Murphy --- .../cataloger/binary/elf_package_cataloger.go | 54 ++++++++++++------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/syft/pkg/cataloger/binary/elf_package_cataloger.go b/syft/pkg/cataloger/binary/elf_package_cataloger.go index 94f6564004a..9dea999e629 100644 --- a/syft/pkg/cataloger/binary/elf_package_cataloger.go +++ b/syft/pkg/cataloger/binary/elf_package_cataloger.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" + "github.com/anchore/syft/internal" "github.com/anchore/syft/internal/log" "github.com/anchore/syft/internal/mimetype" "github.com/anchore/syft/syft/artifact" @@ -57,31 +58,13 @@ func (c *elfPackageCataloger) Catalog(_ context.Context, resolver file.Resolver) // first find all ELF binaries that have notes var notesByLocation = make(map[elfPackageKey][]elfBinaryPackageNotes) for _, location := range locations { - reader, err := resolver.FileContentsByLocation(location) + notes, key, err := parseElfPackageNotes(resolver, location, c) if err != nil { - return nil, nil, fmt.Errorf("unable to get binary contents %q: %w", location.Path(), err) + return nil, nil, err } - - notes, err := c.parseElfNotes(file.LocationReadCloser{ - Location: location, - ReadCloser: reader, - }) - if err != nil { - log.WithFields("file", location.Path(), "error", err).Trace("unable to parse ELF notes") - continue - } - if notes == nil { continue } - - notes.Location = location - key := elfPackageKey{ - Name: notes.Name, - Version: notes.Version, - PURL: notes.PURL, - CPE: notes.CPE, - } notesByLocation[key] = append(notesByLocation[key], *notes) } @@ -105,6 +88,37 @@ func (c *elfPackageCataloger) Catalog(_ context.Context, resolver file.Resolver) return pkgs, nil, nil } +func parseElfPackageNotes(resolver file.Resolver, location file.Location, c *elfPackageCataloger) (*elfBinaryPackageNotes, elfPackageKey, error) { + reader, err := resolver.FileContentsByLocation(location) + if err != nil { + return nil, elfPackageKey{}, fmt.Errorf("unable to get binary contents %q: %w", location.Path(), err) + } + defer internal.CloseAndLogError(reader, location.AccessPath) + + notes, err := c.parseElfNotes(file.LocationReadCloser{ + Location: location, + ReadCloser: reader, + }) + + if err != nil { + log.WithFields("file", location.Path(), "error", err).Trace("unable to parse ELF notes") + return nil, elfPackageKey{}, nil + } + + if notes == nil { + return nil, elfPackageKey{}, nil + } + + notes.Location = location + key := elfPackageKey{ + Name: notes.Name, + Version: notes.Version, + PURL: notes.PURL, + CPE: notes.CPE, + } + return notes, key, nil +} + func (c *elfPackageCataloger) parseElfNotes(reader file.LocationReadCloser) (*elfBinaryPackageNotes, error) { metadata, err := getELFNotes(reader) if err != nil { From 2d86d92901ac304e9e3915104f391eb75ba774c8 Mon Sep 17 00:00:00 2001 From: Will Murphy Date: Mon, 29 Apr 2024 11:37:29 -0400 Subject: [PATCH 2/5] defer closing of generic cataloger file handles Otherwise, a panicking cataloger could leak file handles. Signed-off-by: Will Murphy --- syft/pkg/cataloger/generic/cataloger.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/syft/pkg/cataloger/generic/cataloger.go b/syft/pkg/cataloger/generic/cataloger.go index 51b1cd337d3..2cfd661e230 100644 --- a/syft/pkg/cataloger/generic/cataloger.go +++ b/syft/pkg/cataloger/generic/cataloger.go @@ -118,20 +118,21 @@ func (c *Cataloger) Catalog(ctx context.Context, resolver file.Resolver) ([]pkg. } for _, req := range c.selectFiles(resolver) { - location, parser := req.Location, req.Parser + log.WithFields("path", req.Location.RealPath).Trace("parsing file contents") - log.WithFields("path", location.RealPath).Trace("parsing file contents") + invokeParser := func(location file.Location, parser Parser) ([]pkg.Package, []artifact.Relationship, error) { + contentReader, err := resolver.FileContentsByLocation(location) + if err != nil { + logger.WithFields("location", location.RealPath, "error", err).Warn("unable to fetch contents") + return nil, nil, nil + } + defer internal.CloseAndLogError(contentReader, location.AccessPath) - contentReader, err := resolver.FileContentsByLocation(location) - if err != nil { - logger.WithFields("location", location.RealPath, "error", err).Warn("unable to fetch contents") - continue + return parser(ctx, resolver, &env, file.NewLocationReadCloser(location, contentReader)) } - - discoveredPackages, discoveredRelationships, err := parser(ctx, resolver, &env, file.NewLocationReadCloser(location, contentReader)) - internal.CloseAndLogError(contentReader, location.AccessPath) + discoveredPackages, discoveredRelationships, err := invokeParser(req.Location, req.Parser) if err != nil { - logger.WithFields("location", location.RealPath, "error", err).Warnf("cataloger failed") + logger.WithFields("location", req.Location.RealPath, "error", err).Warnf("cataloger failed") continue } From 2b7e11222b8ae45d6713cf09dbfdd591373255c6 Mon Sep 17 00:00:00 2001 From: Will Murphy Date: Tue, 30 Apr 2024 06:58:26 -0400 Subject: [PATCH 3/5] add unit test for file closed on panic parser Signed-off-by: Will Murphy --- syft/pkg/cataloger/generic/cataloger.go | 21 ++-- syft/pkg/cataloger/generic/cataloger_test.go | 101 +++++++++++++++++++ 2 files changed, 111 insertions(+), 11 deletions(-) diff --git a/syft/pkg/cataloger/generic/cataloger.go b/syft/pkg/cataloger/generic/cataloger.go index 2cfd661e230..51b1cd337d3 100644 --- a/syft/pkg/cataloger/generic/cataloger.go +++ b/syft/pkg/cataloger/generic/cataloger.go @@ -118,21 +118,20 @@ func (c *Cataloger) Catalog(ctx context.Context, resolver file.Resolver) ([]pkg. } for _, req := range c.selectFiles(resolver) { - log.WithFields("path", req.Location.RealPath).Trace("parsing file contents") + location, parser := req.Location, req.Parser - invokeParser := func(location file.Location, parser Parser) ([]pkg.Package, []artifact.Relationship, error) { - contentReader, err := resolver.FileContentsByLocation(location) - if err != nil { - logger.WithFields("location", location.RealPath, "error", err).Warn("unable to fetch contents") - return nil, nil, nil - } - defer internal.CloseAndLogError(contentReader, location.AccessPath) + log.WithFields("path", location.RealPath).Trace("parsing file contents") - return parser(ctx, resolver, &env, file.NewLocationReadCloser(location, contentReader)) + contentReader, err := resolver.FileContentsByLocation(location) + if err != nil { + logger.WithFields("location", location.RealPath, "error", err).Warn("unable to fetch contents") + continue } - discoveredPackages, discoveredRelationships, err := invokeParser(req.Location, req.Parser) + + discoveredPackages, discoveredRelationships, err := parser(ctx, resolver, &env, file.NewLocationReadCloser(location, contentReader)) + internal.CloseAndLogError(contentReader, location.AccessPath) if err != nil { - logger.WithFields("location", req.Location.RealPath, "error", err).Warnf("cataloger failed") + logger.WithFields("location", location.RealPath, "error", err).Warnf("cataloger failed") continue } diff --git a/syft/pkg/cataloger/generic/cataloger_test.go b/syft/pkg/cataloger/generic/cataloger_test.go index 43d0dc801c3..c22113432c3 100644 --- a/syft/pkg/cataloger/generic/cataloger_test.go +++ b/syft/pkg/cataloger/generic/cataloger_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -86,3 +87,103 @@ func Test_Cataloger(t *testing.T) { } } } + +type spyReturningFileResolver struct { + m *file.MockResolver + s *spyingIoReadCloser +} + +type spyingIoReadCloser struct { + rc io.ReadCloser + closed bool +} + +func newSpyReturningFileResolver(s *spyingIoReadCloser, paths ...string) file.Resolver { + m := file.NewMockResolverForPaths(paths...) + return spyReturningFileResolver{ + m: m, + s: s, + } +} + +func (s *spyingIoReadCloser) Read(p []byte) (n int, err error) { + return s.Read(p) +} + +func (s *spyingIoReadCloser) Close() error { + s.closed = true + return s.rc.Close() +} + +var _ io.ReadCloser = (*spyingIoReadCloser)(nil) + +func (m spyReturningFileResolver) FileContentsByLocation(location file.Location) (io.ReadCloser, error) { + return m.s, nil +} + +func (m spyReturningFileResolver) HasPath(path string) bool { + return m.m.HasPath(path) +} + +func (m spyReturningFileResolver) FilesByPath(paths ...string) ([]file.Location, error) { + return m.m.FilesByPath(paths...) +} + +func (m spyReturningFileResolver) FilesByGlob(patterns ...string) ([]file.Location, error) { + return m.m.FilesByGlob(patterns...) +} + +func (m spyReturningFileResolver) FilesByMIMEType(types ...string) ([]file.Location, error) { + return m.m.FilesByMIMEType(types...) +} + +func (m spyReturningFileResolver) RelativeFileByPath(f file.Location, path string) *file.Location { + return m.m.RelativeFileByPath(f, path) +} + +func (m spyReturningFileResolver) AllLocations(ctx context.Context) <-chan file.Location { + return m.m.AllLocations(ctx) +} + +func (m spyReturningFileResolver) FileMetadataByLocation(location file.Location) (file.Metadata, error) { + return m.m.FileMetadataByLocation(location) +} + +var _ file.Resolver = (*spyReturningFileResolver)(nil) + +func TestClosesFileOnParserPanic(t *testing.T) { + rc := io.NopCloser(strings.NewReader("some string")) + spy := spyingIoReadCloser{ + rc: rc, + } + resolver := newSpyReturningFileResolver(&spy, "test-fixtures/another-path.txt") + ctx := context.TODO() + + processors := []processor{ + func(resolver file.Resolver, env Environment) []request { + return []request{ + { + Location: file.Location{ + LocationData: file.LocationData{ + Coordinates: file.Coordinates{}, + AccessPath: "/some/access/path", + }, + }, + Parser: func(context.Context, file.Resolver, *Environment, file.LocationReadCloser) ([]pkg.Package, []artifact.Relationship, error) { + panic("panic!") + }, + }, + } + }, + } + + c := Cataloger{ + processor: processors, + upstreamCataloger: "unit-test-cataloger", + } + + assert.PanicsWithValue(t, "panic!", func() { + _, _, _ = c.Catalog(ctx, resolver) + }) + require.True(t, spy.closed) +} From e10744246c86a4bfbe77b65c78b5061241adab96 Mon Sep 17 00:00:00 2001 From: Will Murphy Date: Tue, 30 Apr 2024 07:02:14 -0400 Subject: [PATCH 4/5] make invoke parser a static function Signed-off-by: Will Murphy --- syft/pkg/cataloger/generic/cataloger.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/syft/pkg/cataloger/generic/cataloger.go b/syft/pkg/cataloger/generic/cataloger.go index 51b1cd337d3..a9be22712fa 100644 --- a/syft/pkg/cataloger/generic/cataloger.go +++ b/syft/pkg/cataloger/generic/cataloger.go @@ -3,6 +3,7 @@ package generic import ( "context" + "github.com/anchore/go-logger" "github.com/anchore/syft/internal" "github.com/anchore/syft/internal/log" "github.com/anchore/syft/syft/artifact" @@ -122,14 +123,7 @@ func (c *Cataloger) Catalog(ctx context.Context, resolver file.Resolver) ([]pkg. log.WithFields("path", location.RealPath).Trace("parsing file contents") - contentReader, err := resolver.FileContentsByLocation(location) - if err != nil { - logger.WithFields("location", location.RealPath, "error", err).Warn("unable to fetch contents") - continue - } - - discoveredPackages, discoveredRelationships, err := parser(ctx, resolver, &env, file.NewLocationReadCloser(location, contentReader)) - internal.CloseAndLogError(contentReader, location.AccessPath) + discoveredPackages, discoveredRelationships, err := invokeParser(ctx, resolver, location, logger, parser, &env) if err != nil { logger.WithFields("location", location.RealPath, "error", err).Warnf("cataloger failed") continue @@ -145,6 +139,18 @@ func (c *Cataloger) Catalog(ctx context.Context, resolver file.Resolver) ([]pkg. return packages, relationships, nil } +func invokeParser(ctx context.Context, resolver file.Resolver, location file.Location, logger logger.Logger, parser Parser, env *Environment) ([]pkg.Package, []artifact.Relationship, error) { + contentReader, err := resolver.FileContentsByLocation(location) + if err != nil { + logger.WithFields("location", location.RealPath, "error", err).Warn("unable to fetch contents") + return nil, nil, nil + } + defer internal.CloseAndLogError(contentReader, location.AccessPath) + + discoveredPackages, discoveredRelationships, err := parser(ctx, resolver, env, file.NewLocationReadCloser(location, contentReader)) + return discoveredPackages, discoveredRelationships, err +} + // selectFiles takes a set of file trees and resolves and file references of interest for future cataloging func (c *Cataloger) selectFiles(resolver file.Resolver) []request { var requests []request From 420507ea686800e2a9c82fa7c6695b623b0c7453 Mon Sep 17 00:00:00 2001 From: Will Murphy Date: Tue, 30 Apr 2024 07:27:33 -0400 Subject: [PATCH 5/5] push error logging down into invoke parser Signed-off-by: Will Murphy --- syft/pkg/cataloger/generic/cataloger.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/syft/pkg/cataloger/generic/cataloger.go b/syft/pkg/cataloger/generic/cataloger.go index a9be22712fa..49052ec88e0 100644 --- a/syft/pkg/cataloger/generic/cataloger.go +++ b/syft/pkg/cataloger/generic/cataloger.go @@ -125,8 +125,7 @@ func (c *Cataloger) Catalog(ctx context.Context, resolver file.Resolver) ([]pkg. discoveredPackages, discoveredRelationships, err := invokeParser(ctx, resolver, location, logger, parser, &env) if err != nil { - logger.WithFields("location", location.RealPath, "error", err).Warnf("cataloger failed") - continue + continue // logging is handled within invokeParser } for _, p := range discoveredPackages { @@ -143,12 +142,17 @@ func invokeParser(ctx context.Context, resolver file.Resolver, location file.Loc contentReader, err := resolver.FileContentsByLocation(location) if err != nil { logger.WithFields("location", location.RealPath, "error", err).Warn("unable to fetch contents") - return nil, nil, nil + return nil, nil, err } defer internal.CloseAndLogError(contentReader, location.AccessPath) discoveredPackages, discoveredRelationships, err := parser(ctx, resolver, env, file.NewLocationReadCloser(location, contentReader)) - return discoveredPackages, discoveredRelationships, err + if err != nil { + logger.WithFields("location", location.RealPath, "error", err).Warnf("cataloger failed") + return nil, nil, err + } + + return discoveredPackages, discoveredRelationships, nil } // selectFiles takes a set of file trees and resolves and file references of interest for future cataloging