avoid reproducibility issues with full rebuilds

We were using temporary filenames for modified Go and assembly files.
For example, an obfuscated "encoding/json/encode.go" would end up as:

	/tmp/garble-shared123/encode.go.456.go

where "123" and "456" are random numbers, usually longer.

This was usually fine for two reasons:

1) We would add "/tmp/garble-shared123/" to -trimpath, so the temporary
   directory and its random number would be invisible.

2) We would add "//line" directives to the source files, replacing
   the filename with obfuscated versions excluding any random number.

Unfortunately, this broke in multiple ways. Most notably, assembly files
do not have any line directives, and it's not clear that there's any
support for them. So the random number in their basename could end up in
the binary, breaking reproducibility.

Another issue is that the -trimpath addition described above was only
done for cmd/compile, not cmd/asm, so assembly filenames included the
randomized temporary directory.

To fix the issues above, the same "encoding/json/encode.go" would now
end up as:

	/tmp/garble-shared123/encoding/json/encode.go

Such a path is still unique even though the "456" random number is gone,
as import paths are unique within a single build.

This fixes issues with the base name of each file, so we no longer rely
on line directives as the only way to remove the second original random
number.

We still rely on -trimpath to get rid of the temporary directory in
filenames. To fix its problem with assembly files, also amend the
-trimpath flag when running the assembler tool.

Finally, add a test that reproducible builds still work when a full
rebuild is done. We choose goprivate.txt for such a test as its
stdimporter package imports a number of std packages, including uses of
assembly and cgo.

For the time being, we don't use such a "full rebuild" reproducibility
test in other test scripts, as this step is expensive, rebuilding many
packages from scratch.

This issue went unnoticed for over a year because such random numbers
"123" and "456" were created when a package was obfuscated, and that
only happened once per package version as long as the build cache was
kept intact.

When clearing the build cache, or forcing a rebuild with -a, one gets
new random numbers, and thus a different binary resulting from the same
build input. That's not something that most users would do regularly,
and our tests did not cover that edge case either, until now.

Fixes #328.

main.go

@@ -441,6 +441,12 @@ func transformAsm(args []string) ([]string, error) {
 		flags = flagSetValue(flags, "-p", curPkg.obfuscatedImportPath())
 	}
 
+	var err error
+	flags, err = alterTrimpath(flags)
+	if err != nil {
+		return nil, err
+	}
+
 	// We need to replace all function references with their obfuscated name
 	// counterparts.
 	// Luckily, all func names in Go assembly files are immediately followed
@@ -514,8 +520,8 @@ func transformAsm(args []string) ([]string, error) {
 			buf.WriteString(newName)
 		}
 
-		// TODO: do the original asm filenames ever matter?
-		if path, err := writeTemp("*.s", buf.Bytes()); err != nil {
+		name := filepath.Base(path)
+		if path, err := writeTemp(name, buf.Bytes()); err != nil {
 			return nil, err
 		} else {
 			newPaths = append(newPaths, path)
@@ -526,24 +532,20 @@ func transformAsm(args []string) ([]string, error) {
 }
 
 // writeTemp is a mix between os.CreateTemp and os.WriteFile, as it writes a
-// temporary file in sharedTempDir given an input buffer.
+// named source file in sharedTempDir given an input buffer.
 //
-// This helper func also makes the "defer" more truthful, as it's often used
-// within a loop.
+// Note that the file is created under a directory tree following curPkg's
+// import path, mimicking how files are laid out in modules and GOROOT.
 func writeTemp(name string, content []byte) (string, error) {
-	tempFile, err := os.CreateTemp(sharedTempDir, name)
-	if err != nil {
-		return "", err
-	}
-	defer tempFile.Close()
-
-	if _, err := tempFile.Write(content); err != nil {
+	pkgDir := filepath.Join(sharedTempDir, filepath.FromSlash(curPkg.ImportPath))
+	if err := os.MkdirAll(pkgDir, 0o777); err != nil {
 		return "", err
 	}
-	if err := tempFile.Close(); err != nil {
+	dstPath := filepath.Join(pkgDir, name)
+	if err := os.WriteFile(dstPath, content, 0o666); err != nil {
 		return "", err
 	}
-	return tempFile.Name(), nil
+	return dstPath, nil
 }
 
 func transformCompile(args []string) ([]string, error) {
@@ -572,11 +574,9 @@ func transformCompile(args []string) ([]string, error) {
 		}
 	}
 
-	// If the value of -trimpath doesn't contain the separator ';', the 'go
-	// build' command is most likely not using '-trimpath'.
-	trimpath := flagValue(flags, "-trimpath")
-	if !strings.Contains(trimpath, ";") {
-		return nil, fmt.Errorf("-toolexec=garble should be used alongside -trimpath")
+	flags, err = alterTrimpath(flags)
+	if err != nil {
+		return nil, err
 	}
 
 	newImportCfg, err := processImportCfg(flags)
@@ -607,11 +607,6 @@ func transformCompile(args []string) ([]string, error) {
 	}
 
 	tf.recordReflectArgs(files)
-
-	// Add our temporary dir to the beginning of -trimpath, so that we don't
-	// leak temporary dirs. Needs to be at the beginning, since there may be
-	// shorter prefixes later in the list, such as $PWD if TMPDIR=$PWD/tmp.
-	flags = flagSetValue(flags, "-trimpath", sharedTempDir+"=>;"+trimpath)
 	// log.Println(flags)
 
 	// If this is a package to obfuscate, swap the -p flag with the new
@@ -627,12 +622,11 @@ func transformCompile(args []string) ([]string, error) {
 	for i, file := range files {
 		tf.handleDirectives(file.Comments)
 
-		origName := filepath.Base(paths[i])
-		name := origName
+		name := filepath.Base(paths[i])
 		switch {
 		case curPkg.ImportPath == "runtime":
 			// strip unneeded runtime code
-			stripRuntime(origName, file)
+			stripRuntime(name, file)
 		case curPkg.ImportPath == "runtime/internal/sys":
 			// The first declaration in zversion.go contains the Go
 			// version as follows. Replace it here, since the
@@ -641,17 +635,14 @@ func transformCompile(args []string) ([]string, error) {
 			//     const TheVersion = `devel ...`
 			//
 			// Don't touch the source in any other way.
-			if origName != "zversion.go" {
+			if name != "zversion.go" {
 				break
 			}
 			spec := file.Decls[0].(*ast.GenDecl).Specs[0].(*ast.ValueSpec)
 			lit := spec.Values[0].(*ast.BasicLit)
 			lit.Value = "`unknown`"
-		case strings.HasPrefix(origName, "_cgo_"):
-			// Cgo generated code requires a prefix. Also, don't
-			// obfuscate it, since it's just generated code and it gets
-			// messy.
-			name = "_cgo_" + name
+		case strings.HasPrefix(name, "_cgo_"):
+			// Don't obfuscate cgo code, since it's generated and it gets messy.
 		default:
 			file = tf.transformGo(file)
 		}
@@ -666,10 +657,10 @@ func transformCompile(args []string) ([]string, error) {
 
 		// Uncomment for some quick debugging. Do not delete.
 		// if curPkg.Private {
-		// 	fmt.Fprintf(os.Stderr, "\n-- %s/%s --\n%s", curPkg.ImportPath, origName, src)
+		// 	fmt.Fprintf(os.Stderr, "\n-- %s/%s --\n%s", curPkg.ImportPath, name, src)
 		// }
 
-		if path, err := writeTemp(name+".*.go", src); err != nil {
+		if path, err := writeTemp(name, src); err != nil {
 			return nil, err
 		} else {
 			newPaths = append(newPaths, path)
@@ -681,7 +672,7 @@ func transformCompile(args []string) ([]string, error) {
 				return nil, err
 			}
 
-			debugFilePath := filepath.Join(pkgDebugDir, origName)
+			debugFilePath := filepath.Join(pkgDebugDir, name)
 			if err := os.WriteFile(debugFilePath, src, 0666); err != nil {
 				return nil, err
 			}
@@ -1430,6 +1421,21 @@ func splitFlagsFromArgs(all []string) (flags, args []string) {
 	return all, nil
 }
 
+func alterTrimpath(flags []string) ([]string, error) {
+	// If the value of -trimpath doesn't contain the separator ';', the 'go
+	// build' command is most likely not using '-trimpath'.
+	trimpath := flagValue(flags, "-trimpath")
+	if !strings.Contains(trimpath, ";") {
+		return nil, fmt.Errorf("-toolexec=garble should be used alongside -trimpath")
+	}
+
+	// Add our temporary dir to the beginning of -trimpath, so that we don't
+	// leak temporary dirs. Needs to be at the beginning, since there may be
+	// shorter prefixes later in the list, such as $PWD if TMPDIR=$PWD/tmp.
+	flags = flagSetValue(flags, "-trimpath", sharedTempDir+"=>;"+trimpath)
+	return flags, nil
+}
+
 // buildFlags is obtained from 'go help build' as of Go 1.15.
 var buildFlags = map[string]bool{
 	"-a":             true,

testdata/scripts/goprivate.txt

@@ -23,6 +23,12 @@ garble build std
 # support ImportMap when linking.
 garble build -o=out ./stdimporter
 
+# Also check that a full rebuild is reproducible,
+# with -a to rebuild all packages.
+# This is slow, but necessary to uncover bugs hidden by the build cache.
+garble build -o=out_rebuild -a ./stdimporter
+bincmp out_rebuild out
+
 -- go.mod --
 module test/main