-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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,path/filepath: make os.Readlink and filepath.EvalSymlinks on Windows (mostly) only evaluate symlinks #63703
Comments
This proposal is interesting. I haven't realized till now that I've done a quick prototype of your proposal (see CL 537295) and added a bunch of tests to make sure they fix #39786 and #40176. Everything seems to work fine with just one modification to the proposal:
Notice the first two bullets are already implemented by Bonus point: the new behavior seems to be an alternative fix for the bug that was originally fixed by making |
[edit: I no longer think this; see later comments] I think I would still rather have But, given that I'm proposing for Especially given #57766, I suspect that the vast majority of existing callers are using |
From reading #30463, I believe that it was filed in response to containerd/continuity#113, which I believe is better addressed by https://go.dev/cl/463177. (The current code in Given that I think this proposal would address the underlying problem described in #29746 (comment) in a different way: namely, by not trying to resolve the |
Good enough 👍. About changing |
Actually, on further reading I think you are correct that I think we should deprecate |
Concretely: if a path with an NT prefix is passed to But if the argument refers to a symlink, and the symlink's substitute path includes an NT prefix, then |
This proposal has been added to the active column of the proposals project |
Can someone briefly summarize the suggested changes here? I may have misunderstood some comments above, but while I'm writing: We should not deprecate syscall.Readlink just because there is no Windows readlink system call. Strictly speaking there are no Windows system calls at all (that we are allowed to use), and the DLL calls that do exist definitely do not line up with the syscall API. Instead package syscall shoehorns Windows as best it can into a Unix-shaped boot. Even Open and Close are not Windows system calls or DLL calls. The fact that syscall.Readlink already exists and mostly matches os.Readlink suggests that it should remain that way, although it would be very good to deduplicate all that code into syscall (perhaps leaving fixLongPath in the os side). |
The changes are:
|
Hmm, so we would report mount points as That does appear to be consistent with what But Boost does uniformly treat them as symlinks:
I'm having trouble finding any calls to |
You can use this query: https://github.com/search?q=junction+os.readlink+language%3AGo&type=code&l=Go. Some selected examples: 1, 2, 3. I acknowledge that these are not popular projects and the value of the |
From the discussion of the current Python state, particularly python/cpython#82015 (comment), it's noted that when using a remote filesystem, symlinks and mount points are importantly different, as symlinks are resolved on the client (i.e. like POSIX) but the mount-point has to be resolved server-side when transitioning it, i.e. symlink-resolving code that reacts to seeing
So I have no strong feelings on |
A few observations from sampling the first page of the query results in #63703 (comment). There are lots of clones of https://github.com/rclone/rclone/. It only appears to call It only translates links for actual symlinks, and actually goes out of its way not to translate mount points that way: There are a couple of clones of https://github.com/BishopFox/sliver. It claims: “There is an edge case where if you are trying to download a junction on Windows, you will get access denied”, and it appears to be trying to work around that. It isn't at all clear to me what causes that error or how the attempted workaround works, but I have a suspicion that it may be the same sort of case as in #29746 (comment). That workaround was added recently by @RafBishopFox, so perhaps they can shed some light on the problem? There are a couple of clones of https://github.com/spicetify/spicetify-cli, but from what I can tell the calls to Its remaining calls to https://github.com/buildkite/agent uses https://github.com/cloudfoundry/groot-windows does use There are a couple of clones of https://github.com/vanadium-archive/go.jiri, but from what I can tell none of them have been updated since 2016 — and they appear to only use actual symlinks (created by https://github.com/tranvictor/jarvis uses https://github.com/petethacker/ll uses https://github.com/nyaosorg/go-windows-junction uses https://github.com/onozaty/go-sandbox also uses |
Since the vast majority of calls to But I do think it is important to arrange that we do not report junctions as symlinks and do not try to resolve them in |
Great summary @bcmills. To be clear, I'm completely onboarded on not reporting junctions as symlinks. I'm just doing some pushback to limit the blast radius of this change. |
I originally wrote that code sometime last year, so my recollection is a bit fuzzy. I was working on functionality in Sliver to create and send back an archive of a specified file or directory on a remote machine given filters (like I cannot reproduce the behavior now, so maybe something else was going on with my test scenario. I remember it was an odd edge case, and for some reason, the junction was not being resolved automatically. |
@RafBishopFox, I wonder if the problem you were running into was one of the ones fixed by https://go.dev/cl/463177. 🤔 |
Quite possibly! At the time, I think I chalked it up to a quirk with how I was trying to do things, and since the workaround was easy to implement, I did not spend a ton of time investigating it. |
What is the current proposal and is there general agreement for it yet? |
I got here while trying to figure out why The problem that got me here is trying to determine whether a path refers to a Volume Shadow Copy, which are identified by volume names, such as To figure out whether a path like |
I think we are still waiting for @qmuntal to return from a break and weigh in here, or someone else from Microsoft to give an opinion. |
I'm back, so I'll have to weigh in 😄. As said before, we should do this. Answering to @mxk:
We should always be using the
That issue will be solved if this proposal is approved, as neither |
Have all remaining concerns about this proposal been addressed? Proposal details are in #63703 (comment) |
It is a little hard to figure out the exact consequences. I would recommend testing against something akin to the list that is in 40180 comment (I can help creating the insane directory on at least two networked computers) and I would suggest apart from whatever GODEBUG is to also adhere to the |
@ericwj none of this seems inconsistent with the proposal. Is there an API for Windows already that respects SymlinkEvaluation policy? It seems like os.Readlink should return an error when the link is disallowed by policy, and then EvalSymlinks would "just work". So is there some function os.Readlink should be using? |
I am happy to see a well thought-through proposal. Tossing canonicalization leads to the solution it seems.
Not using that API anymore might also scrub this So like I said, add tests against a remote machine. Perhaps more than just that from the list I tested years ago. Proof the pudding. |
Based on the discussion above, this proposal seems like a likely accept. Proposal details are in #63703 (comment) |
No change in consensus, so accepted. 🎉 Proposal details are in #63703 (comment) |
Change https://go.dev/cl/565136 mentions this issue: |
Change https://go.dev/cl/567735 mentions this issue: |
This CL changes the behavior of os.Lstat to stop setting the os.ModeSymlink type mode bit for mount points on Windows. As a result, filepath.EvalSymlinks no longer evaluates mount points, which was the cause of many inconsistencies and bugs. Additionally, os.Lstat starts setting the os.ModeIrregular type mode bit for all reparse tags on Windows, except for those that are explicitly supported by the os package, which, since this CL, doesn't include mount points. This helps to identify files that need special handling outside of the os package. This behavior is controlled by the `winsymlink` GODEBUG setting. For Go 1.23, it defaults to `winsymlink=1`. Previous versions default to `winsymlink=0`. Fixes #39786 Fixes #40176 Fixes #61893 Updates #63703 Updates #40180 Updates #63429 Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-longtest,gotip-windows-arm64 Change-Id: I2e7372ab8862f5062667d30db6958d972bce5407 Reviewed-on: https://go-review.googlesource.com/c/go/+/565136 Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>
Change https://go.dev/cl/569195 mentions this issue: |
63703.md contains a paragraph that shouldn't be there, remove it. While here, fix a test error message related to the #63703 implementation. Updates #63703. Change-Id: I82a8b0b7dfa8f96530fb9a3a3aa971e03970f168 Reviewed-on: https://go-review.googlesource.com/c/go/+/569195 Reviewed-by: Michael Knyszek <mknyszek@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Background
Today (as of Go 1.21.3), the documentation for
path/filepath.EvalSymlinks
(including on Windows!) states:Notably,
EvalSymlinks
is not documented to evaluate non-symlinks, just as one would not label a jar as “cinnamon” if it actually contains a mix of many ingredients.Today, the documentation for
os.Readlink
states:#57766 notwithstanding, that documentation seems unambiguously intended to match the behavior of the POSIX
readlink
function.Unfortunately, today the implementation of
os.Readlink
on Windows tries to do much more than what it says on the tin. Specifically, it tries to evaluate the named path to a “canonical” path usingGetFinalPathNameByHandle
, not just evaluate its symlink components to their targets.Because it tries to do so much more, its implementation has some fairly serious bugs — and those bugs also come through in
filepath.EvalSymlinks
:In addition,
filepath.EvalSymlinks
on Windows currently canonicalizes the path elements in case-insensitive paths to use the casing found in the filesystem. I believe that most existing callers offilepath.EvalSymlinks
on Windows care primarily about evaluating symlinks and getting canonical casing for the path elements, and not at all about other kinds of canonicalization (because those other kinds are where most of the bugs are).In the past, we have proposed to do ...something(?) (I'm not clear exactly what) with
filepath.EvalSymlinks
, and to add some new function — perhaps calledfilepath.Resolve
— to evaluate “canonical” paths, providing the existing (aggressive, Windows-specific) behavior of thefilepath.EvalSymlinks
function as well as the Unix behavior offilepath.EvalSymlinks
(which, to reiterate, only evaluates symbolic links).#42201 was rejected due to a lack of consensus. The sticking point seems to have been the definition of “canonical”, which I agree is not well-defined — and I argue is not well-defined on Unix either, where we could imagine such things as platform-specific paths for mounted GUID volumes, hard-link targets, open files in
/proc
filesystems, and so on.This proposal is more limited in scope, aided by the deeper
GODEBUG
support added for #56986.Proposal
I propose that we add a
GODEBUG
setting (perhapswinsymlink
?) that changes the behavior ofos.Readlink
andos.Lstat
, with a default that varies with the Go version.At old Go releases (
winsymlink=0
):os.Lstat
would continue to consider both symlinks and mount points to beModeSymlink
.os.Readlink
would continue to try to resolve both symlinks and mount points by callingwindows.GetFinalPathNameByHandle
and interpreting the result.os.Readlink
has little to do with the function's documented behavior, I don't believe it is meaningful to even try to define what constitutes a “bug” in that implementation, let alone try to fix it. I propose that we mostly leave it as-is, bugs and all.At new Go releases (
winsymlink=1
):os.Lstat
would only reportModeSymlink
forIO_REPARSE_TAG_SYMLINK
. All other reparse tags — including mount points — would be reported as eitherModeIrregular
or regular files (forDEDUP
reparse points in particular) per os: treat all non-symlink reparse points as ModeIrregular on Windows #61893 and os: NTFS deduped file changed from regular to irregular #63429.os.Readlink
would only evaluate the target of a symbolic link (that is, a reparse point with tagIO_REPARSE_TAG_SYMLINK
), and return a value that is analogous to the value returned by POSIXreadlink
(asos.Readlink
already does on POSIX platforms).filepath.EvalSymlinks
would only do two transformations:EvalSymlinks
.Since
filepath.walkSymlinks
is written in a platform-independent way, I believe that fixingos.Readlink
may suffice to fixfilepath.EvalSymlinks
to the above specification. However, if we discover any other bugs infilepath.EvalSymlinks
, we should fix them to be in line with that.In addition,
syscall.Readlink
should be deprecated on Windows, andgolang.org/x/sys/windows.Readlink
should be deprecated: the Win32 API itself does not define areadlink
system call, and platform-agnostic abstractions belong inos
, notsyscall
.Readlink
functions may be left unchanged, or may be changed to also follow thewinsymlink
GODEBUG
setting.Compatibility
This proposal would provide backward-compatibility using a
GODEBUG
setting whose default varies with the Go version in use.Where it changes the behavior of functions, it would change them to better align with the existing documentation for those functions — a kind of change explicitly allowed by the Go 1 compatibility guidelines.
This proposal does not attempt to define a notion of “canonical path” on Windows. Programs would remain free to define their own notion of “canonical” using lower-level syscalls like
GetFinalPathNameByHandle
if desired.(attn @golang/windows; CC @networkimprov)
The text was updated successfully, but these errors were encountered: