Browse Source

actually remove temporary directories after obfuscation

Back in February 2021, we changed the obfuscation logic so that the
entire `garble build` process would use one shared temporary directory
across all package builds, reducing the amount of files we created in
the top-level system temporary directory.

However, we made one mistake: we didn't swap os.Remove for os.RemoveAll.
Ever since then, we've been leaving temporary files behind.

Add regression tests, which failed before the fix, and fix the bug.
Note that we need to test `garble reverse` as well, as it calls
toolexecCmd separately, so it needs its own cleanup as well.

The cleanup happens via the env var, which doesn't feel worse than
having toolexecCmd return an extra string or cleanup func.

While here, also test that we support TMPDIRs with special characters.
Daniel Martí 1 month ago
parent
commit
2d12f41e71
5 changed files with 57 additions and 6 deletions
  1. 1 1
      main.go
  2. 34 0
      main_test.go
  3. 3 1
      reverse.go
  4. 15 4
      testdata/scripts/goenv.txt
  5. 4 0
      testdata/scripts/reverse.txt

+ 1 - 1
main.go

@@ -379,6 +379,7 @@ func mainErr(args []string) error {
 		return commandReverse(args)
 	case "build", "test":
 		cmd, err := toolexecCmd(command, args)
+		defer os.RemoveAll(os.Getenv("GARBLE_SHARED"))
 		if err != nil {
 			return err
 		}
@@ -513,7 +514,6 @@ This command wraps "go %s". Below is its help:
 		return nil, err
 	}
 	os.Setenv("GARBLE_SHARED", sharedTempDir)
-	defer os.Remove(sharedTempDir)
 	wd, err := os.Getwd()
 	if err != nil {
 		return nil, err

+ 34 - 0
main_test.go

@@ -9,10 +9,12 @@ import (
 	"go/ast"
 	"go/printer"
 	"go/token"
+	"io/fs"
 	mathrand "math/rand"
 	"os"
 	"os/exec"
 	"path/filepath"
+	"regexp"
 	"runtime"
 	"strings"
 	"testing"
@@ -118,6 +120,7 @@ func TestScripts(t *testing.T) {
 			"bincmp":            bincmp,
 			"generate-literals": generateLiterals,
 			"setenvfile":        setenvfile,
+			"grepfiles":         grepfiles,
 		},
 		UpdateScripts: *update,
 	}
@@ -262,6 +265,37 @@ func setenvfile(ts *testscript.TestScript, neg bool, args []string) {
 	ts.Setenv(args[0], ts.ReadFile(args[1]))
 }
 
+func grepfiles(ts *testscript.TestScript, neg bool, args []string) {
+	if len(args) != 2 {
+		ts.Fatalf("usage: grepfiles path pattern")
+	}
+	anyFound := false
+	path, pattern := args[0], args[1]
+	rx := regexp.MustCompile(pattern)
+	// TODO: use https://github.com/golang/go/issues/47209 when merged,
+	// hopefully in Go 1.20.
+	errSkipAll := fmt.Errorf("sentinel error: stop walking")
+	if err := filepath.WalkDir(path, func(path string, d fs.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if rx.MatchString(path) {
+			if neg {
+				return fmt.Errorf("%q matches %q", path, pattern)
+			} else {
+				anyFound = true
+				return errSkipAll
+			}
+		}
+		return nil
+	}); err != nil && err != errSkipAll {
+		ts.Fatalf("%s", err)
+	}
+	if !neg && !anyFound {
+		ts.Fatalf("no matches for %q", pattern)
+	}
+}
+
 func TestSplitFlagsFromArgs(t *testing.T) {
 	t.Parallel()
 	tests := []struct {

+ 3 - 1
reverse.go

@@ -44,7 +44,9 @@ One can reverse a captured panic stack trace as follows:
 	listArgs = append(listArgs, pkg)
 	// TODO: We most likely no longer need this "list -toolexec" call, since
 	// we use the original build IDs.
-	if _, err := toolexecCmd("list", listArgs); err != nil {
+	_, err := toolexecCmd("list", listArgs)
+	defer os.RemoveAll(os.Getenv("GARBLE_SHARED"))
+	if err != nil {
 		return err
 	}
 

+ 15 - 4
testdata/scripts/goenv.txt

@@ -1,3 +1,7 @@
+# Ensure that we support temporary directories with spaces and quotes.
+env TMPDIR=${WORK}/'.temp ''quotes'' and spaces'
+mkdir ${TMPDIR}
+
 # We need to properly quote the path to garble for toolexec.
 # If we don't, characters like spaces or quotes will result in errors.
 # EXEC_PATH is the test binary's os.Executable.
@@ -6,24 +10,31 @@
 env NAME='with spaces'
 mkdir $NAME
 cp $EXEC_PATH $NAME/garble$exe
-exec $NAME/garble$exe build main.go
+exec $NAME/garble$exe build
+
+# Ensure that we cleaned up the temporary files.
+! grepfiles ${TMPDIR} 'garble|importcfg|cache\.gob|\.go'
 
 [!windows] env NAME='with"double"quotes'
 [!windows] mkdir $NAME
 [!windows] cp $EXEC_PATH $NAME/garble$exe
-[!windows] exec $NAME/garble$exe build main.go
+[!windows] exec $NAME/garble$exe build
 
 env NAME='with''single''quotes'
 mkdir $NAME
 cp $EXEC_PATH $NAME/garble$exe
-exec $NAME/garble$exe build main.go
+exec $NAME/garble$exe build
 
 [!windows] env NAME='with"both''quotes'
 [!windows] mkdir $NAME
 [!windows] cp $EXEC_PATH $NAME/garble$exe
-[!windows] ! exec $NAME/garble$exe build main.go
+[!windows] ! exec $NAME/garble$exe build
 [!windows] stderr 'cannot be quoted'
 
+-- go.mod --
+module test/main
+
+go 1.18
 -- main.go --
 package main
 

+ 4 - 0
testdata/scripts/reverse.txt

@@ -21,6 +21,10 @@ stdin main.stderr
 garble reverse .
 cmp stdout reverse.stdout
 
+# Ensure that we cleaned up the temporary files.
+# Note that we rely on the unix-like TMPDIR env var name.
+[!windows] ! grepfiles ${TMPDIR} 'garble|importcfg|cache\.gob|\.go'
+
 ! garble build ./build-error
 cp stderr build-error.stderr