std.os.windows: add error.UnrecognizedVolume #18952
Conversation
andrewrk
added
breaking
| // This mapping is not correct; it is actually expected | ||
| // that calling GetFinalPathNameByHandle might return | ||
| // error.UnrecognizedVolume, and in fact has been observed | ||
| // in the wild. The problem is that wToPrefixedFileW was | ||
| // never intended to make *any* OS syscall APIs. It's only | ||
| // supposed to convert a string to one that is eligible to | ||
| // be used in the ntdll syscalls. | ||
| // | ||
| // To solve this, this function needs to no longer call | ||
| // GetFinalPathNameByHandle under any conditions, or the | ||
| // calling function needs to get reworked to not need to | ||
| // call this function. | ||
| // | ||
| // This may involve making breaking API changes. |
There was a problem hiding this comment.
For context, the dilemma is:
- ntdll syscalls cannot handle relative paths with
..components - In the best case, we can remove
..components trivially (this is whatnormalizePath/removeDotDirsSanitizeddoes) - However, to be able to make a syscall when given a path like
../../foo, it needs to be resolved into an absolute path and given the NT namespace prefix (\??\) RtlGetFullPathName_Ucan handle the case when resolving a path like../../fooagainst the cwd, but not against an arbitrary directory handle- Therefore, we use our own
GetFinalPathNameByHandleimplementation in the case where we (1) can't trivially remove..components and (2) need to resolve the path against a non-CWD dir
EDIT: So one possibility here is that there's some way to handle a path like ../../foo without needing to resolve it into an absolute path. The other is that this is a bug in our GetFinalPathNameByHandle implementation rather than wToPrefixedFileW (that is, we're not able to get a path of a handle that we should be able to get a path for).
EDIT#2: Here's where the GetFinalPathNameByHandle call was introduced: #16783 and the bug it fixed: #16779
There was a problem hiding this comment.
However, to be able to make a syscall when given a path like ../../foo, it needs to be resolved into an absolute path and given the NT namespace prefix (??)
Zig file system API should make this fail for all operating systems. You shouldn't be able to open a parent directory from an open directory handle.
The only exception is the current working directory, which is already an exception for other use cases.
This hack should have never been made, and by not making the hack it would have helped us reach that conclusion earlier.
There was a problem hiding this comment.
Fwiw, I believe it was me who reported ".. doesn’t work on windows” (#16779), and, for my use-case, it is indeed enough if that only works for cwd
There was a problem hiding this comment.
Zig file system API should make this fail for all operating systems. You shouldn't be able to open a parent directory from an open directory handle.
This might be a bit complicated due to things like #18452 and #7751. That is, resolving .. before symlinks (the Windows way) would be a break from normal non-Windows behavior, and resolving symlinks before .. would require syscalls. Symlinks in general complicate things, too.
There is a potentially relevant RESOLVE_BENEATH flag on Linux (and O_RESOLVE_BENEATH on FreeBSD), though: https://man7.org/linux/man-pages/man2/openat2.2.html
Thanks to @matklad for finding this additional NTSTATUS possibility when calling GetFinalPathNameByHandle.
andrewrk
force-pushed
the
windows-unrecognized-volume
branch
from
003aa82 to
f4620d4
Compare
Thanks to @matklad for finding this additional NTSTATUS possibility when calling GetFinalPathNameByHandle.
Follow-up issue: