Skip to content

Commit

Permalink
Revert "os/exec: avoid calling LookPath in cmd.Start for resolved paths"
Browse files Browse the repository at this point in the history
This reverts CL 512155.

Reason for revert: CL 512155 introduced a race in that it caused
cmd.Start to set cmd.Path. Previously it was fine if code looked
at cmd.Path in one goroutine while calling cmd.Start in a different
goroutine.

A test case for this race is in CL 527495.

Change-Id: Ic18fdadf6763727f8ea748280d5f0e601b9bf374
Reviewed-on: https://go-review.googlesource.com/c/go/+/527337
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
  • Loading branch information
ianlancetaylor authored and gopherbot committed Sep 12, 2023
1 parent 0613896 commit 8221f90
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 65 deletions.
36 changes: 32 additions & 4 deletions src/os/exec/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,32 @@ func (c *Cmd) Run() error {
return c.Wait()
}

// lookExtensions finds windows executable by its dir and path.
// It uses LookPath to try appropriate extensions.
// lookExtensions does not search PATH, instead it converts `prog` into `.\prog`.
func lookExtensions(path, dir string) (string, error) {
if filepath.Base(path) == path {
path = "." + string(filepath.Separator) + path
}
if dir == "" {
return LookPath(path)
}
if filepath.VolumeName(path) != "" {
return LookPath(path)
}
if len(path) > 1 && os.IsPathSeparator(path[0]) {
return LookPath(path)
}
dirandpath := filepath.Join(dir, path)
// We assume that LookPath will only add file extension.
lp, err := LookPath(dirandpath)
if err != nil {
return "", err
}
ext := strings.TrimPrefix(lp, dirandpath)
return path + ext, nil
}

// Start starts the specified command but does not wait for it to complete.
//
// If Start returns successfully, the c.Process field will be set.
Expand Down Expand Up @@ -623,11 +649,13 @@ func (c *Cmd) Start() error {
}
return c.Err
}
lp, err := lookExtensions(c.Path, c.Dir)
if err != nil {
return err
if runtime.GOOS == "windows" {
lp, err := lookExtensions(c.Path, c.Dir)
if err != nil {
return err
}
c.Path = lp
}
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")
}
Expand Down
6 changes: 0 additions & 6 deletions src/os/exec/lp_plan9.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,3 @@ func LookPath(file string) (string, error) {
}
return "", &Error{file, ErrNotFound}
}

// lookExtensions is a no-op on non-Windows platforms, since
// they do not restrict executables to specific extensions.
func lookExtensions(path, dir string) (string, error) {
return path, nil
}
6 changes: 0 additions & 6 deletions src/os/exec/lp_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,3 @@ func LookPath(file string) (string, error) {
}
return "", &Error{file, ErrNotFound}
}

// lookExtensions is a no-op on non-Windows platforms, since
// they do not restrict executables to specific extensions.
func lookExtensions(path, dir string) (string, error) {
return path, nil
}
6 changes: 0 additions & 6 deletions src/os/exec/lp_wasm.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,3 @@ func LookPath(file string) (string, error) {
// Wasm can not execute processes, so act as if there are no executables at all.
return "", &Error{file, ErrNotFound}
}

// lookExtensions is a no-op on non-Windows platforms, since
// they do not restrict executables to specific extensions.
func lookExtensions(path, dir string) (string, error) {
return path, nil
}
43 changes: 0 additions & 43 deletions src/os/exec/lp_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,45 +63,6 @@ func findExecutable(file string, exts []string) (string, error) {
// As of Go 1.19, LookPath will instead return that path along with an error satisfying
// errors.Is(err, ErrDot). See the package documentation for more details.
func LookPath(file string) (string, error) {
return lookPath(file, pathExt())
}

// lookExtensions finds windows executable by its dir and path.
// It uses LookPath to try appropriate extensions.
// lookExtensions does not search PATH, instead it converts `prog` into `.\prog`.
func lookExtensions(path, dir string) (string, error) {
if filepath.Base(path) == path {
path = "." + string(filepath.Separator) + path
}
exts := pathExt()
if ext := filepath.Ext(path); ext != "" {
for _, e := range exts {
if strings.EqualFold(ext, e) {
// Assume that path has already been resolved.
return path, nil
}
}
}
if dir == "" {
return lookPath(path, exts)
}
if filepath.VolumeName(path) != "" {
return lookPath(path, exts)
}
if len(path) > 1 && os.IsPathSeparator(path[0]) {
return lookPath(path, exts)
}
dirandpath := filepath.Join(dir, path)
// We assume that LookPath will only add file extension.
lp, err := lookPath(dirandpath, exts)
if err != nil {
return "", err
}
ext := strings.TrimPrefix(lp, dirandpath)
return path + ext, nil
}

func pathExt() []string {
var exts []string
x := os.Getenv(`PATHEXT`)
if x != "" {
Expand All @@ -117,11 +78,7 @@ func pathExt() []string {
} else {
exts = []string{".com", ".exe", ".bat", ".cmd"}
}
return exts
}

// lookPath implements LookPath for the given PATHEXT list.
func lookPath(file string, exts []string) (string, error) {
if strings.ContainsAny(file, `:\/`) {
f, err := findExecutable(file, exts)
if err == nil {
Expand Down

0 comments on commit 8221f90

Please sign in to comment.