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: modifications to Path ignored when *Cmd is created using Command with an absolute path on Windows #68314

Closed
dmitshur opened this issue Jul 5, 2024 · 9 comments
Labels
Critical A critical problem that affects the availability or correctness of production systems built using Go 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 Jul 5, 2024

This can be reproduced at tip, go1.23rc1, and go1.22.5 on Windows (but not other OSes).
go1.22.4 and go1.21.12 are unaffected.

What did you do?

Create an *exec.Cmd using exec.Command, giving it an absolute path.
Modify its Path field (and, optionally, Args[0]) to point to another command.
Run it.

What did you see happen?

The modified value of Cmd.Path is ignored. The command that's actually invoked is from the original Command call.

What did you expect to see?

The command whose path is specified in Cmd.Path is the one that's executed.


Here's a small reproducer. For brevity, it uses the go and gofmt binaries.

package main

import (
	"fmt"
	"os/exec"
)

func main() {
	const exe1 = `C:\Program Files\Go\bin\go.exe`
	const exe2 = `C:\Program Files\Go\bin\gofmt.exe`

	cmd := exec.Command(exe1)
	cmd.Path = exe2
	cmd.Args = []string{cmd.Path}

	if err := cmd.Run(); err == nil {
		fmt.Println("OK: gofmt was executed")
	} else {
		fmt.Printf("fail: go was executed despite cmd.Path = %s\n", cmd.Path)
	}
}
$ GOTOOLCHAIN=go1.22.4  go run ./p.go
OK: gofmt was executed
$ GOTOOLCHAIN=go1.21.12 go run ./p.go
OK: gofmt was executed
$ GOTOOLCHAIN=go1.23rc1 go run ./p.go
fail: go was executed despite cmd.Path = C:\Program Files\Go\bin\gofmt.exe
$ GOTOOLCHAIN=go1.22.5  go run ./p.go
fail: go was executed despite cmd.Path = C:\Program Files\Go\bin\gofmt.exe

CC @golang/windows, @ianlancetaylor.

@dmitshur dmitshur added OS-Windows NeedsFix The path to resolution is known, but the work has not been done. labels Jul 5, 2024
@dmitshur dmitshur added this to the Go1.23 milestone Jul 5, 2024
@dmitshur
Copy link
Contributor Author

dmitshur commented Jul 5, 2024

I sent CL 596875 with a proposed fix. Its approach is based on an earlier prototype (CL 575275) for #66586.

@gopherbot
Copy link
Contributor

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

@qiulaidongfeng
Copy link
Member

qiulaidongfeng commented Jul 5, 2024

It seems that as long as we determine the cachedLookextensions and cmd.Path has a common no extension path prefix determines whether to use cachedLookExtension, which can solve the issue.

@qiulaidongfeng
Copy link
Member

For example:C:\go.exe and C:\gofmt, no extension path is C:\go and C:\gofmt ,it no equel, so don't use cachedLookExtension.

@qiulaidongfeng
Copy link
Member

If the no extension path is the same and the extension is different, my solution will fail.
For example: if Cmd.Path is C:\go.exe change to C:\go.bat , these two paths are different executable files, and my solution will fail, but CL596875 will not.
Sorry for the noise caused.

@dmitshur dmitshur added release-blocker Critical A critical problem that affects the availability or correctness of production systems built using Go labels Jul 7, 2024
@dmitshur dmitshur moved this to Done in Release Blockers Jul 7, 2024
@dmitshur
Copy link
Contributor Author

dmitshur commented Jul 7, 2024

@gopherbot Please backport to Go 1.22. Only Go 1.22 needs the fix, as this isn't an issue in Go 1.2​1.

This is a regression that can cause wrong binaries to be executed on Windows, with no workaround other than changing user code.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #68331 (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/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
Critical A critical problem that affects the availability or correctness of production systems built using Go NeedsFix The path to resolution is known, but the work has not been done. OS-Windows release-blocker
Projects
Status: Done
Development

No branches or pull requests

4 participants