qbit/go
1
0
Fork 0
mirror of https://github.com/golang/go synced 2024-11-26 01:17: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

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

@ -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 {

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

@ -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")
}
})
}