cmd/buildlet: override the last PATH entry, not the first

If both the process environment and the "env" post field included
entries for "PATH", the setPathEnv helper function would erroneously
use the first one instead of the last. (In current versions of Go,
os/exec always uses the last entry for each variable.)

This change removes much of the complexity of setPathEnv, pushing some
of the less obvious parts of the logic (skipping empty slices and
suppressing no-op changes) to the caller side.

For golang/go#31567

Change-Id: I7460bf65c61073a71f0ea11d4d69a16f3e9b7c16
Reviewed-on: https://go-review.googlesource.com/c/build/+/354754
Trust: Bryan C. Mills <[email protected]>
Run-TryBot: Bryan C. Mills <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>

Author: Bryan C. Mills

cmd/buildlet/buildlet.go

@@ -936,7 +936,11 @@ func handleExec(w http.ResponseWriter, r *http.Request) {
 	if v := processGoCacheEnv; v != "" {
 		env = append(env, "GOCACHE="+v)
 	}
-	env = setPathEnv(env, r.PostForm["path"], *workDir)
+	if path := r.PostForm["path"]; len(path) > 0 {
+		if kv, ok := pathEnv(runtime.GOOS, env, path, *workDir); ok {
+			env = append(env, kv)
+		}
+	}
 	env = envutil.Dedup(runtime.GOOS, env)
 
 	var cmd *exec.Cmd
@@ -1005,89 +1009,47 @@ func pathNotExist(path string) bool {
 	return os.IsNotExist(err)
 }
 
-// setPathEnv returns a copy of the provided environment with any existing
-// PATH variables replaced by the user-provided path.
-// These substitutions are applied to user-supplied path elements:
-//   - the string "$PATH" expands to the original PATH elements
-//   - the substring "$WORKDIR" expands to the provided workDir
-// A path of just ["$EMPTY"] removes the PATH variable from the environment.
-func setPathEnv(env, path []string, workDir string) []string {
-	if len(path) == 0 {
-		return env
-	}
-
-	var (
-		pathIdx  = -1
-		pathOrig = ""
+// pathEnv returns a key=value string for the system path variable
+// (either PATH or path depending on the platform) with values
+// substituted from env:
+//   - the string "$PATH" expands to the original value of the path variable
+//   - the string "$WORKDIR" expands to the provided workDir
+//   - the string "$EMPTY" expands to the empty string
+//
+// The "ok" result reports whether kv differs from the path found in env.
+func pathEnv(goos string, env, path []string, workDir string) (kv string, ok bool) {
+	pathVar := "PATH"
+	if goos == "plan9" {
+		pathVar = "path"
+	}
+
+	orig := envutil.Get(goos, env, pathVar)
+	r := strings.NewReplacer(
+		"$PATH", orig,
+		"$WORKDIR", workDir,
+		"$EMPTY", "",
 	)
 
-	for i, s := range env {
-		if isPathEnvPair(s) {
-			pathIdx = i
-			pathOrig = s[len("PaTh="):] // in whatever case
-			break
-		}
-	}
-	if len(path) == 1 && path[0] == "$EMPTY" {
-		// Remove existing path variable if it exists.
-		if pathIdx >= 0 {
-			env = append(env[:pathIdx], env[pathIdx+1:]...)
-		}
-		return env
-	}
-
 	// Apply substitions to a copy of the path argument.
-	path = append([]string{}, path...)
-	for i, s := range path {
-		if s == "$PATH" {
-			path[i] = pathOrig // ok if empty
-		} else {
-			path[i] = strings.Replace(s, "$WORKDIR", workDir, -1)
+	subst := make([]string, 0, len(path))
+	for _, elem := range path {
+		if s := r.Replace(elem); s != "" {
+			subst = append(subst, s)
 		}
 	}
-
-	// Put the new PATH in env.
-	env = append([]string{}, env...)
-	pathEnv := pathEnvVar() + "=" + strings.Join(path, pathSeparator())
-	if pathIdx >= 0 {
-		env[pathIdx] = pathEnv
-	} else {
-		env = append(env, pathEnv)
-	}
-
-	return env
-}
-
-// isPathEnvPair reports whether the key=value pair s represents
-// the operating system's path variable.
-func isPathEnvPair(s string) bool {
-	// On Unix it's PATH.
-	// On Plan 9 it's path.
-	// On Windows it's pAtH case-insensitive.
-	if runtime.GOOS == "windows" {
-		return len(s) >= 5 && strings.EqualFold(s[:5], "PATH=")
-	}
-	if runtime.GOOS == "plan9" {
-		return strings.HasPrefix(s, "path=")
-	}
-	return strings.HasPrefix(s, "PATH=")
+	kv = pathVar + "=" + strings.Join(subst, pathListSeparator(goos))
+	v := kv[len(pathVar)+1:]
+	return kv, v != orig
 }
 
-// On Unix it's PATH.
-// On Plan 9 it's path.
-// On Windows it's pAtH case-insensitive.
-func pathEnvVar() string {
-	if runtime.GOOS == "plan9" {
-		return "path"
-	}
-	return "PATH"
-}
-
-func pathSeparator() string {
-	if runtime.GOOS == "plan9" {
+func pathListSeparator(goos string) string {
+	switch goos {
+	case "windows":
+		return ";"
+	case "plan9":
 		return "\x00"
-	} else {
-		return string(filepath.ListSeparator)
+	default:
+		return ":"
 	}
 }
 

cmd/buildlet/buildlet_test.go

@@ -5,57 +5,108 @@
 package main
 
 import (
-	"fmt"
+	"os"
 	"runtime"
 	"testing"
 )
 
-func TestSetPathEnv(t *testing.T) {
-	if runtime.GOOS == "windows" {
-		t.Skip("TODO(adg): make this test work on windows")
-	}
-
-	const workDir = "/workdir"
-
+func TestPathEnv(t *testing.T) {
 	for _, c := range []struct {
+		goos string // default "linux"
+		wd   string // default "/workdir"
 		env  []string
 		path []string
-		want []string
+		want string
+		noOp bool
 	}{
 		{ // No change to PATH
-			[]string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
-			[]string{},
-			[]string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
+			env:  []string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
+			path: []string{"$PATH"},
+			want: "PATH=/bin:/usr/bin",
+			noOp: true,
+		},
+		{ // Test that $EMPTY rewrites the path to be empty
+			env:  []string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
+			path: []string{"$EMPTY"},
+			want: "PATH=",
 		},
-		{ // Test sentinel $EMPTY value to clear PATH
-			[]string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
-			[]string{"$EMPTY"},
-			[]string{"A=1", "B=2"},
+		{ // Test that clearing an already-unset PATH is a no-op
+			env:  []string{"A=1", "B=2"},
+			path: []string{"$EMPTY"},
+			want: "PATH=",
+			noOp: true,
 		},
 		{ // Test $WORKDIR expansion
-			[]string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
-			[]string{"/go/bin", "$WORKDIR/foo"},
-			[]string{"A=1", "PATH=/go/bin:/workdir/foo", "B=2"},
+			env:  []string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
+			path: []string{"/go/bin", "$WORKDIR/foo"},
+			want: "PATH=/go/bin:/workdir/foo",
 		},
 		{ // Test $PATH expansion
-			[]string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
-			[]string{"/go/bin", "$PATH", "$WORKDIR/foo"},
-			[]string{"A=1", "PATH=/go/bin:/bin:/usr/bin:/workdir/foo", "B=2"},
+			env:  []string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
+			path: []string{"/go/bin", "$PATH", "$WORKDIR/foo"},
+			want: "PATH=/go/bin:/bin:/usr/bin:/workdir/foo",
 		},
 		{ // Test $PATH expansion (prepend only)
-			[]string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
-			[]string{"/go/bin", "/a/b", "$PATH"},
-			[]string{"A=1", "PATH=/go/bin:/a/b:/bin:/usr/bin", "B=2"},
+			env:  []string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
+			path: []string{"/go/bin", "/a/b", "$PATH"},
+			want: "PATH=/go/bin:/a/b:/bin:/usr/bin",
 		},
 		{ // Test $PATH expansion (append only)
-			[]string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
-			[]string{"$PATH", "/go/bin", "/a/b"},
-			[]string{"A=1", "PATH=/bin:/usr/bin:/go/bin:/a/b", "B=2"},
+			env:  []string{"A=1", "PATH=/bin:/usr/bin", "B=2"},
+			path: []string{"$PATH", "/go/bin", "/a/b"},
+			want: "PATH=/bin:/usr/bin:/go/bin:/a/b",
+		},
+		{ // Test that empty $PATH expansion is a no-op
+			env:  []string{"A=1", "B=2"},
+			path: []string{"$PATH"},
+			want: "PATH=",
+			noOp: true,
+		},
+		{ // Test that empty $PATH expansion does not add extraneous separators
+			env:  []string{"A=1", "B=2"},
+			path: []string{"$PATH", "$WORKDIR/foo"},
+			want: "PATH=/workdir/foo",
+		},
+		{ // Test that in case of multiple PATH entries we modify the last one,
+			// not the first.
+			env:  []string{"PATH=/bin:/usr/bin", "PATH=/bin:/usr/bin:/usr/local/bin"},
+			path: []string{"$WORKDIR/foo", "$PATH"},
+			want: "PATH=/workdir/foo:/bin:/usr/bin:/usr/local/bin",
+		},
+		{ // Test that Windows reads the existing variable regardless of case
+			goos: "windows",
+			wd:   `C:\workdir`,
+			env:  []string{"A=1", `PaTh=C:\Go\bin;C:\windows`, "B=2"},
+			path: []string{"$PATH", `$WORKDIR\foo`},
+			want: `PATH=C:\Go\bin;C:\windows;C:\workdir\foo`,
+		},
+		{ // Test that plan9 uses plan9 separators and "path" instead of "PATH"
+			goos: "plan9",
+			env:  []string{"path=/bin\x00/usr/bin", "PATH=/bananas"},
+			path: []string{"$PATH", "$WORKDIR/foo"},
+			want: "path=/bin\x00/usr/bin\x00/workdir/foo",
 		},
 	} {
-		got := setPathEnv(c.env, c.path, workDir)
-		if g, w := fmt.Sprint(got), fmt.Sprint(c.want); g != w {
-			t.Errorf("setPathEnv(%q, %q) = %q, want %q", c.env, c.path, g, w)
+		goos := c.goos
+		if goos == "" {
+			goos = "linux"
+		}
+		wd := c.wd
+		if wd == "" {
+			wd = "/workdir"
+		}
+		got, gotOk := pathEnv(goos, c.env, c.path, wd)
+		wantOk := !c.noOp
+		if got != c.want || gotOk != wantOk {
+			t.Errorf("pathEnv(%q, %q, %q, %q) =\n\t%q, %t\nwant:\n\t%q, %t", goos, c.env, c.path, wd, got, gotOk, c.want, wantOk)
 		}
 	}
 }
+
+func TestPathListSeparator(t *testing.T) {
+	sep := pathListSeparator(runtime.GOOS)
+	want := string(os.PathListSeparator)
+	if sep != want {
+		t.Errorf("pathListSeparator(%q) = %q; want %q", runtime.GOOS, sep, want)
+	}
+}