From 36facaa1f9a24175f0fbe4fe5f479bbfb67d05e9 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Wed, 13 Sep 2023 09:58:17 -0400 Subject: 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 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 4 deletions(-) (limited to 'src/os/exec/exec.go') 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, -- cgit v1.3