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

@@ -18,8 +18,8 @@ import (
 
 const buildIDSeparator = "/"
 
-// actionID returns the action ID half of a build ID, the first element.
+// splitActionID returns the action ID half of a build ID, the first element.
-func actionID(buildID string) string {
+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.
+// splitContentID returns the content ID half of a build ID, the last element.
-func contentID(buildID string) string {
+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

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
 		}
-
+		cache.ExecPath, err = os.Executable()
-		sharedName, err := saveShared()
 		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)

shared.go

@@ -16,8 +16,11 @@ import (
 
 // shared this data is shared between the different garble processes
 type shared struct {
-	Options        options
+	ExecPath   string   // absolute path to the garble binary being used
-	ListedPackages listedPackages
+	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...)
 

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:""}