From 93b2873c28db0899bfc6e1565404539bd2500a29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Mon, 27 Dec 2021 18:12:12 +0100 Subject: [PATCH] ensure the runtime is built in a reproducible way We went to great lengths to ensure garble builds are reproducible. This includes how the tool itself works, as its behavior should be the same given the same inputs. However, we made one crucial mistake with the runtime package. It has go:linkname directives pointing at other packages, and some of those pointed packages aren't its dependencies. Imagine two scenarios where garble builds the runtime package: 1) We run "garble build runtime". The way we handle linkname directives calls listPackage on the target package, to obfuscate the target's import path and object name. However, since we only obtained build info of runtime and its deps, calls for some linknames such as listPackage("sync/atomic") will fail. The linkname directive will leave its target untouched. 2) We run "garble build std". Unlike the first scenario, all listPackage calls issued by runtime's linkname directives will succeed, so its linkname directive targets will be obfuscated. At best, this can result in inconsistent builds, depending on how the runtime package was built. At worst, the mismatching object names can result in errors at link time, if the target packages are actually used. The modified test reproduces the worst case scenario reliably, when the fix is reverted: > env GOCACHE=${WORK}/gocache-empty > garble build -a runtime > garble build -o=out_rebuild ./stdimporter [stderr] # test/main/stdimporter JZzQivnl.NtQJu0H3: relocation target JZzQivnl.iioHinYT not defined JZzQivnl.NtQJu0H3.func9: relocation target JZzQivnl.yz5z0NaH not defined JZzQivnl.(*ypvqhKiQ).String: relocation target JZzQivnl.eVciBQeI not defined JZzQivnl.(*ypvqhKiQ).PkgPath: relocation target JZzQivnl.eVciBQeI not defined [...] The fix consists of two steps. First, if we're building the runtime and listPackage fails on a package, that means we ran into scenario 1 above. To avoid the inconsistency, we fill ListedPackages with "go list [...] std". This means we'll always build runtime as described in scenario 2 above. Second, when building packages other than the runtime, we only allow listPackage to succeed if we're listing a dependency of the current package. This ensures we won't run into similar reproducibility bugs in the future. Finally, re-enable test-gotip on CI since this was the last test flake. --- .github/workflows/test.yml | 3 +- main.go | 14 +++++++++- shared.go | 52 +++++++++++++++++++++++++++-------- testdata/scripts/gogarble.txt | 9 ++++-- testdata/scripts/literals.txt | 2 +- 5 files changed, 61 insertions(+), 19 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index be93490..138b829 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -32,13 +32,12 @@ jobs: go test -race ./... test-gotip: - if: ${{ false }} # this fails on tip: go clean -cache && go test -run Script/goprivate runs-on: ubuntu-latest continue-on-error: true # master may not be as stable steps: - name: Install Go env: - GO_COMMIT: 578ada410de8065dbca46bca08a5993d1307f423 # 2021-11-09 + GO_COMMIT: b357b05b70d2b8c4988ac2a27f2af176e7a09e1b # 2021-12-21 run: | cd $HOME mkdir $HOME/gotip diff --git a/main.go b/main.go index 86f0a1b..a0a70dc 100644 --- a/main.go +++ b/main.go @@ -440,7 +440,13 @@ This command wraps "go %s". Below is its help: return nil, err } - if err := setListedPackages(args); err != nil { + binaryBuildID, err := buildidOf(cache.ExecPath) + if err != nil { + return nil, err + } + cache.BinaryContentID = decodeHash(splitContentID(binaryBuildID)) + + if err := appendListedPackages(args...); err != nil { return nil, err } @@ -807,6 +813,12 @@ func (tf *transformer) handleDirectives(comments []*ast.CommentGroup) { // probably a malformed linkname directive continue } + switch newName { + case "main.main", "main..inittask", "runtime..inittask": + // The runtime uses some special symbols with "..". + // We aren't touching those at the moment. + continue + } // If the package path has multiple dots, split on the // last one. diff --git a/shared.go b/shared.go index 6faff59..a5ea8d8 100644 --- a/shared.go +++ b/shared.go @@ -10,6 +10,8 @@ import ( "path/filepath" "strings" "time" + + "golang.org/x/mod/module" ) // sharedCache is shared as a read-only cache between the many garble toolexec @@ -153,9 +155,9 @@ func (p *listedPackage) obfuscatedImportPath() string { return newPath } -// setListedPackages gets information about the current package +// appendListedPackages gets information about the current package // and all of its dependencies -func setListedPackages(patterns []string) error { +func appendListedPackages(patterns ...string) error { startTime := time.Now() args := []string{"list", "-json", "-deps", "-export", "-trimpath"} args = append(args, cache.ForwardBuildFlags...) @@ -163,7 +165,7 @@ func setListedPackages(patterns []string) error { cmd := exec.Command("go", args...) defer func() { - debugf("original build finished in %s via: go %s", debugSince(startTime), strings.Join(args, " ")) + debugf("original build info obtained in %s via: go %s", debugSince(startTime), strings.Join(args, " ")) }() stdout, err := cmd.StdoutPipe() @@ -178,14 +180,10 @@ func setListedPackages(patterns []string) error { return fmt.Errorf("go list error: %v", err) } - binaryBuildID, err := buildidOf(cache.ExecPath) - if err != nil { - return err - } - cache.BinaryContentID = decodeHash(splitContentID(binaryBuildID)) - dec := json.NewDecoder(stdout) - cache.ListedPackages = make(map[string]*listedPackage) + if cache.ListedPackages == nil { + cache.ListedPackages = make(map[string]*listedPackage) + } for dec.More() { var pkg listedPackage if err := dec.Decode(&pkg); err != nil { @@ -216,7 +214,8 @@ func setListedPackages(patterns []string) error { } } - if !anyToObfuscate { + // Don't error if the user ran: GOGARBLE='*' garble build runtime + if !anyToObfuscate && !module.MatchPrefixPatterns(cache.GOGARBLE, "runtime") { return fmt.Errorf("GOGARBLE=%q does not match any packages to be built", cache.GOGARBLE) } @@ -225,6 +224,10 @@ func setListedPackages(patterns []string) error { // listPackage gets the listedPackage information for a certain package func listPackage(path string) (*listedPackage, error) { + if path == curPkg.ImportPath { + return curPkg, nil + } + // If the path is listed in the top-level ImportMap, use its mapping instead. // This is a common scenario when dealing with vendored packages in GOROOT. // The map is flat, so we don't need to recurse. @@ -233,8 +236,33 @@ func listPackage(path string) (*listedPackage, error) { } pkg, ok := cache.ListedPackages[path] + + // The runtime may list any package in std, even those it doesn't depend on. + // This is due to how it linkname-implements std packages, + // such as sync/atomic or reflect, without importing them in any way. + // If ListedPackages lacks such a package we fill it with "std". + if curPkg.ImportPath == "runtime" { + if ok { + return pkg, nil + } + if err := appendListedPackages("std"); err != nil { + panic(err) // should never happen + } + pkg, ok := cache.ListedPackages[path] + if !ok { + panic(fmt.Sprintf("runtime listed a std package we can't find: %s", path)) + } + return pkg, nil + } + // Packages other than runtime can list any package, + // as long as they depend on it directly or indirectly. if !ok { return nil, fmt.Errorf("path not found in listed packages: %s", path) } - return pkg, nil + for _, dep := range curPkg.Deps { + if dep == pkg.ImportPath { + return pkg, nil + } + } + return nil, fmt.Errorf("refusing to list non-dependency package: %s", path) } diff --git a/testdata/scripts/gogarble.txt b/testdata/scripts/gogarble.txt index 6ac0ea2..eafb655 100644 --- a/testdata/scripts/gogarble.txt +++ b/testdata/scripts/gogarble.txt @@ -38,10 +38,13 @@ garble build std # support ImportMap when linking. garble build -o=out ./stdimporter -# Also check that a full rebuild is reproducible, -# with -a to rebuild all packages. +# Also check that a full rebuild is reproducible, via a new GOCACHE. # This is slow, but necessary to uncover bugs hidden by the build cache. -garble build -o=out_rebuild -a ./stdimporter +# We also forcibly rebuild runtime on its own, given it used to be non-reproducible +# due to its use of linknames pointing at std packages it doesn't depend upon. +env GOCACHE=${WORK}/gocache-empty +garble build -a runtime +garble build -o=out_rebuild ./stdimporter bincmp out_rebuild out -- go.mod -- diff --git a/testdata/scripts/literals.txt b/testdata/scripts/literals.txt index bd48468..09a578d 100644 --- a/testdata/scripts/literals.txt +++ b/testdata/scripts/literals.txt @@ -50,7 +50,7 @@ grep '^\s+\w+ = .*\bappend\(\w+,(\s+\w+\[\d+\][\^\-+]\w+\[\d+\],?)+\)$' debug1/t grep '^\s+type \w+ func\(byte\) \w+$' debug1/test/main/extra_literals.go # Finally, sanity check that we can build all of std with -literals. -# Analogous to goprivate.txt. +# Analogous to gogarble.txt. env GOGARBLE='*' garble -literals build std