From 66b61406c12990b7a02c906388027b8ea6c26b4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Sat, 17 Feb 2024 23:59:07 +0000 Subject: [PATCH] obfuscate syscall again to fix x/sys/unix When updating Garble to support Go 1.22.0, CI on MacOS spotted that the syscall package was failing to build given that it uses assembly code which is only allowed in some std packages. That allowlist is based on import paths, and we were obfuscating the syscall package's import path, so that was breaking GOOS=darwin. As a fix, I added syscall to runtimeAndDeps to not obfuscate it. That wasn't a great fix; it's not part of runtime and its dependencies, and there's no reason we should avoid obfuscating the package contents. Not obfuscating the contents in fact broke x/sys/unix, as it contains a copy of syscall.Rlimit which it type converted with. Undo that fix and reinstate the gogarble.txtar syscall test. Implement the fix where we only leave syscall's import path alone. Add a regression test, and add a note about adding x/net and x/sys to check-third-party.sh so that we can catch these bugs earlier. Fixes #830. --- go_std_tables.go | 6 ------ scripts/check-third-party.sh | 3 +++ scripts/gen-go-std-tables.sh | 6 ------ shared.go | 6 +++++- testdata/script/gogarble.txtar | 6 ++++-- testdata/script/implement.txtar | 15 +++++++++++++++ 6 files changed, 27 insertions(+), 15 deletions(-) diff --git a/go_std_tables.go b/go_std_tables.go index cacd5d3..01e22df 100644 --- a/go_std_tables.go +++ b/go_std_tables.go @@ -20,12 +20,6 @@ var runtimeAndDeps = map[string]bool{ "runtime/internal/sys": true, "runtime/internal/syscall": true, "runtime": true, - // Not runtime dependencies, but still use tricks allowed by import path. - // TODO: collect directly from cmd/internal/objabi/pkgspecial.go, - // in this particular case from allowAsmABIPkgs. - "reflect": true, - "syscall": true, - "runtime/internal/startlinetest": true, } var runtimeLinknamed = []string{ diff --git a/scripts/check-third-party.sh b/scripts/check-third-party.sh index f820764..d16ff66 100755 --- a/scripts/check-third-party.sh +++ b/scripts/check-third-party.sh @@ -33,6 +33,9 @@ modules=( github.com/andybalholm/brotli v1.0.4 # TODO: consider github.com/mattn/go-sqlite3 to cover a DB and more cgo + + # TODO: add x/net and x/unix as they include hacks like go:linkname + # or copying syscall types. ) SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &>/dev/null && pwd) diff --git a/scripts/gen-go-std-tables.sh b/scripts/gen-go-std-tables.sh index 62cb389..2ebe255 100755 --- a/scripts/gen-go-std-tables.sh +++ b/scripts/gen-go-std-tables.sh @@ -34,12 +34,6 @@ var runtimeAndDeps = map[string]bool{ $(for path in ${runtime_and_deps}; do echo "\"${path}\": true," done) - // Not runtime dependencies, but still use tricks allowed by import path. - // TODO: collect directly from cmd/internal/objabi/pkgspecial.go, - // in this particular case from allowAsmABIPkgs. - "reflect": true, - "syscall": true, - "runtime/internal/startlinetest": true, } var runtimeLinknamed = []string{ diff --git a/shared.go b/shared.go index 7539191..bcadff0 100644 --- a/shared.go +++ b/shared.go @@ -183,8 +183,12 @@ func (p *listedPackage) obfuscatedImportPath() string { // * runtime: it is special in many ways // * reflect: its presence turns down dead code elimination // * embed: its presence enables using //go:embed + // * others like syscall are allowed by import path to have more ABI tricks + // + // TODO: collect directly from cmd/internal/objabi/pkgspecial.go, + // in this particular case from allowAsmABIPkgs. switch p.ImportPath { - case "runtime", "reflect", "embed": + case "runtime", "reflect", "embed", "syscall", "runtime/internal/startlinetest": return p.ImportPath } // Intrinsics are matched by package import path as well. diff --git a/testdata/script/gogarble.txtar b/testdata/script/gogarble.txtar index 38c7c01..270fbdf 100644 --- a/testdata/script/gogarble.txtar +++ b/testdata/script/gogarble.txtar @@ -36,11 +36,11 @@ exec garble build std # Also ensure we are obfuscating low-level std packages. exec garble build -o=out ./stdimporter ! stderr . # no warnings -! binsubstr out 'http.ListenAndServe' 'debug.WriteHeapDump' 'time.Now' +! binsubstr out 'http.ListenAndServe' 'debug.WriteHeapDump' 'time.Now' 'syscall.Listen' # The same low-level std packages appear in plain sight in regular builds. go build -o=out_regular ./stdimporter -binsubstr out_regular 'http.ListenAndServe' 'debug.WriteHeapDump' 'time.Now' +binsubstr out_regular 'http.ListenAndServe' 'debug.WriteHeapDump' 'time.Now' 'syscall.Listen' # 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. @@ -79,6 +79,7 @@ import ( "net/http" "runtime/debug" "time" + "syscall" ) func main() { @@ -87,4 +88,5 @@ func main() { // as it is implemented by runtime via a linkname. debug.WriteHeapDump(1) time.Now() + syscall.Listen(0, 1) } diff --git a/testdata/script/implement.txtar b/testdata/script/implement.txtar index 8138e51..8f0ff4f 100644 --- a/testdata/script/implement.txtar +++ b/testdata/script/implement.txtar @@ -61,6 +61,21 @@ func main() { fmt.Println(T("foo")) fmt.Println(T("foo").unexportedMethod()) } + +-- main_linux.go -- +package main + +import "syscall" + +// golang.org/x/sys/unix has a copy of syscall.Rlimit and converts with it. +// Note that syscall.Rlimit is only declared in some unix GOOSes. +type Rlimit2 struct { + Cur uint64 + Max uint64 +} + +var _ = (*syscall.Rlimit)(new(Rlimit2)) + -- lib1/lib1.go -- package lib1