From 63c42c3cc74644c0d0ae8191bcb0c66a32bd58b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Fri, 26 Feb 2021 10:51:20 +0000 Subject: [PATCH] support typechecking all of std (#236) There was one bug keeping the command below from working: GOPRIVATE='*' garble build std The bug is rather obscure; I'm still working on a minimal reproducer that I can submit upstream, and I'm not yet convinced about where the bug lives and how it can be fixed. In short, the command would fail with: typecheck error: /go/src/crypto/ecdsa/ecdsa.go:122:12: cannot use asn1.SEQUENCE (constant 48 of type asn1.Tag) as asn1.Tag value in argument to b.AddASN1 Note that the error is ambiguous; there are two asn1 packages, but they are actually mismatching. We can see that by manually adding debug prints to go/types: constant: asn1.SEQUENCE (constant 48 of type golang.org/x/crypto/cryptobyte/asn1.Tag) argument type: vendor/golang.org/x/crypto/cryptobyte/asn1.Tag It's clear that, for some reason, go/types ends up confused and loading a vendored and non-vendored version of asn1. There also seems to be no way to work around this with our lookup function, as it just receives an import path as a parameter, and returns an object file reader. For now, work around the issue by *not* using a custom lookup function in this rare edge case involving vendored dependencies in std packages. The added code has a lengthy comment explaining the reasoning. I still intend to investigate this further, but there's no reason to keep garble failing if we can work around the bug. Fixes #223. --- main.go | 24 ++++++++++++++++++++++++ shared.go | 14 +++++++++----- testdata/scripts/goprivate.txt | 18 +++++------------- 3 files changed, 38 insertions(+), 18 deletions(-) diff --git a/main.go b/main.go index fc4bcc7..bf0b707 100644 --- a/main.go +++ b/main.go @@ -456,6 +456,30 @@ func transformCompile(args []string) ([]string, func() error, error) { }, } + standardLibrary := false + // Note that flagValue only supports "-foo=true" bool flags, but the std + // flag is generally just "-std". + // TODO: Better support boolean flags for the tools. + for _, flag := range flags { + if flag == "-std" { + standardLibrary = true + } + } + + // The standard library vendors external packages, which results in them + // listing "golang.org/x/foo" in go list -json's Deps, plus an ImportMap + // entry to remap them to "vendor/golang.org/x/foo". + // We support that edge case in listPackage, presumably, though it seems + // like importer.ForCompiler with a lookup function isn't capable of it. + // It does work without an explicit lookup func though, which results in + // extra calls to 'go list'. + // Since this is a rare edge case and only occurs for a few std + // packages, do the extra 'go list' calls for now. + // TODO(mvdan): report this upstream and investigate further. + if standardLibrary && len(cache.ListedPackages[curPkgPath].ImportMap) > 0 { + origImporter = importer.Default() + } + origTypesConfig := types.Config{Importer: origImporter} tf.pkg, err = origTypesConfig.Check(curPkgPath, fset, files, tf.info) if err != nil { diff --git a/shared.go b/shared.go index 42304c8..b352d75 100644 --- a/shared.go +++ b/shared.go @@ -214,13 +214,17 @@ func setListedPackages(patterns []string) error { // listPackage gets the listedPackage information for a certain package func listPackage(path string) (*listedPackage, error) { + // 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. + if fromPkg, ok := cache.ListedPackages[curPkgPath]; ok { + if path2 := fromPkg.ImportMap[path]; path2 != "" { + path = path2 + } + } + pkg, ok := cache.ListedPackages[path] if !ok { - if fromPkg, ok := cache.ListedPackages[curPkgPath]; ok { - if path2 := fromPkg.ImportMap[path]; path2 != "" { - return listPackage(path2) - } - } return nil, fmt.Errorf("path not found in listed packages: %s", path) } return pkg, nil diff --git a/testdata/scripts/goprivate.txt b/testdata/scripts/goprivate.txt index fba670e..24a319a 100644 --- a/testdata/scripts/goprivate.txt +++ b/testdata/scripts/goprivate.txt @@ -11,20 +11,12 @@ stderr '^public package "test/main/importer" can''t depend on obfuscated package # Try garbling all of std, given some std packages. # No need for a main package here; building the std packages directly works the # same, and is faster as we don't need to link a binary. +# This used to cause multiple errors, mainly since std vendors some external +# packages so we must properly support ImportMap. +# Plus, some packages like net make heavy use of complex features like Cgo. +# Note that we won't obfuscate a few std packages just yet, mainly those around runtime. env GOPRIVATE='*' - -# This used to cause errors since the "net" import causes 'go list -json' -# to output ImportMap, since "net" imports packages vendored in std. -# Another quirk of "net" is that it makes rather heavy use of cgo, which was -# hitting some edge cases we did not handle. -garble build net - -# This used to cause incorrect errors since we would not obfuscate -# runtime/pprof, but we would try to obfuscate its dependencies. For now, this -# simply errors because we obfuscate nothing, since we can't obfuscate the -# runtime package just yet. -! garble build runtime/pprof -stderr 'does not match any packages to be built' +garble build std -- go.mod -- module test/main