pr: feedback

Signed-off-by: Christopher Phillips <32073428+spiffcs@users.noreply.github.com>

cmd/syft/internal/options/golang.go

@@ -1,7 +1,6 @@
 package options
 
 import (
-	"fmt"
 	"strings"
 
 	"github.com/anchore/clio"
@@ -52,11 +51,7 @@ a more accurate version from the binary.`)
 }
 
 func (o *golangConfig) PostLoad() error {
-	parsed := o.CaptureSymbols.Parse()
+	o.CaptureSymbols = o.CaptureSymbols.Parse()
-	if parsed == "" {
-		return fmt.Errorf("invalid value %q for golang.capture-symbols; valid values are: none, stdlib, all", o.CaptureSymbols)
-	}
-	o.CaptureSymbols = parsed
 	return nil
 }
 

cmd/syft/internal/options/golang_test.go

@@ -31,14 +31,14 @@ func Test_golangConfig_PostLoad(t *testing.T) {
 			expected: cataloging.SymbolScopeNone,
 		},
 		{
-			name:    "error on invalid value",
+			name:     "invalid value defaults to none",
-			cfg:     golangConfig{CaptureSymbols: "bogus"},
+			cfg:      golangConfig{CaptureSymbols: "bogus"},
-			wantErr: assert.Error,
+			expected: cataloging.SymbolScopeNone,
 		},
 		{
-			name:    "boolean spellings are not valid",
+			name:     "boolean spellings default to none",
-			cfg:     golangConfig{CaptureSymbols: "true"},
+			cfg:      golangConfig{CaptureSymbols: "true"},
-			wantErr: assert.Error,
+			expected: cataloging.SymbolScopeNone,
 		},
 	}
 	for _, tt := range tests {

syft/cataloging/symbols.go

@@ -16,16 +16,13 @@ const (
 	SymbolScopeAll SymbolScope = "all"
 )
 
-// Parse normalizes a SymbolScope, treating an empty (unset) value as SymbolScopeNone. It returns an empty
+// Parse normalizes a SymbolScope, treating empty (unset) and unrecognized values as SymbolScopeNone.
-// SymbolScope to signal an unrecognized value, which callers validate against.
 func (s SymbolScope) Parse() SymbolScope {
 	switch strings.ToLower(strings.TrimSpace(string(s))) {
 	case string(SymbolScopeAll):
 		return SymbolScopeAll
 	case string(SymbolScopeStdlib):
 		return SymbolScopeStdlib
-	case string(SymbolScopeNone), "":
-		return SymbolScopeNone
 	}
-	return ""
+	return SymbolScopeNone
 }

syft/cataloging/symbols_test.go

@@ -18,9 +18,9 @@ func Test_SymbolScope_Parse(t *testing.T) {
 		{"Stdlib", SymbolScopeStdlib},
 		{"none", SymbolScopeNone},
 		{"", SymbolScopeNone},
-		{"true", ""},
+		{"true", SymbolScopeNone},
-		{"false", ""},
+		{"false", SymbolScopeNone},
-		{"bogus", ""},
+		{"bogus", SymbolScopeNone},
 	}
 	for _, test := range tests {
 		t.Run(test.input, func(t *testing.T) {

syft/pkg/cataloger/golang/parse_go_binary.go

@@ -64,7 +64,7 @@ func newGoBinaryCataloger(opts CatalogerConfig) *goBinaryCataloger {
 	return &goBinaryCataloger{
 		licenseResolver:   newGoLicenseResolver(binaryCatalogerName, opts),
 		mainModuleVersion: opts.MainModuleVersion,
-		symbolScope:       opts.CaptureSymbols.Parse(),
+		symbolScope:       opts.CaptureSymbols,
 		stdlibSymbols:     make(map[file.Coordinates][]string),
 	}
 }

syft/pkg/cataloger/golang/stdlib_package.go

@@ -80,10 +80,10 @@ func generateStdlibCpe(version string) (stdlibCpe cpe.CPE, err error) {
 
 	// we also need to trim starting from the first +<metadata>  to
 	// correctly extract potential rc candidate information for cpe generation
-	// ex: 2.0.0-rc.1+build.123 -> 2.0.0-rc.1; if no + is found then + is returned
+	// ex: 2.0.0-rc.1+build.123 -> 2.0.0-rc.1; if no + is found version is unchanged
-	after, _, found := strings.Cut("+", version)
+	before, _, found := strings.Cut(version, "+")
 	if found {
-		version = after
+		version = before
 	}
 
 	// extracting <version> and <candidate>

syft/pkg/cataloger/golang/symbols.go

@@ -31,6 +31,7 @@ func getSymbols(r io.ReaderAt) (syms []binarySymbol, err error) {
 	defer func() {
 		if r := recover(); r != nil {
 			// the gosym package can panic on malformed pclntab data
+			syms = nil
 			err = fmt.Errorf("recovered from panic while reading pclntab: %v", r)
 		}
 	}()
@@ -84,6 +85,7 @@ func packagePathFromSymbolName(name string) string {
 	if isCompilerGeneratedName(name) {
 		return ""
 	}
+	name = nameWithoutTypeArgs(name)
 	slash := strings.LastIndex(name, "/")
 	dot := strings.IndexByte(name[slash+1:], '.')
 	if dot < 0 {
@@ -92,13 +94,62 @@ func packagePathFromSymbolName(name string) string {
 	return name[:slash+1+dot]
 }
 
+// nameWithoutTypeArgs strips the type-argument portion from an instantiated generic symbol name, e.g.
+// "foo/bar.Do[net/url.Values]" -> "foo/bar.Do". The slashes and dots inside the brackets would otherwise
+// corrupt package-path derivation (yielding "foo/bar.Do[net" for the example above). Mirrors
+// debug/gosym's (*Sym).nameWithoutInst.
+func nameWithoutTypeArgs(name string) string {
+	start := strings.IndexByte(name, '[')
+	if start < 0 {
+		return name
+	}
+	end := strings.LastIndexByte(name, ']')
+	if end < 0 {
+		// malformed: an opening bracket should always have a closing one
+		return name
+	}
+	return name[:start] + name[end+1:]
+}
+
+// oldStyleCompilerGeneratedPrefixes match compiler/linker-generated symbols from toolchains older than
+// go1.20, which used "." where newer toolchains use ":" (e.g. "go.buildid" is now "go:buildid",
+// "go.type.*" is now "go:type.*"). These must be prefix (not substring) matches, and a bare "go." prefix
+// is not enough: legitimate module paths such as "go.uber.org/zap" also start with "go.".
+var oldStyleCompilerGeneratedPrefixes = []string{
+	"go.buildid",
+	"go.builtin.",
+	"go.constinfo.",
+	"go.cuinfo.",
+	"go.func.",
+	"go.importpath.",
+	"go.info.",
+	"go.interface.",
+	"go.itab.",
+	"go.itablink.",
+	"go.map.",
+	"go.shape.",
+	"go.string.",
+	"go.type.",
+	"go.typelink.",
+	"type.",
+}
+
 // isCompilerGeneratedName reports whether a symbol name was synthesized by the compiler or linker rather
-// than declared in Go source. These names use ':' or '..' (e.g. "type:.eq.*", "type..hash.*",
+// than declared in Go source. Since go1.20 these names contain ':' or '..' (e.g. "type:.eq.*",
-// "go:string.*") — byte sequences that never appear in a real Go import path or identifier — so they
+// "go:string.*") — byte sequences that never appear in a real Go import path or identifier. Older
-// belong to no package and are dropped rather than mis-attributed (e.g. bucketed under a bogus "type"
+// toolchains used '.' as the separator (e.g. "go.type.*", "type..hash.*"), which is matched against the
-// stdlib package).
+// known reserved prefixes. Such names belong to no package and are dropped rather than mis-attributed
+// (e.g. bucketed under a bogus "type" stdlib package).
 func isCompilerGeneratedName(name string) bool {
-	return strings.Contains(name, ":") || strings.Contains(name, "..")
+	if strings.Contains(name, ":") || strings.Contains(name, "..") {
+		return true
+	}
+	for _, prefix := range oldStyleCompilerGeneratedPrefixes {
+		if strings.HasPrefix(name, prefix) {
+			return true
+		}
+	}
+	return false
 }
 
 // funcNameTable returns every function name recorded in the pclntab's funcname table, including the

syft/pkg/cataloger/golang/symbols_test.go

@@ -174,12 +174,24 @@ func Test_packagePathFromSymbolName(t *testing.T) {
 		{"github.com/foo/bar.Parse.func1", "github.com/foo/bar"},
 		{"main.main", "main"},
 		{"runtime.gcBgMarkWorker", "runtime"},
+		// generic instantiations: type arguments must not corrupt the package path
+		{"foo/bar.Do[net/url.Values]", "foo/bar"},
+		{"github.com/foo/bar.Map[go.shape.int,go.shape.string]", "github.com/foo/bar"},
+		{"main.Do[go.shape.int]", "main"},
 		// no package-qualifying dot
 		{"runtime", ""},
 		// compiler/linker-generated symbols belong to no package
 		{"type:.eq.[]string", ""},
 		{"type..hash.runtime._type", ""},
 		{"go:string.\"foo\"", ""},
+		// pre-go1.20 toolchains generated symbols with "." where newer ones use ":"
+		{"go.buildid", ""},
+		{"go.type.*runtime._type", ""},
+		{"go.itab.*os.File,io.Reader", ""},
+		{"go.string.\"foo\"", ""},
+		// module paths that begin with "go." are not compiler-generated
+		{"go.uber.org/zap.(*Logger).Info", "go.uber.org/zap"},
+		{"go.opentelemetry.io/otel.Tracer", "go.opentelemetry.io/otel"},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {