From ff803004d00161d1d7357a3e1b72b7bed7640450 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Sun, 9 Jan 2022 22:26:04 +0000
Subject: [PATCH] avoid build ID mismatches when using -debugdir

A recent change added the -debugdir value to addGarbleToHash,
which is part of the hash seed for all obfuscation taking place.

In principle, it was okay to add, just like any other garble flag.
In practice, it broke the added test case:

	> garble -debugdir ./debug1 build
	[stderr]
	# test/main
	FBi9xa6e.(*ac0bCOhR).String: relocation target FBi9xa6e.rV55e6H9 not defined
	qmECK6zf.init.0: relocation target qmECK6zf.eUU08z98 not defined
	[...]

This is because -debugdir gets turned into an absolute path,
and not all garble processes ended up using it consistently.

The fix is rather simple; since -debugdir doesn't affect obfuscation,
don't include it in the build hash seeding at all.

Fixes #451.
---
 hash.go                       | 19 ++++++++++++++++---
 main.go                       |  2 +-
 testdata/scripts/debugdir.txt | 14 +++++++++++---
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/hash.go b/hash.go
index 88bf6aaa..ae3fe579 100644
--- a/hash.go
+++ b/hash.go
@@ -98,13 +98,14 @@ func addGarbleToHash(inputHash []byte) []byte {
 	if cache.GOGARBLE != "" {
 		fmt.Fprintf(h, " GOGARBLE=%s", cache.GOGARBLE)
 	}
-	appendFlags(h)
+	appendFlags(h, true)
 	return h.Sum(nil)[:buildIDComponentLength]
 }
 
 // appendFlags writes garble's own flags to w in string form.
 // Errors are ignored, as w is always a buffer or hasher.
-func appendFlags(w io.Writer) {
+// If forBuildHash is set, only the flags affecting a build are written.
+func appendFlags(w io.Writer, forBuildHash bool) {
 	if flagLiterals {
 		io.WriteString(w, " -literals")
 	}
@@ -114,7 +115,19 @@ func appendFlags(w io.Writer) {
 	if flagDebug {
 		io.WriteString(w, " -debug")
 	}
-	if flagDebugDir != "" {
+	if flagDebugDir != "" && !forBuildHash {
+		// -debugdir is a bit special.
+		//
+		// When passing down flags via -toolexec,
+		// we do want the actual flag value to be kept.
+		//
+		// For build hashes, we can skip the flag entirely,
+		// as it doesn't affect obfuscation at all.
+		//
+		// TODO: in the future, we could avoid using the -a build flag
+		// by using "-debugdir=yes" here, and caching the obfuscated source.
+		// Incremental builds would recover the cached source
+		// to repopulate the output directory if it was removed.
 		io.WriteString(w, " -debugdir=")
 		io.WriteString(w, flagDebugDir)
 	}
diff --git a/main.go b/main.go
index 1886a571..6ead7a0f 100644
--- a/main.go
+++ b/main.go
@@ -491,7 +491,7 @@ This command wraps "go %s". Below is its help:
 	var toolexecFlag strings.Builder
 	toolexecFlag.WriteString("-toolexec=")
 	toolexecFlag.WriteString(cache.ExecPath)
-	appendFlags(&toolexecFlag)
+	appendFlags(&toolexecFlag, false)
 
 	goArgs := []string{
 		command,
diff --git a/testdata/scripts/debugdir.txt b/testdata/scripts/debugdir.txt
index 98d82df6..a586d212 100644
--- a/testdata/scripts/debugdir.txt
+++ b/testdata/scripts/debugdir.txt
@@ -1,7 +1,10 @@
-env GOGARBLE=test/main
+env GOGARBLE=*
 
+# ! garble -debug -debugdir ./debug1 build
+# cp stderr /tmp/log
+# exit
 garble -debugdir ./debug1 build
-exists 'debug1/test/main/imported/imported.go' 'debug1/test/main/main.go'
+exists 'debug1/test/main/imported/imported.go' 'debug1/test/main/main.go' 'debug1/reflect/type.go'
 ! grep ImportedFunc $WORK/debug1/test/main/imported/imported.go
 ! grep ImportedFunc $WORK/debug1/test/main/main.go
 ! grep 'some comment' $WORK/debug1/test/main/main.go
@@ -22,7 +25,11 @@ go 1.17
 -- main.go --
 package main
 
-import "test/main/imported" // some comment
+import (
+	"reflect"
+
+	"test/main/imported" // some comment
+)
 
 type someType int // some comment
 var someVar = 0
@@ -33,6 +40,7 @@ type someStruct struct {
 
 func main() {
 	imported.ImportedFunc()
+	reflect.TypeOf(123)
 }
 
 -- imported/imported.go --