fail if we are unexpectedly overwriting files (#418)

While investigating a bug report,
I noticed that garble was writing to the same temp file twice.
At best, writing to the same path on disk twice is wasteful,
as the design is careful to be deterministic and use unique paths.
At worst, the two writes could cause races at the filesystem level.

To prevent either of those situations,
we now create files with os.OpenFile and os.O_EXCL,
meaning that we will error if the file already exists.
That change uncovered a number of such unintended cases.

First, transformAsm would write obfuscated Go files twice.
This is because the Go toolchain actually runs:

	[...]/asm -gensymabis [...] foo.s bar.s
	[...]/asm [...] foo.s bar.s

That is, the first run is only meant to generate symbol ABIs,
which are then used by the compiler.
We need to obfuscate at that first stage,
because the symbol ABI descriptions need to use obfuscated names.

However, having already obfuscated the assembly on the first stage,
there is no need to do so again on the second stage.
If we detect gensymabis is missing, we simply reuse the previous files.

This first situation doesn't seem racy,
but obfuscating the Go assembly files twice is certainly unnecessary.

Second, saveKnownReflectAPIs wrote a gob file to the build cache.
Since the build cache can be kept between builds,
and since the build cache uses reproducible paths for each build,
running the same "garble build" twice could overwrite those files.

This could actually cause races at the filesystem level;
if two concurrent builds write to the same gob file on disk,
one of them could end up using a partially-written file.

Note that this is the only of the three cases not using temporary files.
As such, it is expected that the file may already exist.
In such a case, we simply avoid overwriting it rather than failing.

Third, when "garble build -a" was used,
and when we needed an export file not listed in importcfg,
we would end up calling roughly:

	go list -export -toolexec=garble -a <dependency>

This meant we would re-build and re-obfuscate those packages.
Which is unfortunate, because the parent process already did via:

	go build -toolexec=garble -a <main>

The repeated dependency builds tripped the new os.O_EXCL check,
as we would try to overwrite the same obfuscated Go files.
Beyond being wasteful, this could again cause subtle filesystem races.
To fix the problem, avoid passing flags like "-a" to nested go commands.

Overall, we should likely be using safer ways to write to disk,
be it via either atomic writes or locked files.
However, for now, catching duplicate writes is a big step.
I have left a self-assigned TODO for further improvements.

CI on the pull request found a failure on test-gotip.
The failure reproduces on master, so it seems to be related to gotip,
and not a regression introduced by this change.
For now, disable test-gotip until we can investigate.

.github/workflows/test.yml

@@ -32,6 +32,7 @@ jobs:
         go test -race ./...
 
   test-gotip:
+    if: ${{ false }} # this fails on tip: go clean -cache && go test -run Script/goprivate
     runs-on: ubuntu-latest
     continue-on-error: true # master may not be as stable
     steps:

main.go

@@ -9,6 +9,7 @@ import (
 	"encoding/binary"
 	"encoding/gob"
 	"encoding/json"
+	"errors"
 	"flag"
 	"fmt"
 	"go/ast"
@@ -17,6 +18,7 @@ import (
 	"go/token"
 	"go/types"
 	"io"
+	"io/fs"
 	"io/ioutil"
 	"log"
 	mathrand "math/rand"
@@ -170,7 +172,7 @@ func obfuscatedTypesPackage(path string) *types.Package {
 			"-trimpath",
 			"-toolexec=" + cache.ExecPath,
 		}
-		goArgs = append(goArgs, cache.BuildFlags...)
+		goArgs = append(goArgs, cache.ForwardBuildFlags...)
 		goArgs = append(goArgs, path)
 
 		cmd := exec.Command("go", goArgs...)
@@ -398,9 +400,9 @@ This command wraps "go %s". Below is its help:
 
 	// Note that we also need to pass build flags to 'go list', such
 	// as -tags.
-	cache.BuildFlags, _ = filterBuildFlags(flags)
+	cache.ForwardBuildFlags, _ = filterForwardBuildFlags(flags)
 	if command == "test" {
-		cache.BuildFlags = append(cache.BuildFlags, "-test")
+		cache.ForwardBuildFlags = append(cache.ForwardBuildFlags, "-test")
 	}
 
 	if err := fetchGoEnv(); err != nil {
@@ -472,6 +474,28 @@ func transformAsm(args []string) ([]string, error) {
 
 	flags = alterTrimpath(flags)
 
+	// If the assembler is running just for -gensymabis,
+	// don't obfuscate the source, as we are not assembling yet.
+	// The assembler will run again later; obfuscating twice is just wasteful.
+	symabis := false
+	for _, arg := range args {
+		if arg == "-gensymabis" {
+			symabis = true
+			break
+		}
+	}
+	newPaths := make([]string, 0, len(paths))
+	if !symabis {
+		var newPaths []string
+		for _, path := range paths {
+			name := filepath.Base(path)
+			pkgDir := filepath.Join(sharedTempDir, filepath.FromSlash(curPkg.ImportPath))
+			newPath := filepath.Join(pkgDir, name)
+			newPaths = append(newPaths, newPath)
+		}
+		return append(flags, newPaths...), nil
+	}
+
 	// We need to replace all function references with their obfuscated name
 	// counterparts.
 	// Luckily, all func names in Go assembly files are immediately followed
@@ -481,7 +505,6 @@ func transformAsm(args []string) ([]string, error) {
 	const middleDot = '·'
 	middleDotLen := utf8.RuneLen(middleDot)
 
-	newPaths := make([]string, 0, len(paths))
 	for _, path := range paths {
 
 		// Read the entire file into memory.
@@ -572,7 +595,7 @@ func writeTemp(name string, content []byte) (string, error) {
 		return "", err
 	}
 	dstPath := filepath.Join(pkgDir, name)
-	if err := os.WriteFile(dstPath, content, 0o666); err != nil {
+	if err := writeFileExclusive(dstPath, content); err != nil {
 		return "", err
 	}
 	return dstPath, nil
@@ -614,11 +637,17 @@ func transformCompile(args []string) ([]string, error) {
 		return nil, err
 	}
 
+	// Note that if the file already exists in the cache from another build,
+	// we don't need to write to it again thanks to the hash.
+	// TODO: as an optimization, just load that one gob file.
 	if err := loadKnownReflectAPIs(); err != nil {
 		return nil, err
 	}
 	tf.findReflectFunctions(files)
-	if err := saveKnownReflectAPIs(); err != nil {
+	if err := writeGobExclusive(
+		garbleExportFile(curPkg),
+		knownReflectAPIs,
+	); err != nil && !errors.Is(err, fs.ErrExist) {
 		return nil, err
 	}
 
@@ -990,20 +1019,6 @@ func loadKnownReflectAPIs() error {
 	return nil
 }
 
-func saveKnownReflectAPIs() error {
-	filename := garbleExportFile(curPkg)
-	f, err := os.Create(filename)
-	if err != nil {
-		return err
-	}
-	defer f.Close()
-
-	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) {
 	visitReflect := func(node ast.Node) {
 		funcDecl, ok := node.(*ast.FuncDecl)
@@ -1685,16 +1700,22 @@ func alterTrimpath(flags []string) []string {
 	return flagSetValue(flags, "-trimpath", sharedTempDir+"=>;"+trimpath)
 }
 
-// buildFlags is obtained from 'go help build' as of Go 1.17.
-var buildFlags = map[string]bool{
-	"-a":             true,
-	"-n":             true,
+// forwardBuildFlags is obtained from 'go help build' as of Go 1.17.
+var forwardBuildFlags = map[string]bool{
+	// These shouldn't be used in nested cmd/go calls.
+	"-a": false,
+	"-n": false,
+	"-x": false,
+	"-v": false,
+
+	// These are always set by garble.
+	"-trimpath": false,
+	"-toolexec": false,
+
 	"-p":             true,
 	"-race":          true,
 	"-msan":          true,
-	"-v":             true,
 	"-work":          true,
-	"-x":             true,
 	"-asmflags":      true,
 	"-buildmode":     true,
 	"-compiler":      true,
@@ -1708,8 +1729,6 @@ var buildFlags = map[string]bool{
 	"-modfile":       true,
 	"-pkgdir":        true,
 	"-tags":          true,
-	"-trimpath":      true,
-	"-toolexec":      true,
 	"-overlay":       true,
 }
 
@@ -1736,15 +1755,15 @@ var booleanFlags = map[string]bool{
 	"-benchmem": true,
 }
 
-func filterBuildFlags(flags []string) (filtered []string, firstUnknown string) {
+func filterForwardBuildFlags(flags []string) (filtered []string, firstUnknown string) {
 	for i := 0; i < len(flags); i++ {
 		arg := flags[i]
 		name := arg
 		if i := strings.IndexByte(arg, '='); i > 0 {
 			name = arg[:i]
 		}
 
-		buildFlag := buildFlags[name]
+		buildFlag := forwardBuildFlags[name]
 		if buildFlag {
 			filtered = append(filtered, arg)
 		} else {

main_test.go

@@ -276,7 +276,7 @@ func TestSplitFlagsFromArgs(t *testing.T) {
 	}
 }
 
-func TestFilterBuildFlags(t *testing.T) {
+func TestFilterForwardBuildFlags(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		name  string
@@ -304,10 +304,10 @@ func TestFilterBuildFlags(t *testing.T) {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
-			got, _ := filterBuildFlags(test.flags)
+			got, _ := filterForwardBuildFlags(test.flags)
 
 			if diff := cmp.Diff(test.want, got); diff != "" {
-				t.Fatalf("filterBuildFlags(%q) mismatch (-want +got):\n%s", test.flags, diff)
+				t.Fatalf("filterForwardBuildFlags(%q) mismatch (-want +got):\n%s", test.flags, diff)
 			}
 		})
 	}

reverse.go

@@ -51,7 +51,7 @@ One can reverse a captured panic stack trace as follows:
 	// We don't actually run a main Go command with all flags,
 	// so if the user gave a non-build flag,
 	// we need this check to not silently ignore it.
-	if _, firstUnknown := filterBuildFlags(flags); firstUnknown != "" {
+	if _, firstUnknown := filterForwardBuildFlags(flags); firstUnknown != "" {
 		// A bit of a hack to get a normal flag.Parse error.
 		// Longer term, "reverse" might have its own FlagSet.
 		return flag.NewFlagSet("", flag.ContinueOnError).Parse([]string{firstUnknown})

shared.go

@@ -6,7 +6,9 @@ import (
 	"encoding/base64"
 	"encoding/gob"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -20,8 +22,8 @@ import (
 // store it into a temporary file via gob encoding, and then reuse that file
 // in each of the garble toolexec sub-processes.
 type sharedCache struct {
-	ExecPath   string   // absolute path to the garble binary being used
-	BuildFlags []string // build flags fed to the original "garble ..." command
+	ExecPath          string   // absolute path to the garble binary being used
+	ForwardBuildFlags []string // build flags fed to the original "garble ..." command
 
 	Options flagOptions // garble options being used, i.e. our own flags
 
@@ -76,16 +78,43 @@ func saveSharedCache() (string, error) {
 	}
 
 	sharedCache := filepath.Join(dir, "main-cache.gob")
-	f, err := os.Create(sharedCache)
-	if err != nil {
+	if err := writeGobExclusive(sharedCache, &cache); err != nil {
 		return "", err
 	}
-	defer f.Close()
+	return dir, nil
+}
 
-	if err := gob.NewEncoder(f).Encode(&cache); err != nil {
-		return "", err
+func createExclusive(name string) (*os.File, error) {
+	return os.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o666)
+}
+
+// TODO(mvdan): consider using proper atomic file writes.
+// Or possibly even "lockedfile", mimicking cmd/go.
+
+func writeFileExclusive(name string, data []byte) error {
+	f, err := createExclusive(name)
+	if err != nil {
+		return err
 	}
-	return dir, nil
+	_, err = f.Write(data)
+	if err2 := f.Close(); err == nil {
+		err = err2
+	}
+	return err
+}
+
+func writeGobExclusive(name string, val interface{}) error {
+	f, err := createExclusive(name)
+	if err != nil {
+		return err
+	}
+	if err := gob.NewEncoder(f).Encode(val); err != nil {
+		return err
+	}
+	if err2 := f.Close(); err == nil {
+		err = err2
+	}
+	return err
 }
 
 // flagOptions are derived from the flags
@@ -140,7 +169,7 @@ func setFlagOptions() error {
 			flagDebugDir = filepath.Join(wd, flagDebugDir)
 		}
 
-		if err := os.RemoveAll(flagDebugDir); err == nil || os.IsNotExist(err) {
+		if err := os.RemoveAll(flagDebugDir); err == nil || errors.Is(err, fs.ErrExist) {
 			err := os.MkdirAll(flagDebugDir, 0o755)
 			if err != nil {
 				return err
@@ -193,7 +222,7 @@ func (p *listedPackage) obfuscatedImportPath() string {
 // and all of its dependencies
 func setListedPackages(patterns []string) error {
 	args := []string{"list", "-json", "-deps", "-export", "-trimpath"}
-	args = append(args, cache.BuildFlags...)
+	args = append(args, cache.ForwardBuildFlags...)
 	args = append(args, patterns...)
 	cmd := exec.Command("go", args...)