os,path/filepath: make os.Readlink and filepath.EvalSymlinks on Windows (mostly) only evaluate symlinks

[bcmills]

Background

Today (as of Go 1.21.3), the documentation for path/filepath.EvalSymlinks (including on Windows!) states:

EvalSymlinks returns the path name after the evaluation of any symbolic links. If path is relative the result will be relative to the current directory, unless one of the components is an absolute symbolic link. EvalSymlinks calls Clean on the result.

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:

“Readlink returns the destination of the named symbolic link.”

#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 using GetFinalPathNameByHandle, not just evaluate its symlink components to their targets.

• path/filepath: fix EvalSymLinks documentation #40180

Because it tries to do so much more, its implementation has some fairly serious bugs — and those bugs also come through in filepath.EvalSymlinks:

• path/filepath: EvalSymlinks of a directory which is mapped from volumeID (instead of drive letter) does not work #39786
• path/filepath: endless loop in EvalSymlinks because it resolves a link to the same path it asked for #40176

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 of filepath.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 called filepath.Resolve — to evaluate “canonical” paths, providing the existing (aggressive, Windows-specific) behavior of the filepath.EvalSymlinks function as well as the Unix behavior of filepath.EvalSymlinks (which, to reiterate, only evaluates symbolic links).

• proposal: path/filepath: add Resolve, replacing EvalSymlinks #42201

#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 (perhaps winsymlink?) that changes the behavior of os.Readlink and os.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 be ModeSymlink.
• os.Readlink would continue to try to resolve both symlinks and mount points by calling windows.GetFinalPathNameByHandle and interpreting the result.
• Since the existing behavior of 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 report ModeSymlink for IO_REPARSE_TAG_SYMLINK. All other reparse tags — including mount points — would be reported as either ModeIrregular or regular files (for DEDUP 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 tag IO_REPARSE_TAG_SYMLINK), and return a value that is analogous to the value returned by POSIX readlink (as os.Readlink already does on POSIX platforms).
  • If the symlink refers to an absolute path, the absolute path should be returned as an Win32 path (not an NT path) when that is feasible (see os,path/filepath: make os.Readlink and filepath.EvalSymlinks on Windows (mostly) only evaluate symlinks #63703 (comment)).
  • If the symlink refers to a relative path, the returned path fragment should be relative to the directory containing the symlink.
• As a result, filepath.EvalSymlinks would only do two transformations:
  1. Evaluate symlinks (— “what it says on the tin”).
  2. Canonicalize the casing of path elements (to maintain compatibility where it is likely to matter).
     • In addition, this behavior should be added to the doc comment for EvalSymlinks.

Since filepath.walkSymlinks is written in a platform-independent way, I believe that fixing os.Readlink may suffice to fix filepath.EvalSymlinks to the above specification. However, if we discover any other bugs in filepath.EvalSymlinks, we should fix them to be in line with that.

In addition, syscall.Readlink should be deprecated on Windows, and golang.org/x/sys/windows.Readlink should be deprecated: the Win32 API itself does not define a readlink system call, and platform-agnostic abstractions belong in os, not syscall.

• The behavior of these Readlink functions may be left unchanged, or may be changed to also follow the winsymlink 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)

[qmuntal]

This proposal is interesting. I haven't realized till now that GetFinalPathNameByHandle is the root of most devil in filepath.EvalSymlinks.

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: os.Readlink evaluate the target of a symbolic link AND replaces any NT prefix (\??\) with its Win32 counterpart. This means the following:

• Volumes with a device letter assigned get the NT prefix stripped. E.g. \??\C:\foo\bar => C:\foo\bar
• UNC NT path prefix is replaces with the UNC path prefix. E.g. \??\UNC\foo\bar => \\foo\bar
• Volumes without a drive letter (i.e. mount points) get the NT prefix replaced with the Win32 extended path prefix. E.g. \??\Volume{abc}\foo\bar => \\?\Volume{abc}\foo\bar.

Notice the first two bullets are already implemented by os.Readlink. This is necessary so the resulting path can interoperate with other Win32 APIs, which normally don't support the NT path prefix.

Bonus point: the new behavior seems to be an alternative fix for the bug that was originally fixed by making os.Readlink use GetFinalPathNameByHandle in CL 164201.

[bcmills]

[edit: I no longer think this; see later comments]

I think I would still rather have Readlink itself only read links, which I believe would also keep #30463 fixed (because as far as I can tell it doesn't involve any actual symlinks). On Unix platforms, readlink only reads one level of link — it doesn't do anything with earlier elements in the path.

But, given that I'm proposing for EvalSymlinks to keep its case-conversion behavior, I think it would be reasonable for EvalSymlinks to also convert the NT prefix to its Win32 counterpart. That would still make EvalSymlinks do more than just evaluate symlinks, but at least it confines the extra transformations to within one function.

Especially given #57766, I suspect that the vast majority of existing callers are using filepath.EvalSymlinks rather than os.Readlink directly, so moving the transformation into EvalSymlinks probably won't affect many (if any) real programs.

[bcmills]

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 github.com/containerd/continuity appears to incorrectly assume that os.Readlink returns an absolute or relative version of the requested path, which is not what it actually does on Unix; see #57766. For that, I have filed containerd/continuity#232.)

Given that continuity.context.Walk is already (still) buggy, I don't think we should worry too much about maintaining the behavior established for it in #30463.

I think this proposal would address the underlying problem described in #29746 (comment) in a different way: namely, by not trying to resolve the C:\Data mount point as a symlink in the first place.

[qmuntal]

I think I would still rather have Readlink itself only read links, which I believe would also keep #30463 fixed (because as far as I can tell it doesn't involve any actual symlinks). On Unix platforms, readlink only reads one level of link — it doesn't do anything with earlier elements in the path.

Good enough 👍.

About changing os.Readlink to just read the link, what if we partially revert CL 164201 and implement the fix in syscall.Readlink instead (gated by godebug, if necessary). This way we will have just one definition of what reading a link does, and users calling syscall.Readlink will also benefit from having consitiency across OSes.

[bcmills]

Actually, on further reading I think you are correct that os.Readlink itself should return a win32 path: its argument is generally a win32 path, so its return value should be as well. (The exact format of the reparse data buffer should be an implementation detail.)

I think we should deprecate syscall.Readlink entirely on Windows. (As far as I can tell, the Win32 API does not actually provide a readlink system call at all. The syscall package is supposed to provide low-level system calls, not platform-independent wrappers — those belong in os, which is what I am proposing to fix.)

[bcmills]

Concretely: if a path with an NT prefix is passed to os.Readlink as its argument, it shouldn't make any particular effort to strip that prefix.

But if the argument refers to a symlink, and the symlink's substitute path includes an NT prefix, then os.Readlink should return it as a Win32 path instead.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group