From 36facaa1f9a24175f0fbe4fe5f479bbfb67d05e9 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Wed, 13 Sep 2023 09:58:17 -0400 Subject: [PATCH] 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 Auto-Submit: Bryan Mills LUCI-TryBot-Result: Go LUCI --- src/os/exec/exec.go | 44 ++++++++++++++++++++++++++++++---- src/os/exec/exec_test.go | 16 +++++++++++++ src/os/exec/lp_windows_test.go | 29 +++++++++++++++++----- 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) + } +}