From 9c0cdc61efe9e3e849fad904f8fb301290294489 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Tue, 6 Dec 2022 12:29:33 +0000
Subject: [PATCH] avoid reflect method call panics with GOGARBLE=*

We were obfuscating reflect's package path and its declared names,
but the toolchain wants to detect the presence of method reflection
to turn down the aggressiveness of dead code elimination.

Given that the obfuscation broke the detection,
we could easily end up in crashes when making reflect calls:

	fatal error: unreachable method called. linker bug?

	goroutine 1 [running]:
	runtime.throw({0x50c9b3?, 0x2?})
		runtime/panic.go:1047 +0x5d fp=0xc000063660 sp=0xc000063630 pc=0x43245d
	runtime.unreachableMethod()
		runtime/iface.go:532 +0x25 fp=0xc000063680 sp=0xc000063660 pc=0x40a845
	runtime.call16(0xc00010a360, 0xc00000e0a8, 0x0, 0x0, 0x0, 0x8, 0xc000063bb0)
		runtime/wcS9OpRFL:728 +0x49 fp=0xc0000636a0 sp=0xc000063680 pc=0x45eae9
	runtime.reflectcall(0xc00001c120?, 0x1?, 0x1?, 0x18110?, 0xc0?, 0x1?, 0x1?)
		<autogenerated>:1 +0x3c fp=0xc0000636e0 sp=0xc0000636a0 pc=0x462e9c

Avoid obfuscating the three names which cause problems: "reflect",
"Method", and "MethodByName".

While here, we also teach obfuscatedImportPath to skip "runtime",
as I also saw that the toolchain detects it for many reasons.
That wasn't a problem yet, as we do not obfuscate the runtime,
but it was likely going to become a problem in the future.
---
 main.go                       | 19 +++++++++++++------
 shared.go                     | 14 +++++++++++---
 testdata/script/reflect.txtar |  4 ----
 3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/main.go b/main.go
index 191ddeef..1c5673fb 100644
--- a/main.go
+++ b/main.go
@@ -1741,12 +1741,19 @@ func (tf *transformer) transformGo(file *ast.File) *ast.File {
 			return true // universe scope
 		}
 
-		if pkg.Path() == "embed" {
-			// The Go compiler needs to detect types such as embed.FS.
-			// That will fail if we change the import path or type name.
-			// Leave it as is.
-			// Luckily, the embed package just declares the FS type.
-			return true
+		// The Go toolchain needs to detect symbols from these packages,
+		// so we are not obfuscating their package paths or declared names.
+		switch pkg.Path() {
+		case "embed":
+			// FS is detected by the compiler for //go:embed.
+			return name == "FS"
+		case "reflect":
+			// Per the linker's deadcode.go docs,
+			// the Method and MethodByName methods are what drive the logic.
+			switch name {
+			case "Method", "MethodByName":
+				return true
+			}
 		}
 
 		// The package that declared this object did not obfuscate it.
diff --git a/shared.go b/shared.go
index 3d2ab2aa..bd07e4c3 100644
--- a/shared.go
+++ b/shared.go
@@ -158,9 +158,17 @@ type listedPackage struct {
 }
 
 func (p *listedPackage) obfuscatedImportPath() string {
-	// We can't obfuscate the embed package's import path,
-	// as the toolchain expects to recognize the package by it.
-	if p.ImportPath == "embed" || !p.ToObfuscate {
+	// We can't obfuscate these standard library import paths,
+	// as the toolchain expects to recognize the packages by them:
+	//
+	//   * runtime: it is special in many ways
+	//   * reflect: its presence turns down dead code elimination
+	//   * embed: its presence enables using //go:embed
+	switch p.ImportPath {
+	case "runtime", "reflect", "embed":
+		return p.ImportPath
+	}
+	if !p.ToObfuscate {
 		return p.ImportPath
 	}
 	newPath := hashWithPackage(p, p.ImportPath)
diff --git a/testdata/script/reflect.txtar b/testdata/script/reflect.txtar
index 1ebb71ea..194b4c68 100644
--- a/testdata/script/reflect.txtar
+++ b/testdata/script/reflect.txtar
@@ -41,13 +41,9 @@ func main() {
 	printfWithoutPackage("%T\n", importedpkg.ReflectTypeOf(2))
 	printfWithoutPackage("%T\n", importedpkg.ReflectTypeOfIndirect(4))
 
-
 	// More complex use of reflect.
 	v := importedpkg.ReflectValueOfVar
 	printfWithoutPackage("%#v\n", v)
-	// Keep the method from being unreachable, otherwise Call below may panic.
-	// TODO(mvdan): This only started being necessary with GOGARBLE=*. Why?
-	Sink = v.ExportedMethodName
 	method := reflect.ValueOf(&v).MethodByName("ExportedMethodName")
 	if method.IsValid() {
 		fmt.Println(method.Call(nil))