qbit/go
1
0
Fork 0
mirror of https://github.com/golang/go synced 2024-11-26 05:37:57 -07:00
Code Releases Activity

os/exec: only use cachedLookExtensions if Cmd.Path is unmodified

Browse Source 
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.
Fixes #68314.

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 <iant@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
This commit is contained in:
Dmitri Shuralyov 2024-07-04 18:07:45 -04:00 committed by Gopher Robot
parent ad77cefeb2
commit d0146bd85b
2 changed files with 56 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
src/os/exec/exec.go
View File

@ -334,8 +334,10 @@ type Cmd struct {
lookPathErr error lookPathErr error
// cachedLookExtensions caches the result of calling lookExtensions. // 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. // 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 // 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 // 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. // 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 // 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 // cmd.Dir may be set after we return from this function and that may
// the command to resolve to a different extension. // cause the command to resolve to a different extension.
lp, err := lookExtensions(name, "") if lp, err := lookExtensions(name, ""); err == nil {
cmd.cachedLookExtensions = lp cmd.cachedLookExtensions.in, cmd.cachedLookExtensions.out = name, lp
if err != nil { } else {
cmd.Err = err cmd.Err = err
} }
} }
@ -642,14 +644,16 @@ func (c *Cmd) Start() error {
return c.Err return c.Err
} }
lp := c.Path lp := c.Path
if c.cachedLookExtensions != "" { if runtime.GOOS == "windows" {
lp = c.cachedLookExtensions if c.Path == c.cachedLookExtensions.in {
} // If Command was called with an absolute path, we already resolved
if runtime.GOOS == "windows" && c.cachedLookExtensions == "" { // its extension and shouldn't need to do so again (provided c.Path
// If c.Path is relative, we had to wait until now // wasn't set to another value between the calls to Command and Start).
// to resolve it in case c.Dir was changed. lp = c.cachedLookExtensions.out
// (If it is absolute, we already resolved its extension in Command } else {
// and shouldn't need to do so again.) // 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 // Unfortunately, we cannot write the result back to c.Path because programs
// may assume that they can call Start concurrently with reading the path. // may assume that they can call Start concurrently with reading the path.
@ -667,6 +671,7 @@ func (c *Cmd) Start() error {
return err return err
} }
} }
}
if c.Cancel != nil && c.ctx == nil { if c.Cancel != nil && c.ctx == nil {
return errors.New("exec: command with a non-nil Cancel was not created with CommandContext") return errors.New("exec: command with a non-nil Cancel was not created with CommandContext")
} }

22
src/os/exec/exec_test.go
View File

@ -1838,7 +1838,7 @@ func TestPathRace(t *testing.T) {
func TestAbsPathExec(t *testing.T) { func TestAbsPathExec(t *testing.T) {
testenv.MustHaveExec(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. // A simple exec of a full path should work.
// Go 1.22 broke this on Windows, requiring ".exe"; see #66586. // Go 1.22 broke this on Windows, requiring ".exe"; see #66586.
@ -1863,4 +1863,24 @@ func TestAbsPathExec(t *testing.T) {
if err == nil { if err == nil {
t.Errorf("using exec.Cmd{Path: %#q}: unexpected success", cmd.Path) 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")
}
})
} }