From af517a20f88bf6b89da1a074381de859f72ba7b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Tue, 23 Feb 2021 11:59:48 +0000 Subject: [PATCH] make the handling of import paths more robust First, make isPrivate panic on malformed import paths, since that should never happen. This catches the errors that some users had run into with packages like gopkg.in/yaml.v2 and github.com/satori/go.uuid: panic: malformed import path "gopkg.in/garbletest%2ev2": invalid char '%' This seems to trigger when a module path contains a dot after the first element, *and* that module is fetched via the proxy. This results in the toolchain URL-encoding the second dot, and garble ends up seeing that encoded path. We reproduce this behavior with a fake gopkg.in module added to the test module proxy. Using yaml.v2 directly would have been easier, but it's pretty large. Note that we tried a replace directive, but that does not trigger the URL-encoding bug. Also note that we do not obfuscate the gopkg.in package; that's fine, as the isPrivate path validity check catches the bug either way. For now, make initImport use url.PathUnescape to work around this issue. The underlying bug is likely in either the goobj2 fork, or in the upstream Go toolchain itself. hashImport also gives a better error if it cannot find a package now, rather than just an "empty seed" panic. Finally, the sanity check in isPrivate unearthed the fact that we do not support garbling test packages at all, since they were invalid paths which never matched GOPRIVATE. Add an explicit check and TODO about that. Fixes #224. Fixes #228. --- import_obfuscation.go | 29 +++++++++++++++++-- main.go | 12 ++++++++ .../mod/gopkg.in_garbletest.v2_v2.999.0.txt | 12 ++++++++ testdata/scripts/imports.txt | 9 +++++- testdata/scripts/syntax.txt | 4 +-- 5 files changed, 60 insertions(+), 6 deletions(-) create mode 100644 testdata/mod/gopkg.in_garbletest.v2_v2.999.0.txt diff --git a/import_obfuscation.go b/import_obfuscation.go index b6da4e6..50af9bc 100644 --- a/import_obfuscation.go +++ b/import_obfuscation.go @@ -11,6 +11,7 @@ import ( "fmt" "io" "io/ioutil" + "net/url" "os" "path/filepath" "sort" @@ -203,15 +204,27 @@ func obfuscateImports(objPath string, importMap goobj2.ImportMap) (garbledObj st } initImport := func(imp string) string { - if !isPrivate(imp) { + // Due to an apparent bug in goobj2, in some rare + // edge cases like gopkg.in/yaml.v2 we end up with + // URL-escaped paths, like "gopkg.in/yaml%2ev2". + // Unescape them to use isPrivate and hashImport. + // TODO: Investigate and fix this in goobj2. + unesc, err := url.PathUnescape(imp) + if err != nil { + panic(err) + } + + if !isPrivate(unesc) { + // If the path isn't private, leave the + // original path as-is, even if escaped. return imp } - privPaths, privNames := explodeImportPath(imp) + privPaths, privNames := explodeImportPath(unesc) privImports.privatePaths = append(privImports.privatePaths, privPaths...) privImports.privateNames = append(privImports.privateNames, privNames...) - return hashImport(imp, nil) + return hashImport(unesc, nil) } for i := range am.Imports { @@ -433,6 +446,16 @@ func hashImport(pkg string, seed []byte) string { pkgPath = buildInfo.firstImport } seed = buildInfo.imports[pkgPath].actionID + + // If we can't find a seed, provide a more helpful panic than hashWith's. + if len(seed) == 0 { + var imports []string + for path := range buildInfo.imports { + imports = append(imports, path) + } + sort.Strings(imports) + panic(fmt.Sprintf("could not find package path %s; imports:\n%s", pkgPath, strings.Join(imports, "\n"))) + } } return hashWith(seed, pkg) diff --git a/main.go b/main.go index a11e3cf..f1131c1 100644 --- a/main.go +++ b/main.go @@ -718,6 +718,18 @@ var runtimeRelated = map[string]bool{ // To allow using garble without GOPRIVATE for standalone main packages, it will // default to not matching standard library packages. func isPrivate(path string) bool { + // isPrivate is used in lots of places, so use it as a way to sanity + // check that none of our package paths are invalid. + // This can happen if we end up with an escaped or corrupted path. + // TODO: Do we want to support obfuscating test packages? + // It is a bit tricky as their import paths are confusing, such as + // "test/bar.test" and "test/bar [test/bar.test]". + if strings.HasSuffix(path, ".test") || strings.HasSuffix(path, ".test]") { + return false + } + if err := module.CheckImportPath(path); err != nil { + panic(err) + } if runtimeRelated[path] { return false } diff --git a/testdata/mod/gopkg.in_garbletest.v2_v2.999.0.txt b/testdata/mod/gopkg.in_garbletest.v2_v2.999.0.txt new file mode 100644 index 0000000..308e50a --- /dev/null +++ b/testdata/mod/gopkg.in_garbletest.v2_v2.999.0.txt @@ -0,0 +1,12 @@ +module gopkg.in/garbletest.v2@v2.999.0 + +-- .mod -- +module gopkg.in/garbletest.v2 + +go 1.15 +-- .info -- +{"Version":"v2.999.0","Time":"2020-11-17T15:46:20Z"} +-- apic.go -- +package garbletest + +func Test() { println("this is the dummy garbletest.v2 package") } diff --git a/testdata/scripts/imports.txt b/testdata/scripts/imports.txt index fe554d2..b3f2bfd 100644 --- a/testdata/scripts/imports.txt +++ b/testdata/scripts/imports.txt @@ -47,10 +47,15 @@ module test/main go 1.15 -require rsc.io/quote v1.5.2 +require ( + rsc.io/quote v1.5.2 + gopkg.in/garbletest.v2 v2.999.0 +) -- go.sum -- golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c h1:pvCbr/wm8HzDD3fVywevekufpn6tCGPY3spdHeZJEsw= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +gopkg.in/garbletest.v2 v2.999.0 h1:wiZfOKGiXX7DoYVgbNvnTaCjqElrpZQSvKg0HYouw/o= +gopkg.in/garbletest.v2 v2.999.0/go.mod h1:MF1BPTBjmDdc9x86+9UMLL9pAH2eMFPHvltohOvlGEw= rsc.io/quote v1.5.2 h1:3fEykkD9k7lYzXqCYrwGAf7iNhbk4yCjHmKBN9td4L0= rsc.io/quote v1.5.2/go.mod h1:LzX7hefJvL54yjefDEDHNONDjII0t9xZLPXsUe+TKr0= rsc.io/sampler v1.3.0 h1:HLGR/BgEtI3r0uymSP/nl2uPLsUnNJX8toRyhfpBTII= @@ -66,6 +71,7 @@ import ( "test/main/imported" "rsc.io/quote" + garbletest "gopkg.in/garbletest.v2" ) func main() { @@ -91,6 +97,7 @@ func main() { } fmt.Println(quote.Go()) + garbletest.Test() } func printfWithoutPackage(format string, v interface{}) { diff --git a/testdata/scripts/syntax.txt b/testdata/scripts/syntax.txt index 48091e4..274ad8b 100644 --- a/testdata/scripts/syntax.txt +++ b/testdata/scripts/syntax.txt @@ -1,4 +1,4 @@ -env GOPRIVATE='test/main,private.source/*' +env GOPRIVATE='test/main,private.source' garble build exec ./main$exe @@ -17,7 +17,7 @@ binsubstr main$exe 'globalVar' # 'globalType' only matches on go < 1.15 ! binsubstr main$exe 'localName' 'globalConst' 'remoteIntReturn' 'intReturn' -- extra/go.mod -- -module "private.source/extra" +module private.source/extra go 1.15 -- extra/extra.go --