From d5d1131b7572f99a49f660f1376cb37856aaa913 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Wed, 17 Nov 2021 18:38:15 +0000
Subject: [PATCH] keep importmap entries in the right direction

For packages that we alter, we parse and modify the importcfg file.
Parsing is necessary so we can locate obfuscated object files,
which we use to remember what identifiers were obfuscated.
Modifying the files is necessary when we obfuscate import paths,
and those import paths have entries in an importcfg file.

However, we made one crucial mistake when writing the code.
When handling importmap entries such as:

	importmap golang.org/x/net/idna=vendor/golang.org/x/net/idna

we would name the two sides beforePath and afterPath, respectively.
They were added to importMap with afterPath as the key,
but when we iterated over the map to write a modified importcfg file,
we would then assume the key is beforepath.
All in all, we would end up writing the opposite direction:

	importmap vendor/golang.org/x/net/idna=golang.org/x/net/idna

This would ultimately result in the importmap never being useful,
and some rather confusing error messages such as:

	cannot find package golang.org/x/net/idna (using -importcfg)

Add a test case that reproduces this error,
and fix the code so it always uses beforePath as the key.
Note that we were also updating importCfgEntries with such entries.
I could not reproduce any failure when just removing that code,
nor could I explain why it was added there in the first place.
As such, remove that bit of code as well.

Finally, a reasonable question might be why we never noticed the bug.
In practice, such "importmap"s, represented as ImportMap by "go list",
only currently appear for packages vendored into the standard library.
Until very recently, we didn't support obfuscating most of std,
so we would usually not alter the affected importcfg files.
Now that we do parse and modify them, the bug surfaced.

Fixes #408.
---
 main.go                        | 10 ++++------
 testdata/scripts/goprivate.txt |  6 ++++++
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/main.go b/main.go
index 2903745..835f4c0 100644
--- a/main.go
+++ b/main.go
@@ -194,7 +194,7 @@ func obfuscatedTypesPackage(path string) *types.Package {
 		entry = &importCfgEntry{
 			packagefile: pkg.Export,
 		}
-		// Adding it to buildInfo.imports allows us to reuse the
+		// Adding it to importCfgEntries allows us to reuse the
 		// "if" branch below. Plus, if this edge case triggers
 		// multiple times in a single package compile, we can
 		// call "go list" once and cache its result.
@@ -351,6 +351,7 @@ func mainErr(args []string) error {
 			return err
 		}
 	}
+	// log.Println(tool, transformed)
 	cmd := exec.Command(args[0], transformed...)
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
@@ -867,6 +868,7 @@ func processImportCfg(flags []string) (newImportCfg string, _ error) {
 		return "", err
 	}
 
+	// TODO: use slices rather than maps to generate a deterministic importcfg.
 	importCfgEntries = make(map[string]*importCfgEntry)
 	importMap := make(map[string]string)
 
@@ -888,7 +890,7 @@ func processImportCfg(flags []string) (newImportCfg string, _ error) {
 				continue
 			}
 			beforePath, afterPath := args[:j], args[j+1:]
-			importMap[afterPath] = beforePath
+			importMap[beforePath] = afterPath
 		case "packagefile":
 			args := strings.TrimSpace(line[i+1:])
 			j := strings.Index(args, "=")
@@ -899,10 +901,6 @@ func processImportCfg(flags []string) (newImportCfg string, _ error) {
 
 			impPkg := &importCfgEntry{packagefile: objectPath}
 			importCfgEntries[importPath] = impPkg
-
-			if otherPath, ok := importMap[importPath]; ok {
-				importCfgEntries[otherPath] = impPkg
-			}
 		}
 	}
 	// log.Printf("%#v", buildInfo)
diff --git a/testdata/scripts/goprivate.txt b/testdata/scripts/goprivate.txt
index cd66db9..c3ac592 100644
--- a/testdata/scripts/goprivate.txt
+++ b/testdata/scripts/goprivate.txt
@@ -5,6 +5,12 @@ stderr '^GOPRIVATE="match-absolutely/nothing" does not match any packages to be
 env GOPRIVATE=test/main/imported
 garble build ./importer
 
+# Obfuscated packages which import non-obfuscated std packages.
+# Some of the imported std packages use "import maps" due to vendoring,
+# and a past bug made this case fail for "garble build".
+env GOPRIVATE=test/main
+garble build -o=out ./stdimporter
+
 [short] stop # rebuilding std is slow
 
 env GOPRIVATE='*'