From 5effe20c199e433742802791b8bf700aefd3e247 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Sat, 11 Feb 2023 18:34:47 +0000
Subject: [PATCH] avoid breaking github.com/davecgh/go-spew

It assumes that reflect.Value has a field named "flag",
which wasn't the case with obfuscated builds as we obfuscated it.

We already treated the reflect package as special,
for instance when not obfuscating Method or MethodByName.
In a similar fashion, mark reflect's rtype and Value types to not be
obfuscated alongside their field names. Note that rtype is the
implementation behind the reflect.Type interface.

This fix is fairly manual and repetitive.
transformCompile, transformLinkname, and transformAsm should all
use the same mechanism to tell if names should be obfuscated.
However, they do not do that right now, and that refactor feels too
risky for a bugfix release. We add more TODOs instead.

We're not adding go-spew to scripts/check-third-party.sh since the
project is largely abandoned. It's not even a Go module yet.
The only broken bit from it is what we've added to our tests.

Fixes #676.
---
 main.go                       | 27 +++++++++++++++++++--------
 testdata/script/linker.txtar  |  2 ++
 testdata/script/reflect.txtar | 10 ++++++++++
 3 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/main.go b/main.go
index f56cec63..8c0b860e 100644
--- a/main.go
+++ b/main.go
@@ -1118,7 +1118,10 @@ func (tf *transformer) transformLinkname(localName, newName string) (string, str
 
 	var newForeignName string
 	if receiver, name, ok := strings.Cut(foreignName, "."); ok {
-		if strings.HasPrefix(receiver, "(*") {
+		if lpkg.ImportPath == "reflect" && (receiver == "(*rtype)" || receiver == "Value") {
+			// These receivers are not obfuscated.
+			// See the TODO below.
+		} else if strings.HasPrefix(receiver, "(*") {
 			// pkg/path.(*Receiver).method
 			receiver = strings.TrimPrefix(receiver, "(*")
 			receiver = strings.TrimSuffix(receiver, ")")
@@ -1129,8 +1132,8 @@ func (tf *transformer) transformLinkname(localName, newName string) (string, str
 		}
 		// Exported methods are never obfuscated.
 		//
-		// TODO: we're duplicating the logic behind these decisions.
-		// How can we more easily reuse the same logic?
+		// TODO(mvdan): We're duplicating the logic behind these decisions.
+		// Reuse the logic with transformCompile.
 		if !token.IsExported(name) {
 			name = hashWithPackage(lpkg, name)
 		}
@@ -1908,19 +1911,29 @@ func (tf *transformer) transformGoFile(file *ast.File) *ast.File {
 			return true // universe scope
 		}
 
-		// The Go toolchain needs to detect symbols from these packages,
-		// so we are not obfuscating their package paths or declared names.
+		// TODO: We match by object name here, which is actually imprecise.
+		// For example, in package embed we match the type FS, but we would also
+		// match any field or method named FS.
 		path := pkg.Path()
 		switch path {
 		case "embed":
 			// FS is detected by the compiler for //go:embed.
+			// TODO: We probably want a conditional, otherwise we're not
+			// obfuscating the embed package at all.
 			return name == "FS"
 		case "reflect":
+			switch name {
 			// Per the linker's deadcode.go docs,
 			// the Method and MethodByName methods are what drive the logic.
-			switch name {
 			case "Method", "MethodByName":
 				return true
+			// Some packages reach into reflect internals, like go-spew.
+			// It's not particularly right of them to do that,
+			// and it's entirely unsupported, but try to accomodate for now.
+			// At least it's enough to leave the rtype and Value types intact.
+			case "rtype", "Value":
+				tf.recursivelyRecordAsNotObfuscated(obj.Type())
+				return true
 			}
 		}
 
@@ -2059,8 +2072,6 @@ func (tf *transformer) recursivelyRecordAsNotObfuscated(t types.Type) {
 		obj := t.Obj()
 		if pkg := obj.Pkg(); pkg == nil || pkg != tf.pkg {
 			return // not from the specified package
-		} else if pkg.Path() == "reflect" {
-			return // reflect's own types can always be obfuscated
 		}
 		if recordedAsNotObfuscated(obj) {
 			return // prevent endless recursion
diff --git a/testdata/script/linker.txtar b/testdata/script/linker.txtar
index 20d40d46..f2384639 100644
--- a/testdata/script/linker.txtar
+++ b/testdata/script/linker.txtar
@@ -1,3 +1,5 @@
+# Past garble versions might not properly patch cmd/link with "git apply"
+# when running inside a git repository. Skip the extra check with -short.
 [!short] [exec:git] exec git init -q
 [!short] [exec:git] env GARBLE_CACHE_DIR=$WORK/linker-cache
 
diff --git a/testdata/script/reflect.txtar b/testdata/script/reflect.txtar
index 194b4c68..99f933c8 100644
--- a/testdata/script/reflect.txtar
+++ b/testdata/script/reflect.txtar
@@ -154,6 +154,16 @@ type FmtType struct {
 	FmtTypeField int
 }
 
+// copied from github.com/davecgh/go-spew, which reaches into reflect's internals
+var _ = func() uintptr {
+	field, ok := reflect.TypeOf(reflect.Value{}).FieldByName("flag")
+	if !ok {
+		panic("reflect.Value has no flag field")
+	}
+	return field.Offset
+}()
+
+
 -- importedpkg/imported.go --
 package importedpkg