os/exec: avoid writing to Cmd.Path in Cmd.Start on Windows

Fixes #62596.

Change-Id: I9003318ac1c4e3036f32383e62e9ba08c383d5c2
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-race,gotip-windows-amd64-race,gotip-windows-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/527820
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

3 files changed, 79 insertions, 10 deletions

diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index dfede0e7e2..ea520f872a 100644
--- a/src/os/exec/exec.go
+++ b/src/os/exec/exec.go
@@ -426,6 +426,26 @@ func Command(name string, arg ...string) *Cmd {
 		if err != nil {
 			cmd.Err = err
 		}
+	} else if runtime.GOOS == "windows" && filepath.IsAbs(name) {
+		// We may need to add a filename extension from PATHEXT
+		// or verify an extension that is already present.
+		// (We need to do this even for names that already have an extension
+		// in case of weird names like "foo.bat.exe".)
+		//
+		// Since the path is absolute, its extension should be unambiguous
+		// and independent of cmd.Dir, and we can go ahead and update cmd.Path to
+		// reflect it.
+		//
+		// Note that we cannot add an extension here for relative paths, because
+		// cmd.Dir may be set after we return from this function and that may cause
+		// the command to resolve to a different extension.
+		lp, err := LookPath(name)
+		if lp != "" {
+			cmd.Path = lp
+		}
+		if err != nil {
+			cmd.Err = err
+		}
 	}
 	return cmd
 }
@@ -649,12 +669,28 @@ func (c *Cmd) Start() error {
 		}
 		return c.Err
 	}
-	if runtime.GOOS == "windows" {
-		lp, err := lookExtensions(c.Path, c.Dir)
+	lp := c.Path
+	if runtime.GOOS == "windows" && !filepath.IsAbs(c.Path) {
+		// If c.Path is relative, we had to wait until now
+		// to resolve it in case c.Dir was changed.
+		// (If it is absolute, we already resolved its extension in Command
+		// and shouldn't need to do so again.)
+		//
+		// Unfortunately, we cannot write the result back to c.Path because programs
+		// may assume that they can call Start concurrently with reading the path.
+		// (It is safe and non-racy to do so on Unix platforms, and users might not
+		// test with the race detector on all platforms;
+		// see https://go.dev/issue/62596.)
+		//
+		// So we will pass the fully resolved path to os.StartProcess, but leave
+		// c.Path as is: missing a bit of logging information seems less harmful
+		// than triggering a surprising data race, and if the user really cares
+		// about that bit of logging they can always use LookPath to resolve it.
+		var err error
+		lp, err = lookExtensions(c.Path, c.Dir)
 		if err != nil {
 			return err
 		}
-		c.Path = lp
 	}
 	if c.Cancel != nil && c.ctx == nil {
 		return errors.New("exec: command with a non-nil Cancel was not created with CommandContext")
@@ -690,7 +726,7 @@ func (c *Cmd) Start() error {
 		return err
 	}
 
-	c.Process, err = os.StartProcess(c.Path, c.argv(), &os.ProcAttr{
+	c.Process, err = os.StartProcess(lp, c.argv(), &os.ProcAttr{
 		Dir:   c.Dir,
 		Files: childFiles,
 		Env:   env,
diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go
index 9783a133ba..71a00494ad 100644
--- a/src/os/exec/exec_test.go
+++ b/src/os/exec/exec_test.go
@@ -1819,3 +1819,19 @@ func TestConcurrentExec(t *testing.T) {
 	cancel()
 	hangs.Wait()
 }
+
+// TestPathRace tests that [Cmd.String] can be called concurrently
+// with [Cmd.Start].
+func TestPathRace(t *testing.T) {
+	cmd := helperCommand(t, "exit", "0")
+
+	done := make(chan struct{})
+	go func() {
+		out, err := cmd.CombinedOutput()
+		t.Logf("%v: %v\n%s", cmd, err, out)
+		close(done)
+	}()
+
+	t.Logf("running in background: %v", cmd)
+	<-done
+}
diff --git a/src/os/exec/lp_windows_test.go b/src/os/exec/lp_windows_test.go
index f2c56ccce4..6e7615fd44 100644
--- a/src/os/exec/lp_windows_test.go
+++ b/src/os/exec/lp_windows_test.go
@@ -554,14 +554,8 @@ func TestCommand(t *testing.T) {
 			if wantPath == "" {
 				if strings.Contains(tt.arg0, `\`) {
 					wantPath = tt.arg0
-					if filepath.Ext(wantPath) == "" {
-						wantPath += filepath.Ext(tt.want)
-					}
 				} else if tt.wantErrDot {
 					wantPath = strings.TrimPrefix(tt.want, tt.dir+`\`)
-					if filepath.Base(wantPath) == wantPath {
-						wantPath = `.\` + wantPath
-					}
 				} else {
 					wantPath = filepath.Join(root, tt.want)
 				}
@@ -572,3 +566,26 @@ func TestCommand(t *testing.T) {
 		})
 	}
 }
+
+func TestAbsCommandWithDoubledExtension(t *testing.T) {
+	t.Parallel()
+
+	comPath := filepath.Join(t.TempDir(), "example.com")
+	batPath := comPath + ".bat"
+	installBat(t, batPath)
+
+	cmd := exec.Command(comPath)
+	out, err := cmd.CombinedOutput()
+	t.Logf("%v:\n%s", cmd, out)
+	if err == nil {
+		got := strings.TrimSpace(string(out))
+		if got != batPath {
+			t.Errorf("wanted output %#q", batPath)
+		}
+	} else {
+		t.Errorf("%v: %v", cmd, err)
+	}
+	if cmd.Path != batPath {
+		t.Errorf("exec.Command(%#q).Path =\n     %#q\nwant %#q", comPath, cmd.Path, batPath)
+	}
+}