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

x/sys/windows/mkwinsyscall: IsStdRepo can be wrong when Go tree is inside a symlinked directory #44079

Closed
dmitshur opened this issue Feb 3, 2021 · 8 comments
Closed

x/sys/windows/mkwinsyscall: IsStdRepo can be wrong when Go tree is inside a symlinked directory #44079

dmitshur opened this issue Feb 3, 2021 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.17

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Feb 3, 2021 •
edited

The Source.IsStdRepo method tries to report whether src is part of standard library. It can return an incorrect result when src is a part of standard library but the Go tree is inside a symlinked directory.

To reproduce:

$ mkdir -p $HOME/dir1/dir2 && cd $HOME/dir1/dir2
$ git clone https://go.googlesource.com/go && cd go/src
$ ./make.bash

$ ln -s $HOME/dir1/dir2 $HOME/aaa && cd $HOME/aaa/go/src
$ export GOROOT="$HOME/aaa/go"
$ export PATH="$HOME/aaa/go/bin:$PATH"
$ go generate syscall
$ git status
[...]
	modified:   syscall/zsyscall_windows.go
[...]
$ git diff
[...]
 import (
-       "internal/syscall/windows/sysdll"
        "unsafe"
+
+       "golang.org/x/sys/windows"
 )
[...]

Filing this issue to determine if this can be fixed in the general case. There is a real possibility that it's infeasible due to the properties of symlinks.

It may also be relevant that go generate does not pass-through the PWD env var to the process it invokes, which can help with preserving a consistent os.Getwd result (but I haven't checked if changing that would be sufficient to handle the general case).

CC @zx2c4, @alexbrainman, @bcmills.
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 3, 2021
@dmitshur dmitshur added this to the Unreleased milestone Feb 3, 2021
@dmitshur dmitshur mentioned this issue Feb 3, 2021
Closed
@bcmills
Copy link
Contributor

bcmills commented Feb 3, 2021

It may also be relevant that go generate does not pass-through the PWD env var to the process it invokes

FWIW, that's #43862.

@bcmills
Copy link
Contributor

bcmills commented Feb 3, 2021

I note that (*Source).IsStdRepo is using a strictly lexical comparison, whereas the go command makes an attempt to evaluate symlinks for the equivalent comparison (see search.InDir).

If propagating PWD doesn't work, it may be possible to fix this issue by copying (or using a similar approach to) search.InDir.

@bcmills
Copy link
Contributor

bcmills commented Feb 3, 2021

A more robust alternative might be to give mkwinsyscall an explicit flag to indicate whether the package is in std, instead of (or in addition to) having it try to infer that information.

@dmitshur
Copy link
Contributor Author

dmitshur commented Feb 3, 2021

Thank you for analyzing this Bryan.

From local testing I've done earlier, I expect CL 287152 should be enough to fix the issue.

@alexbrainman
Copy link
Member

@dmitshur and @bcmills thank you for taking care of this. I am at my deep end here. So I trust you guys whatever decision you make.

Alex

@dmitshur dmitshur mentioned this issue Mar 23, 2021
Closed
@dmitshur
Copy link
Contributor Author

From local testing I've done earlier, I expect CL 287152 should be enough to fix the issue.

That CL has been submitted, and I can confirm the reproduce steps I've provided in the original issue no longer reproduce it.

However, I'm still running into this issue in a different context. I'll try to understand what's different and update this issue.

@dmitshur dmitshur added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 23, 2021
@dmitshur dmitshur self-assigned this Mar 23, 2021
@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. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Mar 24, 2021
@dmitshur dmitshur removed their assignment Mar 24, 2021
@dmitshur
Copy link
Contributor Author

dmitshur commented Mar 24, 2021 •
edited

Ok, removing WaitingForInfo, the problem was just that I was confused.

This issue is now fixed (by the go generate fix in CL 287152) and the reproduce instructions don't trigger at Go tip (yet they do trigger at Go 1.16.2 since that doesn't have the fix).

Closing because I don't see a need to do more at this point. (I agree that it would be more robust to give mkwinsyscall an explicit flag to indicate whether the package is in std, but we can defer that until there is a new reproducible problem and this needs more attention.)

@dmitshur dmitshur modified the milestones: Unreleased, Go1.17 Mar 24, 2021
@gopherbot
Copy link

Change https://golang.org/cl/304449 mentions this issue: cmd/internal/moddeps: fix false positive when $TMPDIR is symlinked

gopherbot pushed a commit that referenced this issue Mar 24, 2021
@dmitshur
cmd/internal/moddeps: fix false positive when $TMPDIR is symlinked
dade83a 
os.Getwd notes that if the current directory can be reached via
multiple paths (due to symbolic links), Getwd may return any one
of them. A way to ensure that the desired path is used is to set
the PWD environment variable pointing to it.

The go generate command has started to update the PWD environment
variable as of CL 287152, which was the missing link previously
resulting in mkwinsyscall misunderstanding whether it's inside
the std lib when symbolic links are involved (issue 44079).

Now all that's left is for us to also set the PWD environment
variable when invoking the go command in the test, so that it
too knows the intended working directory path to use.

Fixes #44080.
Updates #44079.
Updates #43862.

Change-Id: I65c9d19d0979f486800b9b328c9b45a1a3180e81
Reviewed-on: https://go-review.googlesource.com/c/go/+/304449
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
4 participants
@dmitshur @bcmills @gopherbot @alexbrainman