From 249501b5e9cceba54e98793ec7d852d79ec47fb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Wed, 13 Jan 2021 00:47:17 +0000 Subject: [PATCH] fix garbling names belonging to indirect imports (#203) main.go includes a lengthy comment that documents this edge case, why it happened, and how we are fixing it. To summarize, we should no longer error with a build error in those cases. Read the comment for details. A few other minor changes were done to allow writing this patch. First, the actionID and contentID funcs were renamed, since they started to collide with variable names. Second, the logging has been improved a bit, which allowed me to debug the issue. Third, the "cache" global shared by all garble sub-processes now includes the necessary parameters to run "go list -toolexec", including the path to garble and the build flags being used. Thanks to lu4p for writing a test case, which also applied gofmt to that testdata Go file. Fixes #180. Closes #181, since it includes its test case. --- hash.go | 14 +++--- main.go | 87 +++++++++++++++++++++++++++++++----- shared.go | 11 +++-- testdata/scripts/imports.txt | 30 +++++++++---- 4 files changed, 110 insertions(+), 32 deletions(-) diff --git a/hash.go b/hash.go index db5a86b..e6312c0 100644 --- a/hash.go +++ b/hash.go @@ -18,8 +18,8 @@ import ( const buildIDSeparator = "/" -// actionID returns the action ID half of a build ID, the first element. -func actionID(buildID string) string { +// splitActionID returns the action ID half of a build ID, the first element. +func splitActionID(buildID string) string { i := strings.Index(buildID, buildIDSeparator) if i < 0 { return buildID @@ -27,12 +27,12 @@ func actionID(buildID string) string { return buildID[:i] } -// contentID returns the content ID half of a build ID, the last element. -func contentID(buildID string) string { +// splitContentID returns the content ID half of a build ID, the last element. +func splitContentID(buildID string) string { return buildID[strings.LastIndex(buildID, buildIDSeparator)+1:] } -// decodeHash isthe opposite of hashToString, but with a panic for error +// decodeHash is the opposite of hashToString, but with a panic for error // handling since it should never happen. func decodeHash(str string) []byte { h, err := base64.RawURLEncoding.DecodeString(str) @@ -59,7 +59,7 @@ func alterToolVersion(tool string, args []string) error { var toolID []byte if f[2] == "devel" { // On the development branch, use the content ID part of the build ID. - toolID = decodeHash(contentID(f[len(f)-1])) + toolID = decodeHash(splitContentID(f[len(f)-1])) } else { // For a release, the output is like: "compile version go1.9.1 X:framepointer". // Use the whole line. @@ -96,7 +96,7 @@ func ownContentID(toolID []byte) (string, error) { if err != nil { return "", err } - ownID := decodeHash(contentID(buildID)) + ownID := decodeHash(splitContentID(buildID)) // Join the two content IDs together into a single base64-encoded sha256 // sum. This includes the original tool's content ID, and garble's own diff --git a/main.go b/main.go index 9df81d7..79de186 100644 --- a/main.go +++ b/main.go @@ -253,33 +253,33 @@ func mainErr(args []string) error { // Note that we also need to pass build flags to 'go list', such // as -tags. - listFlags := filterBuildFlags(flags) + cache.BuildFlags = filterBuildFlags(flags) if cmd == "test" { - listFlags = append(listFlags, "-test") + cache.BuildFlags = append(cache.BuildFlags, "-test") } if err := setGoPrivate(); err != nil { return err } - if err := setListedPackages(listFlags, args); err != nil { + if err := setListedPackages(args); err != nil { return err } - - sharedName, err := saveShared() + cache.ExecPath, err = os.Executable() if err != nil { return err } - defer os.Remove(sharedName) - execPath, err := os.Executable() + sharedName, err := saveShared() if err != nil { return err } + defer os.Remove(sharedName) + goArgs := []string{ cmd, "-trimpath", - "-toolexec=" + execPath, + "-toolexec=" + cache.ExecPath, } if cmd == "test" { // vet is generally not useful on garbled code; keep it @@ -694,7 +694,7 @@ func fillBuildInfo(flags []string) error { case "", "true": return fmt.Errorf("could not find -buildid argument") } - curActionID = decodeHash(actionID(buildID)) + curActionID = decodeHash(splitActionID(buildID)) curImportCfg = flagValue(flags, "-importcfg") if curImportCfg == "" { return fmt.Errorf("could not find -importcfg argument") @@ -742,7 +742,7 @@ func fillBuildInfo(flags []string) error { } impPkg := importedPkg{ packagefile: objectPath, - actionID: decodeHash(actionID(buildID)), + actionID: decodeHash(splitActionID(buildID)), } buildInfo.imports[importPath] = impPkg @@ -992,7 +992,66 @@ func (tf *transformer) transformGo(file *ast.File) *ast.File { default: return true // we only want to rename the above } + + // Handle the case where the name is defined in an indirectly + // imported package. Since only direct imports show up in our + // importcfg, buildInfo.imports will not initially contain the + // package path we want. + // + // This edge case can happen, for example, if package A imports + // package B and calls its API, and B's API returns C's struct. + // Suddenly, A can use struct field names defined in C, even + // though A never directly imports C. + // + // For this rare case, for now, do an extra "go list -toolexec" + // call to retrieve its export path. + // TODO: Think about ways to avoid this extra exec call. Perhaps + // add an extra archive header to record all direct and indirect + // importcfg data, like we do with private name maps. + if _, e := buildInfo.imports[path]; !e && path != curPkgPath { + goArgs := []string{ + "list", + "-json", + "-export", + "-trimpath", + "-toolexec=" + cache.ExecPath, + } + goArgs = append(goArgs, cache.BuildFlags...) + goArgs = append(goArgs, path) + + cmd := exec.Command("go", goArgs...) + cmd.Dir = opts.GarbleDir + out, err := cmd.Output() + if err != nil { + if err := err.(*exec.ExitError); err != nil { + panic(fmt.Sprintf("%v: %s", err, err.Stderr)) + } + panic(err) + } + var pkg listedPackage + if err := json.Unmarshal(out, &pkg); err != nil { + panic(err) // shouldn't happen + } + buildID, err := buildidOf(pkg.Export) + if err != nil { + panic(err) // shouldn't happen + } + // Adding it to buildInfo.imports allows us to reuse the + // "if" branch below. Plus, if this edge case triggers + // multiple times in a single package compile, we can + // call "go list" once and cache its result. + buildInfo.imports[path] = importedPkg{ + packagefile: pkg.Export, + actionID: decodeHash(splitActionID(buildID)), + } + // log.Printf("fetched indirect dependency %q from: %s", path, pkg.Export) + } + actionID := curActionID + // TODO: Make this check less prone to bugs, like the one we had + // with indirect dependencies. If "path" is not our current + // package, then it must exist in buildInfo.imports. Otherwise + // we should panic. if id := buildInfo.imports[path].actionID; len(id) > 0 { garbledPkg, err := garbledImport(path) if err != nil { @@ -1007,16 +1066,21 @@ func (tf *transformer) transformGo(file *ast.File) *ast.File { actionID = id } + origName := node.Name + _ = origName // used for debug prints below + // The exported names cannot be shortened as counter synchronization // between packages is not currently implemented if token.IsExported(node.Name) { node.Name = hashWith(actionID, node.Name) + // log.Printf("%q hashed with %x to %q", origName, actionID, node.Name) return true } fullName := tf.pkg.Path() + "." + node.Name if name, ok := tf.privateNameMap[fullName]; ok { node.Name = name + // log.Printf("%q retrieved private name %q for %q", origName, name, path) return true } @@ -1029,10 +1093,9 @@ func (tf *transformer) transformGo(file *ast.File) *ast.File { } } - // orig := node.Name tf.privateNameMap[fullName] = name node.Name = name - // log.Printf("%q hashed with %q to %q", orig, actionID, node.Name) + // log.Printf("%q assigned private name %q for %q", origName, name, path) return true } return astutil.Apply(file, pre, nil).(*ast.File) diff --git a/shared.go b/shared.go index 6e50550..d8d4b97 100644 --- a/shared.go +++ b/shared.go @@ -16,8 +16,11 @@ import ( // shared this data is shared between the different garble processes type shared struct { - Options options - ListedPackages listedPackages + ExecPath string // absolute path to the garble binary being used + BuildFlags []string // build flags fed to the original "garble ..." command + + Options options // garble options being used, i.e. our own flags + ListedPackages listedPackages // non-garbled view of all packages to build } var cache *shared @@ -146,9 +149,9 @@ type listedPackage struct { // setListedPackages gets information about the current package // and all of its dependencies -func setListedPackages(flags, patterns []string) error { +func setListedPackages(patterns []string) error { args := []string{"list", "-json", "-deps", "-export"} - args = append(args, flags...) + args = append(args, cache.BuildFlags...) args = append(args, patterns...) cmd := exec.Command("go", args...) diff --git a/testdata/scripts/imports.txt b/testdata/scripts/imports.txt index f8bdd97..fe554d2 100644 --- a/testdata/scripts/imports.txt +++ b/testdata/scripts/imports.txt @@ -60,8 +60,8 @@ package main import ( "fmt" - "strings" "reflect" + "strings" "test/main/imported" @@ -75,8 +75,9 @@ func main() { fmt.Println(imported.ImportedType(3)) fmt.Println(imported.ReflectInDefinedVar.ExportedField2) fmt.Println(imported.ReflectInDefined{ExportedField2: 5}) - fmt.Println(imported.NormalStruct{SharedName: 3}) - + normal := imported.NormalStruct{SharedName: 3} + normal.IndirectStruct.Field = 23 + fmt.Println(normal) printfWithoutPackage("%T\n", imported.ReflectTypeOf(2)) printfWithoutPackage("%T\n", imported.ReflectTypeOfIndirect(4)) @@ -112,7 +113,10 @@ func init() { fmt.Println("buildtag init func") } -- imported/imported.go -- package imported -import "reflect" +import ( + "reflect" + "test/main/imported/indirect" +) var ImportedVar = "imported var value" @@ -131,19 +135,19 @@ type ReflectTypeOfIndirect int var _ = reflect.TypeOf(new([]*ReflectTypeOfIndirect)) type ReflectValueOf struct { - ExportedField string + ExportedField string unexportedField string } -func (r *ReflectValueOf) ExportedMethodName() string { return "method: "+r.ExportedField } +func (r *ReflectValueOf) ExportedMethodName() string { return "method: " + r.ExportedField } var ReflectValueOfVar = ReflectValueOf{ExportedField: "abc"} var _ = reflect.TypeOf(ReflectValueOfVar) type ReflectInDefined struct { - ExportedField2 int + ExportedField2 int unexportedField2 int } @@ -155,13 +159,21 @@ var _ = reflect.TypeOf(ReflectInDefinedVar) const SharedName = 2 type NormalStruct struct { - SharedName int + SharedName int + IndirectStruct indirect.Indirect normalUnexportedField int } // ImportedType comes after the calls to reflect, to ensure no false positives. type ImportedType int +-- imported/indirect/indirect.go -- +package indirect + +type Indirect struct { + Field int +} + -- main.stdout -- buildtag init func imported var value @@ -170,7 +182,7 @@ x 3 9000 {5 0} -{3 0} +{3 {23} 0} ReflectTypeOf ReflectTypeOfIndirect ReflectValueOf{ExportedField:"abc", unexportedField:""}