Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os/exec: calling Cmd.Start after setting Cmd.Path manually to absolute path without ".exe" no longer implicitly adds ".exe" in Go 1.22 #66586

Closed
dmitshur opened this issue Mar 28, 2024 · 15 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Windows release-blocker
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Mar 28, 2024

https://go.dev/doc/go1.22#os/exec includes:

On Windows, Command and Cmd.Start no longer call LookPath if the path to the executable is already absolute and has an executable file extension.

I don't quite understand what criteria is used for determining whether a path has an executable file extension. Which extensions are included? Is the PATHEXT environment variable involved?


That said, I've narrowed down a following behavior change between Go 1.21 and 1.22 that I'm not sure if it's working as intended, so reporting it for investigation.

Consider the output of the following Go program on a Windows machine that has an executable file at the path "C:\Program Files\Go\bin\gofmt.exe", in a roughly default environment (i.e., PATHEXT is not modified):

package main

import (
	"fmt"
	"os"
	"os/exec"
	"strings"

	"github.com/google/go-cmp/cmp"
	"github.com/google/go-cmp/cmp/cmpopts"
)

func main() {
	if _, err := os.Stat(`C:\Program Files\Go\bin\gofmt.exe`); err != nil {
		fmt.Println("returning early; if gofmt.exe doesn't exist the rest of the output will be misleading")
		return
	} else if !strings.Contains(os.Getenv("PATHEXT"), ".EXE") {
		fmt.Println("returning early; if .exe isn't included in PATHEXT the rest of the output will be misleading")
		return
	}

	cmdViaCommand := exec.Command(`C:\Program Files\Go\bin\gofmt`)
	cmdManualPath := &exec.Cmd{
		Path: `C:\Program Files\Go\bin\gofmt`,
		Args: []string{`C:\Program Files\Go\bin\gofmt`},
	}
	diff := cmp.Diff(cmdViaCommand, cmdManualPath, cmpopts.IgnoreUnexported(exec.Cmd{}))
	if diff == "" {
		diff = "(no diff)\n"
	}
	fmt.Printf("diff (-cmdViaCommand +cmdManualPath):\n%s\n", diff)

	err := cmdManualPath.Run()
	fmt.Println("err:", err)
}

When running it using Go 1.21.8, the output is:

$ go run .
diff (-cmdViaCommand +cmdManualPath):
(no diff)

err: <nil>

But when running it with Go 1.22.1:

$ go run .
diff (-cmdViaCommand +cmdManualPath):
  &exec.Cmd{
-       Path: `C:\Program Files\Go\bin\gofmt.exe`,
+       Path: `C:\Program Files\Go\bin\gofmt`,
        Args: {`C:\Program Files\Go\bin\gofmt`},
        Env:  nil,
        ... // 8 ignored and 11 identical fields
  }

err: fork/exec C:\Program Files\Go\bin\gofmt: The system cannot find the file specified.

Not having to manually add ".exe" to the path and instead relying on the PATHEXT mechanism is very convenient when writing multi-platform Go programs, since it permits there not to be special cases for one of the GOOS values.

In that context, it seems there's no change in behavior when using exec.Command to create a *exec.Cmd and then calling Cmd.Start on it. But when creating it manually, Go 1.21 would use PATHEXT compensate for Cmd.Path missing a ".exe" suffix, whereas Go 1.22 doesn't. I can't quite tell from os/exec documentation (or the seemingly relevant release note) if this is a bug fix or a bug.

CC @golang/windows.

@dmitshur dmitshur added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 28, 2024
@rsc rsc self-assigned this Mar 29, 2024
@rsc
Copy link
Contributor

rsc commented Mar 29, 2024

@gopherbot please backport go1.22

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #66598 (for 1.22).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/575255 mentions this issue: os/exec: revert "avoid calling LookPath in cmd.Start for resolved paths"

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/575275 mentions this issue: os/exec: resolve extension in Cmd.Start if Command wasn't used

@cagedmantis
Copy link
Contributor

This issue was reviewed in the release meeting. Is there any status update to report on this issue?

@joedian
Copy link

joedian commented Apr 24, 2024

@dmitshur I see the CL was abandoned, is this issue also obsolete?

@dmitshur
Copy link
Contributor Author

@joedian It was a draft CL associated with a review comment. Russ has go.dev/cl/575255 which is still open but needs attention.

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Apr 25, 2024
For avoid data race for cmd.Path , in CL 527820 fixed data race , but addition of suffixe as shown in golang#66586 was also introduced.
The result of call lookExtensions is actually the name passed to os.StartProcess,
But solutions at the time chose to reuse cmd.Path to represent the name passed to os.StartProcess,since this results in golang#66586,
So use new field save call lookExtensions result.

Fixes golang#66586

Change-Id: Ib1baa6d7803f9471af6e38bcb55f0298422e6743
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/581695 mentions this issue: os/exec: not add a suffix to Cmd.Path

@cagedmantis
Copy link
Contributor

@rsc Is there is status update on this issue? There is a CL that is ready for review. This came up in the release meeting.

@mdempsky
Copy link
Contributor

Ping? Is this still a 1.23 release blocker? It seems like it's an issue present in 1.22, and so it merits backporting either way.

@rsc
Copy link
Contributor

rsc commented May 29, 2024

I'll take a look later today.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/589016 mentions this issue: os/exec: add new test

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue May 30, 2024
For avoid data race for cmd.Path , in CL 527820 fixed data race , but addition of suffixe as shown in golang#66586 was also introduced.
The result of call lookExtensions is actually the name passed to os.StartProcess,
But solutions at the time chose to reuse cmd.Path to represent the name passed to os.StartProcess,since this results in golang#66586,
So use new field save call lookExtensions result.

Fixes golang#66586

Change-Id: Ib1baa6d7803f9471af6e38bcb55f0298422e6743
qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue May 30, 2024
For avoid data race for cmd.Path , in CL 527820 fixed data race , but addition of suffixe as shown in golang#66586 was also introduced.
The result of call lookExtensions is actually the name passed to os.StartProcess,
But solutions at the time chose to reuse cmd.Path to represent the name passed to os.StartProcess,since this results in golang#66586,
So use new field save call lookExtensions result.

Fixes golang#66586

Change-Id: Ib1baa6d7803f9471af6e38bcb55f0298422e6743
qiulaidongfeng pushed a commit to qiulaidongfeng/go that referenced this issue May 30, 2024
Add a test of the Go 1.21 Windows path lookup behavior.

Fixes golang#66586.

Change-Id: I5691edb6047c0553f93545c7f3d0ee3530fa2ae4
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 7, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/594495 mentions this issue: [release-branch.go1.22] os/exec: on Windows look for extensions in Run if not already done

gopherbot pushed a commit that referenced this issue Jun 24, 2024
…n if not already done

CL 512155 fixed #36768, but introduced #62596.
CL 527820 fixed #62596, but meant that the code failed to look up
file extensions on Windows for a relative path.
This CL fixes that problem by recording whether it has already
looked up file extensions.
This does mean that if Path is set manually then we do not update
it with file extensions, as doing that would be racy.

For #66586
Fixes #66598

Change-Id: I9a0305d1e466c5e07bfbe442566ea12f5255a96e
GitHub-Last-Rev: dc3169f
GitHub-Pull-Request: #67035
Reviewed-on: https://go-review.googlesource.com/c/go/+/581695
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
(cherry picked from commit 5532427)
Reviewed-on: https://go-review.googlesource.com/c/go/+/594495
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Commit-Queue: Ian Lance Taylor <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/596875 mentions this issue: os/exec: only use cachedLookExtensions if Cmd.Path is unmodified

gopherbot pushed a commit that referenced this issue Jul 7, 2024
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 <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/596976 mentions this issue: [release-branch.go1.22] os/exec: only use cachedLookExtensions if Cmd.Path is unmodified

gopherbot pushed a commit that referenced this issue Jul 10, 2024
….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 <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
(cherry picked from commit d0146bd)
Reviewed-on: https://go-review.googlesource.com/c/go/+/596976
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Windows release-blocker
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants