wrapper: correctly Join() paths for Android

filepath.Join(".", "foo") folds to "foo", which is fine as long as we're
not Exec()ing the result of that Join(). If we do, we might execute a
binary we weren't expecting to (e.g., one looked up from ${PATH}, rather
than the one at ${CWD}).

BUG=None
TEST=`go test`

Change-Id: Ifbd247c2e489c786d0a0d3342b5d1b61a7320796
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/2088432
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>

3 files changed, 62 insertions, 7 deletions

diff --git a/compiler_wrapper/compile_with_fallback_test.go b/compiler_wrapper/compile_with_fallback_test.go
index 4ea847f6..b36c3e18 100644
--- a/compiler_wrapper/compile_with_fallback_test.go
+++ b/compiler_wrapper/compile_with_fallback_test.go
@@ -229,7 +229,7 @@ func TestCompileWithFallbackLogCommandAndErrors(t *testing.T) {
 
 		log := readCompileWithFallbackErrorLog(ctx)
 		if log != `==================COMMAND:====================
-clang.real main.cc -fno-color-diagnostics -a -b
+./clang.real main.cc -fno-color-diagnostics -a -b
 
 someerror
 ==============================================
diff --git a/compiler_wrapper/compiler_wrapper.go b/compiler_wrapper/compiler_wrapper.go
index cecde1c6..c8c3b6f9 100644
--- a/compiler_wrapper/compiler_wrapper.go
+++ b/compiler_wrapper/compiler_wrapper.go
@@ -38,6 +38,29 @@ func callCompiler(env env, cfg *config, inputCmd *command) int {
 	return exitCode
 }
 
+// Given the main builder path and the absolute path to our wrapper, returns the path to the
+// 'real' compiler we should invoke.
+func calculateAndroidWrapperPath(mainBuilderPath string, absWrapperPath string) string {
+	// FIXME: This combination of using the directory of the symlink but the basename of the
+	// link target is strange but is the logic that old android wrapper uses. Change this to use
+	// directory and basename either from the absWrapperPath or from the builder.path, but don't
+	// mix anymore.
+
+	// We need to be careful here: path.Join Clean()s its result, so `./foo` will get
+	// transformed to `foo`, which isn't good since we're passing this path to exec.
+	basePart := filepath.Base(absWrapperPath) + ".real"
+	if !strings.ContainsRune(mainBuilderPath, filepath.Separator) {
+		return basePart
+	}
+
+	dirPart := filepath.Dir(mainBuilderPath)
+	if cleanResult := filepath.Join(dirPart, basePart); strings.ContainsRune(cleanResult, filepath.Separator) {
+		return cleanResult
+	}
+
+	return "." + string(filepath.Separator) + basePart
+}
+
 func callCompilerInternal(env env, cfg *config, inputCmd *command) (exitCode int, err error) {
 	if err := checkUnsupportedFlags(inputCmd); err != nil {
 		return 0, err
@@ -52,12 +75,7 @@ func callCompilerInternal(env env, cfg *config, inputCmd *command) (exitCode int
 	var compilerCmd *command
 	clangSyntax := processClangSyntaxFlag(mainBuilder)
 	if cfg.isAndroidWrapper {
-		// FIXME: This combination of using the directory of the symlink but the
-		// basename of the link target is strange but is the logic that old android
-		// wrapper uses. Change this to use directory and basename either from the
-		// absWrapperPath or from the builder.path, but don't mix anymore.
-		mainBuilder.path = filepath.Join(filepath.Dir(mainBuilder.path), filepath.Base(mainBuilder.absWrapperPath)+".real")
-
+		mainBuilder.path = calculateAndroidWrapperPath(mainBuilder.path, mainBuilder.absWrapperPath)
 		switch mainBuilder.target.compilerType {
 		case clangType:
 			mainBuilder.addPreUserArgs(mainBuilder.cfg.clangFlags...)
diff --git a/compiler_wrapper/compiler_wrapper_test.go b/compiler_wrapper/compiler_wrapper_test.go
index 67cbda92..a132ec5c 100644
--- a/compiler_wrapper/compiler_wrapper_test.go
+++ b/compiler_wrapper/compiler_wrapper_test.go
@@ -148,3 +148,40 @@ func TestPrintOtherCompilerError(t *testing.T) {
 		t.Errorf("Unexpected string. Got: %s", buffer.String())
 	}
 }
+
+func TestCalculateAndroidWrapperPath(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		mainBuilderPath string
+		absWrapperPath  string
+		want            string
+	}{
+		{
+			mainBuilderPath: "/foo/bar",
+			absWrapperPath:  "/bar/baz",
+			want:            "/foo/baz.real",
+		},
+		{
+			mainBuilderPath: "/my_wrapper",
+			absWrapperPath:  "/bar/baz",
+			want:            "/baz.real",
+		},
+		{
+			mainBuilderPath: "no_seps",
+			absWrapperPath:  "/bar/baz",
+			want:            "baz.real",
+		},
+		{
+			mainBuilderPath: "./a_sep",
+			absWrapperPath:  "/bar/baz",
+			want:            "./baz.real",
+		},
+	}
+
+	for _, tc := range testCases {
+		if result := calculateAndroidWrapperPath(tc.mainBuilderPath, tc.absWrapperPath); result != tc.want {
+			t.Errorf("Failed calculating the wrapper path with (%q, %q); got %q, want %q", tc.mainBuilderPath, tc.absWrapperPath, result, tc.want)
+		}
+	}
+}