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>

1 files changed, 40 insertions, 4 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,