From 08ec70e9a9622370592b0a68b2f8fd1519a2e415 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Sun, 19 Sep 2021 17:57:32 +0100
Subject: [PATCH] avoid a filesystem race with build cache entries

After the last commit, I started seeing seemingly random test failures:

    --- FAIL: TestScripts/cgo (1.17s)
        testscript.go:397:
            > env GOPRIVATE=test/main
            > garble build
            [stderr]
            # runtime/internal/math
            EOF
            # internal/bytealg
            EOF
            exit status 2
            [exit status 1]
            FAIL: testdata/scripts/cgo.txt:3: unexpected command failure

    --- FAIL: TestScripts/reflect (8.63s)
        testscript.go:397:
            > env GOPRIVATE=test/main
            > garble build
            [stderr]
            # math
            EOF
            exit status 2
            [exit status 1]
            FAIL: testdata/scripts/reflect.txt:3: unexpected command failure

    --- FAIL: TestScripts/linkname (8.72s)
        testscript.go:397:
            > env GOPRIVATE=test/main
            > garble build
            [stderr]
            # math
            EOF
            exit status 2
            [exit status 1]
            FAIL: testdata/scripts/linkname.txt:3: unexpected command failure

After some investigation,
it turned out that concurrent "garble build" processes
were writing to the same build cache paths at the same time.
This is effectively a filesystem race,
and encoding/gob could error when reading partly written files.

To fix this problem,
use a cache path that changes according to the obfuscated build.
See garbleExportFile.

Note that the data we store in that file does not vary with obfuscation.
We could fix the filesystem race by adding locking around the old path.
However, we'll want to cache data that does vary with garble flags,
such as the -debugdir source code.
---
 main.go | 47 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/main.go b/main.go
index 82dce54..3ee0970 100644
--- a/main.go
+++ b/main.go
@@ -658,11 +658,11 @@ func transformCompile(args []string) ([]string, error) {
 		return nil, err
 	}
 
-	if err = loadKnownReflectAPIs(curPkg); err != nil {
+	if err := loadKnownReflectAPIs(); err != nil {
 		return nil, err
 	}
 	tf.findReflectFunctions(files)
-	if err := saveKnownReflectAPIs(curPkg); err != nil {
+	if err := saveKnownReflectAPIs(); err != nil {
 		return nil, err
 	}
 
@@ -1036,16 +1036,37 @@ var knownReflectAPIs = map[funcFullName][]reflectParameterPosition{
 	"reflect.ValueOf": {0},
 }
 
-func loadKnownReflectAPIs(currPkg *listedPackage) error {
-	for path := range importCfgEntries {
+// garbleExportFile returns an absolute path to a build cache entry
+// which belongs to garble and corresponds to the given Go package.
+//
+// Unlike pkg.Export, it is only read and written by garble itself.
+// Also unlike pkg.Export, it includes GarbleActionID,
+// so its path will change if the obfuscated build changes.
+//
+// The purpose of such a file is to store garble-specific information
+// in the build cache, to be reused at a later time.
+// The file should have the same lifetime as pkg.Export,
+// as it lives under the same cache directory that gets trimmed automatically.
+func garbleExportFile(pkg *listedPackage) string {
+	trimmed := strings.TrimSuffix(pkg.Export, "-d")
+	if trimmed == pkg.Export {
+		panic(fmt.Sprintf("unexpected export path of %s: %q", pkg.ImportPath, pkg.Export))
+	}
+	return trimmed + "-garble-" + hashToString(pkg.GarbleActionID) + "-d"
+}
+
+func loadKnownReflectAPIs() error {
+	for _, path := range curPkg.Deps {
 		pkg, err := listPackage(path)
 		if err != nil {
 			return err
 		}
-
+		if pkg.Export == "" {
+			continue // nothing to load
+		}
 		// this function literal is used for the deferred close
 		err = func() error {
-			filename := strings.TrimSuffix(pkg.Export, "-d") + "-garble-d"
+			filename := garbleExportFile(pkg)
 			f, err := os.Open(filename)
 			if err != nil {
 				return err
@@ -1053,7 +1074,10 @@ func loadKnownReflectAPIs(currPkg *listedPackage) error {
 			defer f.Close()
 
 			// Decode appends new entries to the existing map
-			return gob.NewDecoder(f).Decode(&knownReflectAPIs)
+			if err := gob.NewDecoder(f).Decode(&knownReflectAPIs); err != nil {
+				return fmt.Errorf("gob decode: %w", err)
+			}
+			return nil
 		}()
 		if err != nil {
 			return err
@@ -1063,15 +1087,18 @@ func loadKnownReflectAPIs(currPkg *listedPackage) error {
 	return nil
 }
 
-func saveKnownReflectAPIs(currPkg *listedPackage) error {
-	filename := strings.TrimSuffix(currPkg.Export, "-d") + "-garble-d"
+func saveKnownReflectAPIs() error {
+	filename := garbleExportFile(curPkg)
 	f, err := os.Create(filename)
 	if err != nil {
 		return err
 	}
 	defer f.Close()
 
-	return gob.NewEncoder(f).Encode(knownReflectAPIs)
+	if err := gob.NewEncoder(f).Encode(knownReflectAPIs); err != nil {
+		return fmt.Errorf("gob encode: %w", err)
+	}
+	return f.Close()
 }
 
 func (tf *transformer) findReflectFunctions(files []*ast.File) {