From 712bbae0a65ff773ab21b045c2676682f533631c Mon Sep 17 00:00:00 2001 From: Zvonimir Pavlinovic Date: Wed, 23 Aug 2023 14:03:13 -0700 Subject: [PATCH] internal/vulncheck/internal/buildinfo: support stripped darwin binaries With go1.22 and its prereleases, binaries are also stripped on darwin. This cannot be observed by checking emptiness of the symbol table, yet by non-existence of program symbols, "runtime.main" being a symbol that every program should have. Fixes golang/go#61051 Change-Id: If39214df9531bee66931a4155a2a8fbfbf3823cb Reviewed-on: https://go-review.googlesource.com/c/vuln/+/522157 Run-TryBot: Zvonimir Pavlinovic Reviewed-by: Cherry Mui TryBot-Result: Gopher Robot --- cmd/govulncheck/main_command_118_test.go | 3 +- .../internal/buildinfo/additions_buildinfo.go | 20 +++++- .../internal/buildinfo/additions_scan_test.go | 53 --------------- .../buildinfo/additions_stripped_122_test.go | 38 +++++++++++ .../buildinfo/additions_stripped_test.go | 68 +++++++++++++++++++ .../vulncheck/internal/buildinfo/buildinfo.go | 1 + 6 files changed, 125 insertions(+), 58 deletions(-) create mode 100644 internal/vulncheck/internal/buildinfo/additions_stripped_122_test.go create mode 100644 internal/vulncheck/internal/buildinfo/additions_stripped_test.go diff --git a/cmd/govulncheck/main_command_118_test.go b/cmd/govulncheck/main_command_118_test.go index 7f056c4e..69bd7553 100644 --- a/cmd/govulncheck/main_command_118_test.go +++ b/cmd/govulncheck/main_command_118_test.go @@ -142,8 +142,7 @@ func TestCommand(t *testing.T) { } runTestSuite(t, filepath.Join(testDir, "testdata"), govulndbURI.String(), *update) if runtime.GOOS != "darwin" { - // TODO(https://go.dev/issue/61051): binaries are not currently stripped on darwin. - // This is expected to change in Go 1.22. + // Binaries are not stripped on darwin with go1.21 and earlier. See #61051. runTestSuite(t, filepath.Join(testDir, "testdata/strip"), govulndbURI.String(), *update) } } diff --git a/internal/vulncheck/internal/buildinfo/additions_buildinfo.go b/internal/vulncheck/internal/buildinfo/additions_buildinfo.go index 1bdd38c7..642920be 100644 --- a/internal/vulncheck/internal/buildinfo/additions_buildinfo.go +++ b/internal/vulncheck/internal/buildinfo/additions_buildinfo.go @@ -192,7 +192,10 @@ func (x *peExe) PCLNTab() ([]byte, uint64) { // SymbolInfo is derived from cmd/internal/objfile/macho.go:symbols. func (x *machoExe) SymbolInfo(name string) (uint64, uint64, io.ReaderAt, error) { - sym := x.lookupSymbol(name) + sym, err := x.lookupSymbol(name) + if err != nil { + return 0, 0, nil, err + } if sym == nil { return 0, 0, nil, fmt.Errorf("no symbol %q", name) } @@ -203,15 +206,26 @@ func (x *machoExe) SymbolInfo(name string) (uint64, uint64, io.ReaderAt, error) return sym.Value, seg.Addr, seg.ReaderAt, nil } -func (x *machoExe) lookupSymbol(name string) *macho.Symbol { +func (x *machoExe) lookupSymbol(name string) (*macho.Symbol, error) { + const mustExistSymbol = "runtime.main" x.symbolsOnce.Do(func() { x.symbols = make(map[string]*macho.Symbol, len(x.f.Symtab.Syms)) for _, s := range x.f.Symtab.Syms { s := s // make a copy to prevent aliasing x.symbols[s.Name] = &s } + // In the presence of stripping, the symbol table for darwin + // binaries will not be empty, but the program symbols will + // be missing. + if _, ok := x.symbols[mustExistSymbol]; !ok { + x.symbolsErr = ErrNoSymbols + } }) - return x.symbols[name] + + if x.symbolsErr != nil { + return nil, x.symbolsErr + } + return x.symbols[name], nil } func (x *machoExe) segmentContaining(addr uint64) *macho.Segment { diff --git a/internal/vulncheck/internal/buildinfo/additions_scan_test.go b/internal/vulncheck/internal/buildinfo/additions_scan_test.go index 0948d486..7e238cea 100644 --- a/internal/vulncheck/internal/buildinfo/additions_scan_test.go +++ b/internal/vulncheck/internal/buildinfo/additions_scan_test.go @@ -63,56 +63,3 @@ func TestExtractPackagesAndSymbols(t *testing.T) { } }) } - -// TestStrippedBinary checks that there is no symbol table for -// stripped binaries. This does not include darwin binaries. -// For more info, see #61051. -func TestStrippedBinary(t *testing.T) { - // We exclude darwin as its stripped binaries seem to - // preserve the symbol table. See TestStrippedDarwin. - testAll(t, []string{"linux", "windows", "freebsd"}, []string{"amd64", "386", "arm", "arm64"}, - func(t *testing.T, goos, goarch string) { - binary, done := test.GoBuild(t, "testdata", "", true, "GOOS", goos, "GOARCH", goarch) - defer done() - - f, err := os.Open(binary) - if err != nil { - t.Fatal(err) - } - defer f.Close() - _, syms, _, err := ExtractPackagesAndSymbols(f) - if err != nil { - t.Fatal(err) - } - if syms != nil { - t.Errorf("want empty symbol table; got %v symbols", len(syms)) - } - }) -} - -// TestStrippedDarwin checks that the symbol table exists and -// is complete on darwin even in the presence of stripping. -// This test will become obsolete once #61051 is addressed. -func TestStrippedDarwin(t *testing.T) { - t.Skip("to temporarily resolve #61511") - testAll(t, []string{"darwin"}, []string{"amd64", "386", "arm", "arm64"}, - func(t *testing.T, goos, goarch string) { - binary, done := test.GoBuild(t, "testdata", "", true, "GOOS", goos, "GOARCH", goarch) - defer done() - - f, err := os.Open(binary) - if err != nil { - t.Fatal(err) - } - defer f.Close() - _, syms, _, err := ExtractPackagesAndSymbols(f) - if err != nil { - t.Fatal(err) - } - got := syms["main"] - want := []string{"f", "g", "main"} - if !cmp.Equal(got, want) { - t.Errorf("\ngot %q\nwant %q", got, want) - } - }) -} diff --git a/internal/vulncheck/internal/buildinfo/additions_stripped_122_test.go b/internal/vulncheck/internal/buildinfo/additions_stripped_122_test.go new file mode 100644 index 00000000..98620372 --- /dev/null +++ b/internal/vulncheck/internal/buildinfo/additions_stripped_122_test.go @@ -0,0 +1,38 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.22 +// +build go1.22 + +package buildinfo + +import ( + "os" + "testing" + + "golang.org/x/vuln/internal/test" +) + +// TestStrippedBinary checks that there is no symbol table for +// stripped binaries. +func TestStrippedBinary(t *testing.T) { + testAll(t, []string{"linux", "windows", "freebsd", "darwin"}, []string{"amd64", "386", "arm", "arm64"}, + func(t *testing.T, goos, goarch string) { + binary, done := test.GoBuild(t, "testdata", "", true, "GOOS", goos, "GOARCH", goarch) + defer done() + + f, err := os.Open(binary) + if err != nil { + t.Fatal(err) + } + defer f.Close() + _, syms, _, err := ExtractPackagesAndSymbols(f) + if err != nil { + t.Fatal(err) + } + if syms != nil { + t.Errorf("want empty symbol table; got %v symbols", len(syms)) + } + }) +} diff --git a/internal/vulncheck/internal/buildinfo/additions_stripped_test.go b/internal/vulncheck/internal/buildinfo/additions_stripped_test.go new file mode 100644 index 00000000..ce10de6e --- /dev/null +++ b/internal/vulncheck/internal/buildinfo/additions_stripped_test.go @@ -0,0 +1,68 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.18 && !go1.22 +// +build go1.18,!go1.22 + +package buildinfo + +import ( + "os" + "testing" + + "github.com/google/go-cmp/cmp" + "golang.org/x/vuln/internal/test" +) + +// TestStrippedBinary checks that there is no symbol table for +// stripped binaries. This does not include darwin binaries. +// For more info, see #61051. +func TestStrippedBinary(t *testing.T) { + // We exclude darwin as its stripped binaries seem to + // preserve the symbol table. See TestStrippedDarwin. + testAll(t, []string{"linux", "windows", "freebsd"}, []string{"amd64", "386", "arm", "arm64"}, + func(t *testing.T, goos, goarch string) { + binary, done := test.GoBuild(t, "testdata", "", true, "GOOS", goos, "GOARCH", goarch) + defer done() + + f, err := os.Open(binary) + if err != nil { + t.Fatal(err) + } + defer f.Close() + _, syms, _, err := ExtractPackagesAndSymbols(f) + if err != nil { + t.Fatal(err) + } + if syms != nil { + t.Errorf("want empty symbol table; got %v symbols", len(syms)) + } + }) +} + +// TestStrippedDarwin checks that the symbol table exists and +// is complete on darwin even in the presence of stripping. +// For more info, see #61051. +func TestStrippedDarwin(t *testing.T) { + testAll(t, []string{"darwin"}, []string{"amd64", "386"}, + func(t *testing.T, goos, goarch string) { + binary, done := test.GoBuild(t, "testdata", "", true, "GOOS", goos, "GOARCH", goarch) + defer done() + + f, err := os.Open(binary) + if err != nil { + t.Fatal(err) + } + defer f.Close() + _, syms, _, err := ExtractPackagesAndSymbols(f) + if err != nil { + t.Fatal(err) + } + got := syms["main"] + want := []string{"f", "g", "main"} + if !cmp.Equal(got, want) { + t.Errorf("\ngot %q\nwant %q", got, want) + } + }) +} diff --git a/internal/vulncheck/internal/buildinfo/buildinfo.go b/internal/vulncheck/internal/buildinfo/buildinfo.go index 1be795ee..f29dffa2 100644 --- a/internal/vulncheck/internal/buildinfo/buildinfo.go +++ b/internal/vulncheck/internal/buildinfo/buildinfo.go @@ -174,6 +174,7 @@ type machoExe struct { symbols map[string]*macho.Symbol // Addition: symbols in the binary symbolsOnce sync.Once // Addition: for computing symbols + symbolsErr error // Addition: error for computing symbols } func (x *machoExe) ReadData(addr, size uint64) ([]byte, error) {