From ea96074191e2fdd435c7f38ce0283bb868336122 Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Thu, 4 Jul 2024 18:07:45 -0400 Subject: [PATCH] [release-branch.go1.22] os/exec: only use cachedLookExtensions if Cmd.Path is unmodified Caching the invocation of lookExtensions on an absolute path in Command and reusing the cached result in Start is only viable if Cmd.Path isn't set to a different value after Command returns. For #66586. For #68314. Fixes #68331. Change-Id: I57007850aca2011b11344180c00faded737617b5 Reviewed-on: https://go-review.googlesource.com/c/go/+/596875 Reviewed-by: qiu laidongfeng2 <2645477756@qq.com> Reviewed-by: Ian Lance Taylor Auto-Submit: Dmitri Shuralyov LUCI-TryBot-Result: Go LUCI Reviewed-by: Dmitri Shuralyov (cherry picked from commit d0146bd85bb6870aa43a498b06ccb473af55cbe3) Reviewed-on: https://go-review.googlesource.com/c/go/+/596976 --- src/os/exec/exec.go | 65 +++++++++++++++++++++------------------- src/os/exec/exec_test.go | 22 +++++++++++++- 2 files changed, 56 insertions(+), 31 deletions(-) diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index a52b75f69cde0f..b8ef5a087b619e 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -334,8 +334,10 @@ type Cmd struct { lookPathErr error // cachedLookExtensions caches the result of calling lookExtensions. + // It is set when Command is called with an absolute path, letting it do + // the work of resolving the extension, so Start doesn't need to do it again. // This is only used on Windows. - cachedLookExtensions string + cachedLookExtensions struct{ in, out string } } // A ctxResult reports the result of watching the Context associated with a @@ -436,12 +438,12 @@ func Command(name string, arg ...string) *Cmd { // Since the path is absolute, its extension should be unambiguous // and independent of cmd.Dir, and we can go ahead and cache the lookup now. // - // 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 := lookExtensions(name, "") - cmd.cachedLookExtensions = lp - if err != nil { + // Note that we don't cache anything 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. + if lp, err := lookExtensions(name, ""); err == nil { + cmd.cachedLookExtensions.in, cmd.cachedLookExtensions.out = name, lp + } else { cmd.Err = err } } @@ -642,29 +644,32 @@ func (c *Cmd) Start() error { return c.Err } lp := c.Path - if c.cachedLookExtensions != "" { - lp = c.cachedLookExtensions - } - if runtime.GOOS == "windows" && c.cachedLookExtensions == "" { - // 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 + if runtime.GOOS == "windows" { + if c.Path == c.cachedLookExtensions.in { + // If Command was called with an absolute path, we already resolved + // its extension and shouldn't need to do so again (provided c.Path + // wasn't set to another value between the calls to Command and Start). + lp = c.cachedLookExtensions.out + } else { + // If *Cmd was made without using Command at all, or if Command was + // called with a relative path, we had to wait until now to resolve + // it in case c.Dir was changed. + // + // 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 + } } } if c.Cancel != nil && c.ctx == nil { diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go index b08e0c1ccf2110..fb03a6116ac87f 100644 --- a/src/os/exec/exec_test.go +++ b/src/os/exec/exec_test.go @@ -1838,7 +1838,7 @@ func TestPathRace(t *testing.T) { func TestAbsPathExec(t *testing.T) { testenv.MustHaveExec(t) - testenv.MustHaveGoBuild(t) // must have GOROOT/bin/gofmt, but close enough + testenv.MustHaveGoBuild(t) // must have GOROOT/bin/{go,gofmt} // A simple exec of a full path should work. // Go 1.22 broke this on Windows, requiring ".exe"; see #66586. @@ -1863,4 +1863,24 @@ func TestAbsPathExec(t *testing.T) { if err == nil { t.Errorf("using exec.Cmd{Path: %#q}: unexpected success", cmd.Path) } + + // A simple exec after modifying Cmd.Path should work. + // This broke on Windows. See go.dev/issue/68314. + t.Run("modified", func(t *testing.T) { + if exec.Command(filepath.Join(testenv.GOROOT(t), "bin/go")).Run() == nil { + // The implementation of the test case below relies on the go binary + // exiting with a non-zero exit code when run without any arguments. + // In the unlikely case that changes, we need to use another binary. + t.Fatal("test case needs updating to verify fix for go.dev/issue/68314") + } + exe1 := filepath.Join(testenv.GOROOT(t), "bin/go") + exe2 := filepath.Join(testenv.GOROOT(t), "bin/gofmt") + cmd := exec.Command(exe1) + cmd.Path = exe2 + cmd.Args = []string{cmd.Path} + err := cmd.Run() + if err != nil { + t.Error("ran wrong binary") + } + }) }