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.

Sign up for GitHub

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

Jump to bottom

os/exec: Cmd.String races with Cmd.Start on Windows #62596

Closed
bcmills opened this issue Sep 12, 2023 · 7 comments · May be fixed by #67035
Closed

os/exec: Cmd.String races with Cmd.Start on Windows #62596

bcmills opened this issue Sep 12, 2023 · 7 comments · May be fixed by #67035
Assignees
@bcmills
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone
Go1.22

Comments

@bcmills
Copy link
Contributor

bcmills commented Sep 12, 2023

https://go.dev/cl/527337 reverted a change in os/exec due to a data race between the Start method and anything that inspects the Path field of the Cmd, notably including its String method.

https://go.dev/cl/527495 attempts to add a regression test for that data race, but currently fails on Windows because the data race is still present there.

The race appears to have been introduced in https://go.dev/cl/83020043, which added logic to update the Path field to reflect the resolved extension of the requested path. However, some of the regression tests added in that change are already failing in some circumstances (#62594).

In light of those failing tests, I believe we should resolve the race by no longer updating the Path field.

(CC @golang/windows, @ianlancetaylor)
@bcmills bcmills added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 12, 2023
@bcmills bcmills added this to the Go1.22 milestone Sep 12, 2023
@bcmills bcmills self-assigned this Sep 12, 2023
@ianlancetaylor
Copy link
Contributor

I wonder if it would make sense to update the Path field in exec.Command.

@bcmills
Copy link
Contributor Author

bcmills commented Sep 12, 2023

I think it would, but there are some corner cases where that will be a slightly incompatible change. (I have a stack with a bunch of test cleanup and most of a fix, but couldn't get it quite done today because my gopls is having fits of hanging and deadlocking. 😞)

@gopherbot
Copy link

Change https://go.dev/cl/528035 mentions this issue: os/exec: simplify Windows-specific tests

@gopherbot
Copy link

Change https://go.dev/cl/527820 mentions this issue: os/exec: avoid writing to Cmd.Path in Cmd.Start on Windows

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Sep 13, 2023
@gopherbot
os/exec: simplify Windows-specific tests
105f9d5 
- Use the test binary itself for printing paths instead of building a
  separate binary and running it through additional subprocesses.

- Factor out a common chdir helper.

- Use t.Setenv where appropriate.

- Reduce indirection in test helpers.

- Set NoDefaultCurrentDirectoryInExePath consistently in the
  environment.

Also add a test case demonstrating an interesting behavior for
relative paths that may interact with #62596.

Fixes #62594.
For #62596.

Change-Id: I19b9325034edf78cd0ca747594476cd7432bb451
Reviewed-on: https://go-review.googlesource.com/c/go/+/528035
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Sep 13, 2023
@gopherbot
os/exec: avoid calling LookPath in cmd.Start for resolved paths
f7f266c 
This reapplies CL 512155, which was previously reverted in CL 527337.
The race that prompted the revert should be fixed by CL 527820,
which will be submitted before this one.

For #36768.
Updates #62596.

Change-Id: I3c3cd92470254072901b6ef91c0ac52c8071e0a2
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-race,gotip-windows-amd64-race,gotip-windows-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/528038
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/548481 mentions this issue: doc: document os/exec changes on Windows

gopherbot pushed a commit that referenced this issue Dec 11, 2023
@gopherbot
doc: document os/exec changes on Windows
59275e2 
For #61422.
Updates #62596.
Updates #61493.

Change-Id: I5c910f9961da24d90b3618ee53540118db06ff91
Reviewed-on: https://go-review.googlesource.com/c/go/+/548481
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@ezz-no
doc: document os/exec changes on Windows
762f394 
For golang#61422.
Updates golang#62596.
Updates golang#61493.

Change-Id: I5c910f9961da24d90b3618ee53540118db06ff91
Reviewed-on: https://go-review.googlesource.com/c/go/+/548481
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@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 Mar 28, 2024
@qiulaidongfeng qiulaidongfeng mentioned this issue Apr 25, 2024
Open
@gopherbot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bcmills bcmills

Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Milestone
Go1.22
Development

Successfully merging a pull request may close this issue.

os/exec: on Windows look for extensions in Run if not already done qiulaidongfeng/go
4 participants
@dmitshur @ianlancetaylor @bcmills @gopherbot