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 · 9 comments
Closed

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

bcmills opened this issue Sep 12, 2023 · 9 comments
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
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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 <[email protected]>
Auto-Submit: Bryan Mills <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
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 <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Bryan Mills <[email protected]>
@gopherbot
Copy link
Contributor

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 <[email protected]>
Reviewed-by: Alex Brainman <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
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 <[email protected]>
Reviewed-by: Alex Brainman <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@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
Closed
@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit that referenced this issue Jun 7, 2024
@qiulaidongfeng @gopherbot
os/exec: on Windows look for extensions in Run if not already done
5532427 
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.

Fixes #66586

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]>
qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Jun 8, 2024
@qiulaidongfeng
[release-branch.go1.22] os/exec: on Windows look for extensions in Ru…
fa7825e 
…n if not already done

CL 512155 fixed golang#36768, but introduced golang#62596.
CL 527820 fixed golang#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.

Fixes golang#66598

Change-Id: Id6199945ecca7bbe19d531070eea66b206b0564b
@qiulaidongfeng qiulaidongfeng mentioned this issue Jun 8, 2024
Closed
@gopherbot
Copy link
Contributor

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

@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
@qiulaidongfeng @gopherbot
[release-branch.go1.22] os/exec: on Windows look for extensions in Ru…
dfe4dbf 
…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]>
@gabyhelp gabyhelp mentioned this issue Jul 5, 2024
Closed
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