From ab9fe53ff2dca2bdcbe2559f1e3f09b8550fdf20 Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Fri, 10 Dec 2021 11:49:50 -0500 Subject: [PATCH] Recover from panics from the stdlib when cataloging malformed binaries (#663) * recover from panics in stdlib binary parsing Signed-off-by: Alex Goodman * add CLI test to cover regression case Signed-off-by: Alex Goodman --- syft/pkg/cataloger/golang/binary_cataloger.go | 6 +-- syft/pkg/cataloger/golang/parse_go_bin.go | 21 ++++++++--- .../pkg/cataloger/golang/parse_go_bin_test.go | 32 +++++++++++++--- test/cli/packages_cmd_test.go | 37 +++++++++++++------ .../image-bad-binaries/Dockerfile | 5 +++ .../image-bad-binaries/sources.list | 1 + test/cli/utils_test.go | 1 + 7 files changed, 79 insertions(+), 24 deletions(-) create mode 100644 test/cli/test-fixtures/image-bad-binaries/Dockerfile create mode 100644 test/cli/test-fixtures/image-bad-binaries/sources.list diff --git a/syft/pkg/cataloger/golang/binary_cataloger.go b/syft/pkg/cataloger/golang/binary_cataloger.go index 4033c21a9..2ddf7d3b1 100644 --- a/syft/pkg/cataloger/golang/binary_cataloger.go +++ b/syft/pkg/cataloger/golang/binary_cataloger.go @@ -40,12 +40,12 @@ func (c *Cataloger) Catalog(resolver source.FileResolver) ([]pkg.Package, []arti for _, location := range fileMatches { r, err := resolver.FileContentsByLocation(location) if err != nil { - return pkgs, nil, fmt.Errorf("failed to resolve file contents by location: %w", err) + return pkgs, nil, fmt.Errorf("failed to resolve file contents by location=%q: %w", location.RealPath, err) } - goPkgs, err := parseGoBin(location, r) + goPkgs, err := parseGoBin(location, r, openExe) if err != nil { - log.Warnf("could not parse possible go binary: %+v", err) + log.Warnf("could not parse possible go binary at %q: %+v", location.RealPath, err) } internal.CloseAndLogError(r, location.RealPath) diff --git a/syft/pkg/cataloger/golang/parse_go_bin.go b/syft/pkg/cataloger/golang/parse_go_bin.go index f98438d64..8992efce6 100644 --- a/syft/pkg/cataloger/golang/parse_go_bin.go +++ b/syft/pkg/cataloger/golang/parse_go_bin.go @@ -2,6 +2,7 @@ package golang import ( "bufio" + "fmt" "io" "strings" @@ -14,19 +15,29 @@ const ( replaceIdentifier = "=>" ) -func parseGoBin(location source.Location, reader io.ReadCloser) ([]pkg.Package, error) { +type exeOpener func(file io.ReadCloser) ([]exe, error) + +func parseGoBin(location source.Location, reader io.ReadCloser, opener exeOpener) (pkgs []pkg.Package, err error) { + var exes []exe + // it has been found that there are stdlib paths within openExe that can panic. We want to prevent this behavior + // bubbling up and halting execution. For this reason we try to recover from any panic and return an error. + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("recovered from panic while parse go binary at %q: %+v", location.RealPath, r) + } + }() + // Identify if bin was compiled by go - exes, err := openExe(reader) + exes, err = opener(reader) if err != nil { - return nil, err + return pkgs, err } - var pkgs []pkg.Package for _, x := range exes { goVersion, mod := findVers(x) pkgs = append(pkgs, buildGoPkgInfo(location, mod, goVersion, x.ArchName())...) } - return pkgs, nil + return pkgs, err } func buildGoPkgInfo(location source.Location, mod, goVersion, arch string) []pkg.Package { diff --git a/syft/pkg/cataloger/golang/parse_go_bin_test.go b/syft/pkg/cataloger/golang/parse_go_bin_test.go index 52cbe0757..b1d645036 100644 --- a/syft/pkg/cataloger/golang/parse_go_bin_test.go +++ b/syft/pkg/cataloger/golang/parse_go_bin_test.go @@ -1,6 +1,7 @@ package golang import ( + "io" "testing" "github.com/anchore/syft/syft/pkg" @@ -144,17 +145,38 @@ func TestBuildGoPkgInfo(t *testing.T) { }, } - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { location := source.Location{ Coordinates: source.Coordinates{ RealPath: "/a-path", FileSystemID: "layer-id", }, } - pkgs := buildGoPkgInfo(location, tt.mod, goCompiledVersion, archDetails) - assert.Equal(t, tt.expected, pkgs) + pkgs := buildGoPkgInfo(location, test.mod, goCompiledVersion, archDetails) + assert.Equal(t, test.expected, pkgs) + }) + } +} + +func Test_parseGoBin_recoversFromPanic(t *testing.T) { + freakOut := func(file io.ReadCloser) ([]exe, error) { + panic("baaahhh!") + } + tests := []struct { + name string + wantPkgs []pkg.Package + wantErr assert.ErrorAssertionFunc + }{ + { + name: "recovers from panic", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + pkgs, err := parseGoBin(source.NewLocation("some/path"), nil, freakOut) + assert.Error(t, err) + assert.Nil(t, pkgs) }) } } diff --git a/test/cli/packages_cmd_test.go b/test/cli/packages_cmd_test.go index afa3c0ada..757142d04 100644 --- a/test/cli/packages_cmd_test.go +++ b/test/cli/packages_cmd_test.go @@ -6,7 +6,8 @@ import ( ) func TestPackagesCmdFlags(t *testing.T) { - request := "docker-archive:" + getFixtureImage(t, "image-pkg-coverage") + coverageImage := "docker-archive:" + getFixtureImage(t, "image-pkg-coverage") + badBinariesImage := "docker-archive:" + getFixtureImage(t, "image-bad-binaries") tests := []struct { name string @@ -25,18 +26,32 @@ func TestPackagesCmdFlags(t *testing.T) { }, { name: "json-output-flag", - args: []string{"packages", "-o", "json", request}, + args: []string{"packages", "-o", "json", coverageImage}, assertions: []traitAssertion{ assertJsonReport, assertSuccessfulReturnCode, }, }, + { + name: "regression-survive-bad-binaries", + // this image has all sorts of rich binaries from the clang-13 test suite that should do pretty bad things + // to the go cataloger binary path. We should NEVER let a panic stop the cataloging process for these + // specific cases. + + // this is more of an integration test, however, to assert the output we want to see from the application + // a CLI test is much easier. + args: []string{"packages", "-vv", badBinariesImage}, + assertions: []traitAssertion{ + assertInOutput("recovered from panic while parse go binary"), + assertSuccessfulReturnCode, + }, + }, { name: "output-env-binding", env: map[string]string{ "SYFT_OUTPUT": "json", }, - args: []string{"packages", request}, + args: []string{"packages", coverageImage}, assertions: []traitAssertion{ assertJsonReport, assertSuccessfulReturnCode, @@ -44,7 +59,7 @@ func TestPackagesCmdFlags(t *testing.T) { }, { name: "table-output-flag", - args: []string{"packages", "-o", "table", request}, + args: []string{"packages", "-o", "table", coverageImage}, assertions: []traitAssertion{ assertTableReport, assertSuccessfulReturnCode, @@ -52,7 +67,7 @@ func TestPackagesCmdFlags(t *testing.T) { }, { name: "default-output-flag", - args: []string{"packages", request}, + args: []string{"packages", coverageImage}, assertions: []traitAssertion{ assertTableReport, assertSuccessfulReturnCode, @@ -60,7 +75,7 @@ func TestPackagesCmdFlags(t *testing.T) { }, { name: "squashed-scope-flag", - args: []string{"packages", "-o", "json", "-s", "squashed", request}, + args: []string{"packages", "-o", "json", "-s", "squashed", coverageImage}, assertions: []traitAssertion{ assertPackageCount(20), assertSuccessfulReturnCode, @@ -68,7 +83,7 @@ func TestPackagesCmdFlags(t *testing.T) { }, { name: "all-layers-scope-flag", - args: []string{"packages", "-o", "json", "-s", "all-layers", request}, + args: []string{"packages", "-o", "json", "-s", "all-layers", coverageImage}, assertions: []traitAssertion{ assertPackageCount(22), assertSuccessfulReturnCode, @@ -76,7 +91,7 @@ func TestPackagesCmdFlags(t *testing.T) { }, { name: "all-layers-scope-flag-by-env", - args: []string{"packages", "-o", "json", request}, + args: []string{"packages", "-o", "json", coverageImage}, env: map[string]string{ "SYFT_PACKAGE_CATALOGER_SCOPE": "all-layers", }, @@ -87,7 +102,7 @@ func TestPackagesCmdFlags(t *testing.T) { }, { name: "attempt-upload-on-cli-switches", - args: []string{"packages", "-vv", "-H", "localhost:8080", "-u", "the-username", "-d", "test-fixtures/image-pkg-coverage/Dockerfile", "--overwrite-existing-image", request}, + args: []string{"packages", "-vv", "-H", "localhost:8080", "-u", "the-username", "-d", "test-fixtures/image-pkg-coverage/Dockerfile", "--overwrite-existing-image", coverageImage}, env: map[string]string{ "SYFT_ANCHORE_PATH": "path/to/api", "SYFT_ANCHORE_PASSWORD": "the-password", @@ -108,7 +123,7 @@ func TestPackagesCmdFlags(t *testing.T) { }, { name: "dockerfile-without-upload-is-invalid", - args: []string{"packages", "-vv", "-d", "test-fixtures/image-pkg-coverage/Dockerfile", request}, + args: []string{"packages", "-vv", "-d", "test-fixtures/image-pkg-coverage/Dockerfile", coverageImage}, assertions: []traitAssertion{ assertNotInOutput("uploading results to localhost:8080"), @@ -118,7 +133,7 @@ func TestPackagesCmdFlags(t *testing.T) { }, { name: "attempt-upload-with-env-host-set", - args: []string{"packages", "-vv", request}, + args: []string{"packages", "-vv", coverageImage}, env: map[string]string{ "SYFT_ANCHORE_HOST": "localhost:8080", }, diff --git a/test/cli/test-fixtures/image-bad-binaries/Dockerfile b/test/cli/test-fixtures/image-bad-binaries/Dockerfile new file mode 100644 index 000000000..7851a945b --- /dev/null +++ b/test/cli/test-fixtures/image-bad-binaries/Dockerfile @@ -0,0 +1,5 @@ +FROM debian:sid +ADD sources.list /etc/apt/sources.list.d/sources.list +RUN apt update -y && apt install -y dpkg-dev +# this as a "macho-invalid" directory which is useful for testing +RUN apt-get source -y clang-13 diff --git a/test/cli/test-fixtures/image-bad-binaries/sources.list b/test/cli/test-fixtures/image-bad-binaries/sources.list new file mode 100644 index 000000000..65c201e37 --- /dev/null +++ b/test/cli/test-fixtures/image-bad-binaries/sources.list @@ -0,0 +1 @@ +deb-src http://deb.debian.org/debian sid main diff --git a/test/cli/utils_test.go b/test/cli/utils_test.go index 550377dd3..9f772c3de 100644 --- a/test/cli/utils_test.go +++ b/test/cli/utils_test.go @@ -15,6 +15,7 @@ import ( ) func getFixtureImage(t testing.TB, fixtureImageName string) string { + t.Logf("obtaining fixture image for %s", fixtureImageName) imagetest.GetFixtureImage(t, "docker-archive", fixtureImageName) return imagetest.GetFixtureImageTarPath(t, fixtureImageName) }