os/exec: return error when PATH lookup would use current directory

Following discussion on #43724, change os/exec to take the
approach of golang.org/x/sys/execabs, refusing to respect
path entries mentioning relative paths by default.

Code that insists on being able to find executables in relative
directories in the path will need to add a couple lines to override the error.

See the updated package docs in exec.go for more details.

Fixes #43724.
Fixes #43947.

Change-Id: I73c1214f322b60b4167a23e956e933d50470fe13
Reviewed-on: https://go-review.googlesource.com/c/go/+/381374
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>

api/next/43724.txt

@@ -0,0 +1,2 @@
+pkg os/exec, type Cmd struct, Err error #43724
+pkg os/exec, var ErrDot error #43724

src/cmd/dist/build.go

@@ -27,6 +27,7 @@ import (
 var (
 	goarch           string
 	gorootBin        string
+	gorootBinGo      string
 	gohostarch       string
 	gohostos         string
 	goos             string
@@ -114,6 +115,12 @@ func xinit() {
 	goroot = filepath.Clean(b)
 	gorootBin = pathf("%s/bin", goroot)
 
+	// Don't run just 'go' because the build infrastructure
+	// runs cmd/dist inside go/bin often, and on Windows
+	// it will be found in the current directory and refuse to exec.
+	// All exec calls rewrite "go" into gorootBinGo.
+	gorootBinGo = pathf("%s/bin/go", goroot)
+
 	b = os.Getenv("GOROOT_FINAL")
 	if b == "" {
 		b = goroot

src/cmd/dist/test.go

@@ -27,6 +27,7 @@ func cmdtest() {
 	gogcflags = os.Getenv("GO_GCFLAGS")
 
 	var t tester
+
 	var noRebuild bool
 	flag.BoolVar(&t.listMode, "list", false, "list available tests")
 	flag.BoolVar(&t.rebuild, "rebuild", false, "rebuild everything first")
@@ -96,15 +97,9 @@ type distTest struct {
 func (t *tester) run() {
 	timelog("start", "dist test")
 
-	var exeSuffix string
-	if goos == "windows" {
-		exeSuffix = ".exe"
-	}
-	if _, err := os.Stat(filepath.Join(gorootBin, "go"+exeSuffix)); err == nil {
-		os.Setenv("PATH", fmt.Sprintf("%s%c%s", gorootBin, os.PathListSeparator, os.Getenv("PATH")))
-	}
+	os.Setenv("PATH", fmt.Sprintf("%s%c%s", gorootBin, os.PathListSeparator, os.Getenv("PATH")))
 
-	cmd := exec.Command("go", "env", "CGO_ENABLED")
+	cmd := exec.Command(gorootBinGo, "env", "CGO_ENABLED")
 	cmd.Stderr = new(bytes.Buffer)
 	slurp, err := cmd.Output()
 	if err != nil {
@@ -419,7 +414,7 @@ func (t *tester) registerStdTest(pkg string) {
 				args = append(args, "-run=^$")
 			}
 			args = append(args, stdMatches...)
-			cmd := exec.Command("go", args...)
+			cmd := exec.Command(gorootBinGo, args...)
 			cmd.Stdout = os.Stdout
 			cmd.Stderr = os.Stderr
 			return cmd.Run()
@@ -456,7 +451,7 @@ func (t *tester) registerRaceBenchTest(pkg string) {
 				args = append(args, "-bench=.*")
 			}
 			args = append(args, benchMatches...)
-			cmd := exec.Command("go", args...)
+			cmd := exec.Command(gorootBinGo, args...)
 			cmd.Stdout = os.Stdout
 			cmd.Stderr = os.Stderr
 			return cmd.Run()
@@ -484,7 +479,7 @@ func (t *tester) registerTests() {
 	} else {
 		// Use a format string to only list packages and commands that have tests.
 		const format = "{{if (or .TestGoFiles .XTestGoFiles)}}{{.ImportPath}}{{end}}"
-		cmd := exec.Command("go", "list", "-f", format)
+		cmd := exec.Command(gorootBinGo, "list", "-f", format)
 		if t.race {
 			cmd.Args = append(cmd.Args, "-tags=race")
 		}
@@ -619,7 +614,7 @@ func (t *tester) registerTests() {
 					fmt.Println("skipping terminal test; stdout/stderr not terminals")
 					return nil
 				}
-				cmd := exec.Command("go", "test")
+				cmd := exec.Command(gorootBinGo, "test")
 				setDir(cmd, filepath.Join(os.Getenv("GOROOT"), "src/cmd/go/testdata/testterminal18153"))
 				cmd.Stdout = os.Stdout
 				cmd.Stderr = os.Stderr
@@ -1003,7 +998,11 @@ func flattenCmdline(cmdline []interface{}) (bin string, args []string) {
 	}
 	list = out
 
-	return list[0], list[1:]
+	bin = list[0]
+	if bin == "go" {
+		bin = gorootBinGo
+	}
+	return bin, list[1:]
 }
 
 func (t *tester) addCmd(dt *distTest, dir string, cmdline ...interface{}) *exec.Cmd {
@@ -1157,7 +1156,7 @@ func (t *tester) registerHostTest(name, heading, dir, pkg string) {
 }
 
 func (t *tester) runHostTest(dir, pkg string) error {
-	out, err := exec.Command("go", "env", "GOEXE", "GOTMPDIR").Output()
+	out, err := exec.Command(gorootBinGo, "env", "GOEXE", "GOTMPDIR").Output()
 	if err != nil {
 		return err
 	}

src/cmd/dist/util.go

@@ -71,7 +71,11 @@ func run(dir string, mode int, cmd ...string) string {
 		errprintf("run: %s\n", strings.Join(cmd, " "))
 	}
 
-	xcmd := exec.Command(cmd[0], cmd[1:]...)
+	bin := cmd[0]
+	if bin == "go" {
+		bin = gorootBinGo
+	}
+	xcmd := exec.Command(bin, cmd[1:]...)
 	setDir(xcmd, dir)
 	var data []byte
 	var err error

src/cmd/go/testdata/script/cgo_path.txt

@@ -14,7 +14,7 @@ env GOCACHE=$WORK/gocache  # Looking for compile flags, so need a clean cache.
 [windows] exists -exec p/gcc.bat p/clang.bat
 ! exists p/bug.txt
 ! go build -x
-stderr '^cgo: exec (clang|gcc): (clang|gcc) resolves to executable relative to current directory \(.[/\\](clang|gcc)(.bat)?\)$'
+stderr '^cgo: C compiler "(clang|gcc)" not found: exec: "(clang|gcc)": cannot run executable found relative to current directory'
 ! exists p/bug.txt
 
 -- go.mod --

src/internal/execabs/execabs.go

@@ -12,11 +12,7 @@ package execabs
 
 import (
 	"context"
-	"fmt"
 	"os/exec"
-	"path/filepath"
-	"reflect"
-	"unsafe"
 )
 
 var ErrNotFound = exec.ErrNotFound
@@ -27,44 +23,14 @@ type (
 	ExitError = exec.ExitError
 )
 
-func relError(file, path string) error {
-	return fmt.Errorf("%s resolves to executable relative to current directory (.%c%s)", file, filepath.Separator, path)
-}
-
 func LookPath(file string) (string, error) {
-	path, err := exec.LookPath(file)
-	if err != nil {
-		return "", err
-	}
-	if filepath.Base(file) == file && !filepath.IsAbs(path) {
-		return "", relError(file, path)
-	}
-	return path, nil
-}
-
-func fixCmd(name string, cmd *exec.Cmd) {
-	if filepath.Base(name) == name && !filepath.IsAbs(cmd.Path) {
-		// exec.Command was called with a bare binary name and
-		// exec.LookPath returned a path which is not absolute.
-		// Set cmd.lookPathErr and clear cmd.Path so that it
-		// cannot be run.
-		lookPathErr := (*error)(unsafe.Pointer(reflect.ValueOf(cmd).Elem().FieldByName("lookPathErr").Addr().Pointer()))
-		if *lookPathErr == nil {
-			*lookPathErr = relError(name, cmd.Path)
-		}
-		cmd.Path = ""
-	}
+	return exec.LookPath(file)
 }
 
 func CommandContext(ctx context.Context, name string, arg ...string) *exec.Cmd {
-	cmd := exec.CommandContext(ctx, name, arg...)
-	fixCmd(name, cmd)
-	return cmd
-
+	return exec.CommandContext(ctx, name, arg...)
 }
 
 func Command(name string, arg ...string) *exec.Cmd {
-	cmd := exec.Command(name, arg...)
-	fixCmd(name, cmd)
-	return cmd
+	return exec.Command(name, arg...)
 }

src/os/exec/dot_test.go

@@ -0,0 +1,88 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package exec_test
+
+import (
+	"errors"
+	"internal/testenv"
+	"io/ioutil"
+	"os"
+	. "os/exec"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+)
+
+func TestLookPath(t *testing.T) {
+	testenv.MustHaveExec(t)
+
+	tmpDir := filepath.Join(t.TempDir(), "testdir")
+	if err := os.Mkdir(tmpDir, 0777); err != nil {
+		t.Fatal(err)
+	}
+
+	executable := "execabs-test"
+	if runtime.GOOS == "windows" {
+		executable += ".exe"
+	}
+	if err := ioutil.WriteFile(filepath.Join(tmpDir, executable), []byte{1, 2, 3}, 0777); err != nil {
+		t.Fatal(err)
+	}
+	cwd, err := os.Getwd()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() {
+		if err := os.Chdir(cwd); err != nil {
+			panic(err)
+		}
+	}()
+	if err = os.Chdir(tmpDir); err != nil {
+		t.Fatal(err)
+	}
+	origPath := os.Getenv("PATH")
+	defer os.Setenv("PATH", origPath)
+
+	// Add "." to PATH so that exec.LookPath looks in the current directory on all systems.
+	// And try to trick it with "../testdir" too.
+	for _, dir := range []string{".", "../testdir"} {
+		os.Setenv("PATH", dir+string(filepath.ListSeparator)+origPath)
+		t.Run("PATH="+dir, func(t *testing.T) {
+			good := dir + "/execabs-test"
+			if found, err := LookPath(good); err != nil || !strings.HasPrefix(found, good) {
+				t.Fatalf("LookPath(%q) = %q, %v, want \"%s...\", nil", good, found, err, good)
+			}
+			if runtime.GOOS == "windows" {
+				good = dir + `\execabs-test`
+				if found, err := LookPath(good); err != nil || !strings.HasPrefix(found, good) {
+					t.Fatalf("LookPath(%q) = %q, %v, want \"%s...\", nil", good, found, err, good)
+				}
+			}
+
+			if _, err := LookPath("execabs-test"); err == nil {
+				t.Fatalf("LookPath didn't fail when finding a non-relative path")
+			} else if !errors.Is(err, ErrDot) {
+				t.Fatalf("LookPath returned unexpected error: want Is ErrDot, got %q", err)
+			}
+
+			cmd := Command("execabs-test")
+			if cmd.Err == nil {
+				t.Fatalf("Command didn't fail when finding a non-relative path")
+			} else if !errors.Is(cmd.Err, ErrDot) {
+				t.Fatalf("Command returned unexpected error: want Is ErrDot, got %q", cmd.Err)
+			}
+			cmd.Err = nil
+
+			// Clearing cmd.Err should let the execution proceed,
+			// and it should fail because it's not a valid binary.
+			if err := cmd.Run(); err == nil {
+				t.Fatalf("Run did not fail: expected exec error")
+			} else if errors.Is(err, ErrDot) {
+				t.Fatalf("Run returned unexpected error ErrDot: want error like ENOEXEC: %q", err)
+			}
+		})
+	}
+}