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>

diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index dfede0e7e2029ff4cbd2f55bb4c6a558bd4f580c..ea520f872abfab56c064f4b97fa66321944176a7 100644 (file)
--- 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 9783a133ba215f7b89ba97b915851760f81168a4..71a00494ad3bbefd942ac8ca7c8be832810c26b4 100644 (file)
--- 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 f2c56ccce4d4c3447f0c5f08e1891421cd0c32f8..6e7615fd444aa050e7a93d5e442d0f471e1ac1fa 100644 (file)
--- 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)
+       }
+}