-
Notifications
You must be signed in to change notification settings - Fork 18k
path/filepath: use Windows GetFinalPathNameByHandle instead of os.Readlink to implement EvalSymlinks #20506
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
Comments
@darrenstahlmsft this is a reasonable request, but I do not know how to fetch the information you require. At this moment we use Windows DeviceIoControl call with FSCTL_GET_REPARSE_POINT parameter to fetch symlink target (see syscall.Readlink function). I tried calling DeviceIoControl with your example, and I get How did you encountered this problem? What is it that you are trying to do? Thank you. Alex |
@alexbrainman I am using a slightly forked version of filepath.EvalSymlinks that allows for paths starting with |
So we should just replace
That would be nice. Thank you.
Did os.Readlink change during go1.8? What change are you referring to? Regardless, I don't think we could change os.Readlink or syscall.Readlink now. But maybe we could just return whatever DeviceIoControl(...FSCTL_GET_REPARSE_POINT...) returns as is? Add new syscall.ReadlinkNTName or something?
Well, if your problem is filepath.EvalSymlinks, then tell me more about what is broken with filepath.EvalSymlinks. Just provide some examples. Alex |
@alexbrainman Using Unfortunately it's Vista+ only, so you'll have to fall back to the old behavior for XP (it's still supported, right?). |
Thank you for confirming. I am happy to retrofit this issue to fixing filepath.EvalSymlinks. But can you provide some examples of where filepath.EvalSymlinks is broken, please? I have some ideas, but nothing I can put into a test that will run on our builders. Also I noticed GetFinalPathNameByHandle can return "paths with drive letter" or "nt paths". We should use the former for compatibility reasons. But sometimes (see #18555) paths inside container lead to "nt path" only. If we ask for "paths with drive letter", the GetFinalPathNameByHandle fails. But it succeeds if we ask for "nt path". Should
I am not worried about XP. XP does not support symlinks and Docker. And there aren't many XP users left. We will leave XP as is. Alex |
Sorry for the delays in following up; I missed Alex's response. The easiest case where this is broken is when you have an NTFS volume mounted to a mount point (e.g. C:\foo) without having a separate drive letter for it. In this case, as I understand it EvalSymlinks on this path will fail. I don't know how to set this up easily in your test environment, unfortunately. You could create, format, and mount a VHD, but this will require administrative privileges and some fiddling to get right. I'm not aware of any call that would fail with a desktop or server OS in its default clean-install configuration. Regarding #18555, I suspect you should just fail if the drive letter option doesn't work. We want to fix this at the Windows level so that GetFInalPathNameByHandle just works. This will take a little while for us to fix, but I don't think Go should encode a workaround for what's really a Windows container bug. |
Fair enough. We already have plenty of filepath.EvalSymlinks tests. We would have to rely on them.
SGTM
SGTM I also changed this issue description. Hopefully new description covers what you want. Alex |
Change https://golang.org/cl/55250 mentions this issue: |
Change https://golang.org/cl/55612 mentions this issue: |
Change https://golang.org/cl/135295 mentions this issue: |
For a relative symlink in the root directory, such as /tmp -> private/tmp, we were dropping the leading slash. No test because we can't create a symlink in the root directory. The test TestGZIPFilesHaveZeroMTimes was failing on the Darwin builders. Updates #19922 Updates #20506 Change-Id: Ic83cb6d97ad0cb628fc551ac772a44fb3e20f038 Reviewed-on: https://go-review.googlesource.com/135295 Run-TryBot: Ian Lance Taylor <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Robert Griesemer <[email protected]>
This fix was added in 8e71b1e to work around a go issue (golang/go#20506). That issue was fixed in golang/go@66c03d3, which is part of Go 1.10 and up. This reverts the changes that were made in 8e71b1e, and are no longer needed. Signed-off-by: Sebastiaan van Stijn <[email protected]>
This fix was added in 8e71b1e to work around a go issue (golang/go#20506). That issue was fixed in golang/go@66c03d3, which is part of Go 1.10 and up. This reverts the changes that were made in 8e71b1e, and are no longer needed. Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: bad0b4e6043fa45e5f17f863f6c4dfba4398f414 Component: engine
What version of Go are you using (
go version
)?What operating system and processor architecture are you using (
go env
)?What did you do?
When a directory junction points to a path in the NT Namespace (See this MSDN page for details) that starts with
\\?\
os.Readlink strips the leading\\?\
which results in returning an invalid path.This requires cmd to create the Junction, so doesn't seem to work on play.golang.org. I've uploaded a repro to https://github.com/darrenstahlmsft/ReadJunction
To run the repro, run
mountvol.exe
which will print out:Replace the folderPath in main.go with the retrieved volume GUID path then run main.go.
What did you expect to see?
When run in go1.7.5 this works as expected and returns:
What did you see instead?
When run in go1.8.1 the returned path is invalid:
/cc @jstarks
The text was updated successfully, but these errors were encountered: