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: on Windows, LookPath sometimes improperly returns ErrDot #53536

Closed
dkess opened this issue Jun 24, 2022 · 3 comments
Closed

os/exec: on Windows, LookPath sometimes improperly returns ErrDot #53536

dkess opened this issue Jun 24, 2022 · 3 comments
Assignees
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows release-blocker
Milestone

Comments

@dkess
Copy link

dkess commented Jun 24, 2022

Caused by the changes in https://go.dev/cl/403274.

When using LookPath to lookup an executable which is both in the current working directory and in the PATH, it returns an ErrDot, which is improper since the chosen path is still in the PATH.

Example: consider exec.LookPath("netsh"). When run from a "normal" working directory (C:\Users\MyUser), this correctly returns "C:\Windows\system32\netsh.exe", nil. But if the working directory is C:\Windows\system32, it returns the relative path with ErrDot.

This can subtly break any Go program that calls system32 executables that don't specify the full path, which is a common pattern.

An more concrete example from Google code: https://github.com/google/winops/blob/89df70d833b79d5b41af14ad1070df6ce09a5086/powershell/powershell_windows.go#L37. This would break if the current working directory is C:\Windows\system32 (which happens to be the default working directory of an administrator cmd window).

@bcmills bcmills added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Jun 24, 2022
@bcmills bcmills added this to the Go1.19 milestone Jun 24, 2022
@bcmills
Copy link
Contributor

bcmills commented Jun 24, 2022

The fix seems easy enough — if we would return ErrDot, we could first check to see whether the same path is also an absolute entry in %PATH%. If it is, we can return the absolute version instead of the relative one.

That's still a (very minor) semantic change compared to prior versions of Go — in prior versions we would return the relative path instead of the absolute one — but that seems acceptable to me because the two paths still refer to exactly the same executable.

@golang/windows, does that sound right?

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/414054 mentions this issue: os/exec: on Windows, suppress ErrDot if the implicit path matches the explicit one

@bcmills
Copy link
Contributor

bcmills commented Jun 28, 2022

Should be fixed at HEAD, and in the next beta or RC. @dkess, thanks for testing early and reporting it!

copybara-service bot pushed a commit to google/certtostore that referenced this issue Jul 1, 2022
copybara-service bot pushed a commit to google/certtostore that referenced this issue Jul 1, 2022
copybara-service bot pushed a commit to google/certtostore that referenced this issue Jul 1, 2022
copybara-service bot pushed a commit to google/winops that referenced this issue Jul 1, 2022
copybara-service bot pushed a commit to google/winops that referenced this issue Jul 1, 2022
copybara-service bot pushed a commit to google/winops that referenced this issue Jul 1, 2022
copybara-service bot pushed a commit to google/winops that referenced this issue Jul 1, 2022
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
… explicit one

If the current directory is also listed explicitly in %PATH%,
this changes the behavior of LookPath to prefer the explicit name for it
(and thereby avoid ErrDot).

However, in order to avoid running a different executable from what
would have been run by previous Go versions, we still return the
implicit path (and ErrDot) if it refers to a different file entirely.

Fixes golang#53536.
Updates golang#43724.

Change-Id: I7ab01074e21a0e8b07a176e3bc6d3b8cf0c873cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/414054
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jun 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows release-blocker
Projects
Status: Done
Development

No branches or pull requests

4 participants