Skip to content

Implement std.os.windows.GetPathNameByHandle using NT routines only #5993

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

Merged
merged 8 commits into from
Aug 11, 2020

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Aug 6, 2020

This PR is part of #1840.

This PR proposes an in-house implementation of std.os.windows.GetPathNameByHandle routine which you'd normally link to via kernelbase.dll. In particular, the routine is implemented using std.os.windows.ntdll.NtQueryInformationFile with class flags FileNormalizedNameInformation and FileVolumeNameInformation. The former allows us to get the normalized (canonical) path to the resource, however, the volume name is not prepended. This is what we use the latter for. Hence, in effect we call NtQueryInformationFile twice. However, there is still one complication in the fact that, by default, NTOS returns NT volume names, e.g., \Device\HarddiskVolume0 which needs to be converted to the more familiar DOS volume name such as C: or whatnot (an interesting fact, C: and \DosDevices\C: are simply symbolic link objects to \Device\HarddiskVolume0). To convert the NT volume name into DOS volume name, we issue std.os.windows.DeviceIoControl syscall with IOCTL_MOUNTMGR_QUERY_POINTS opcode, with the result of FileVolumeNameInformation as the input. Then, the mount manager returns all symbolic link objects that point at that volume, and we select the first available from the result pool as our DOS drive letter.

This PR took me quite a while to figure out as most of the API is not very well documented. The things got even more complicated since ReactOS doesn't provide GetFinalPathNameByHandleW yet, and Wine uses a slightly different (may I say dated approach?) plus Wine it seems somehow manages to obtain DOS drive letter upfront: link for the interested. Finally, using NtQueryInformationFile with FileNormalizedNameInformation seems to agree with what I've reversed engineered using ProcMon on the actual kernelbase.GetFinalPathNameByHandleW on Windows.

Anyhow, lemme know what y'all reckon! Comments and suggestions most welcome!

@kubkon kubkon added standard library This issue involves writing Zig code for the standard library. os-windows labels Aug 6, 2020
@kubkon kubkon requested review from andrewrk and daurnimator August 6, 2020 17:29
@kubkon
Copy link
Member Author

kubkon commented Aug 6, 2020

@andrewrk we can finally get rid of the TODO in std.os.realpathW ;-)

EDIT: after I figure out why the build fails in Windows CI...

EDIT2: I've found a bug in my implementation and patched it up, so the CI should be green now on all platforms. In your hands @andrewrk and @daurnimator.

kubkon added 3 commits August 6, 2020 23:56
This commit proposes an initial draft of `GetPathNameByHandle` function
which wraps NT syscalls and strives to emulate (currently only
partially) the `kernel32.GetFinalPathNameByHandleW` function.
Favour newer API which uses `NtQueryInformationFile` with class flags
`FileNormalizedNameInformation` and `FileVolumeNameInformation`
instead of lower-level `NtQueryObject`. `NtQueryObject` is still
used as a fallback in case the former are unavailable.
@kubkon kubkon force-pushed the getpathnamebyhandle branch 4 times, most recently from a93b0cb to 2f82d23 Compare August 7, 2020 08:45
This commit reimagines `std.os.windows.GetFinalPathNameByHandle`
using `DeviceIoControl` to query the OS mount manager for the DOS
(symlink) paths for the given NT volume name. In particular,
it uses `IOCTL_MOUNTMGR_QUERY_POINTS` ioctl opcode to query the
manager for the available moount points.
@kubkon kubkon force-pushed the getpathnamebyhandle branch from 2f82d23 to bdda8fa Compare August 7, 2020 09:33
@andrewrk
Copy link
Member

andrewrk commented Aug 7, 2020

589/1468 os.test.test "std-native-ReleaseSafe-bare-multi open smoke test"...incorrect alignment
D:\a\1\s\lib\std\os.zig:4011:9: 0x7ff65507c3f5 in os.realpath (test.obj)
        return realpathW(pathname_w.span(), out_buffer);
        ^
D:\a\1\s\lib\std\os\test.zig:37:40: 0x7ff654fb0aac in os.test.test "std-native-ReleaseSafe-bare-multi open smoke test" (test.obj)
        break :blk try fs.realpathAlloc(&arena.allocator, relative_path);
                                       ^
D:\a\1\s\lib\std\start.zig:134:65: 0x7ff65507cf26 in start.WinMainCRTStartup (test.obj)
    std.os.windows.kernel32.ExitProcess(initEventLoopAndCallMain());
                                                                ^
Unable to dump stack trace: FileNotFound

"incorrect alignment" is a safety check that is triggered for @intToPtr and @alignCast. It's unclear exactly which one of those is occurring here because of the optimizations moving code around. The closest occurrence to the above lines of code would be the std.mem.Allocator code that calls @alignCast for the realpathAlloc function. However that allocation uses a u8 which is 1-byte aligned, so it should be impossible to trigger this safety panic.

It's possible this PR has uncovered an unrelated bug. I'll dive in here and try to help figure out what's going on.

@andrewrk
Copy link
Member

andrewrk commented Aug 7, 2020

Related: #5996

@ptrCast already has assert(align_bytes != 1); in the codegen. So I think the stack trace above may be misleading. Is there any other @ptrToInt or @ptrCast happening in the modified codepaths of this PR?

@andrewrk
Copy link
Member

andrewrk commented Aug 7, 2020

Oh I see it :) Looks like it was a True Positive:

var path_buffer: [@sizeOf(FILE_NAME_INFORMATION) + PATH_MAX_WIDE * 2 + 2]u8 = undefined; 
var volume_buffer: [MAX_PATH]u8 = undefined;
// ...
const file_name = @ptrCast(*const FILE_NAME_INFORMATION, @alignCast(@alignOf(FILE_NAME_INFORMATION), &path_buffer[0]));
const volume_name = @ptrCast(*const FILE_NAME_INFORMATION, @alignCast(@alignOf(FILE_NAME_INFORMATION), &volume_buffer[0])); 

Those @alignCast there are asserting that the respective pointers will be properly aligned, however, the variable declarations are only 1 byte aligned. In this patch, I think you do not need to use @alignCast at all. Zig actually has pretty decent compile-time alignment awareness that makes it rare to need @alignCast. In this case, the use of @alignCast turned a compile error that caught a real bug into a runtime safety check panic (again that caught a real bug). Instead of @alignCast what you want to do there is something like this:

var volume_buffer: [MAX_PATH]u8 align(@alignOf(FILE_NAME_INFORMATION)) = undefined;

@kubkon
Copy link
Member Author

kubkon commented Aug 7, 2020

Oh nice one, thanks for looking into the issue here @andrewrk. I’ll make the necessary changes and run the release-safe tests again.

@kubkon
Copy link
Member Author

kubkon commented Aug 8, 2020

All green now! Thanks again @andrewrk for an alignment fix!

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kubkon nice work, merge at will

@kubkon
Copy link
Member Author

kubkon commented Aug 10, 2020

I guess this is okay as long as there is never a possible output that is more than can fit in a u32. i.e. would be bad in ReadFile, but okay here.

Agreed! To make sure we catch situations like these, a panic would be preferable here, so lemme tweak that!

And some other minor refactors which address more review comments.
@kubkon kubkon force-pushed the getpathnamebyhandle branch from bb8b24f to 73b9f65 Compare August 10, 2020 14:39
@kubkon
Copy link
Member Author

kubkon commented Aug 10, 2020

File paths don't have to be valid Utf16.
Just use std.mem.indexOfScalar(u16, out_buffer[0..total_len], 0)

Awesome suggestion, thanks! Incorporated in 73b9f65.

@kubkon
Copy link
Member Author

kubkon commented Aug 10, 2020

Unless you've got more comments you'd like me to address @daurnimator, I'm gonna go ahead and merge this in when the CI is green. Let me know what you reckon!

@daurnimator
Copy link
Contributor

Unless you've got more comments you'd like me to address @daurnimator, I'm gonna go ahead and merge this in when the CI is green. Let me know what you reckon!

go for it!

@andrewrk andrewrk merged commit 6325d6a into ziglang:master Aug 11, 2020
@kubkon kubkon deleted the getpathnamebyhandle branch August 11, 2020 04:57
@squeek502
Copy link
Collaborator

squeek502 commented Sep 16, 2020

Random note: with this merged, it's no longer possible to build Zig on Windows 7. FileNormalizedNameInformation is Windows 8+ specific, and building on Windows 7 now gives:

  reached unreachable code
  C:\Users\Ryan\Programming\Zig\zig\lib\std\os\windows.zig:1067:31: 0x13f5cd4f3 in std.os.windows.QueryInformationFile (build.obj)
          .INVALID_PARAMETER => unreachable,
                                ^
  C:\Users\Ryan\Programming\Zig\zig\lib\std\os\windows.zig:946:29: 0x13f5c00ec in std.os.windows.GetFinalPathNameByHandle (build.obj)
      try QueryInformationFile(hFile, .FileNormalizedNameInformation, path_buffer[0..]);
                              ^
  C:\Users\Ryan\Programming\Zig\zig\lib\std\os.zig:4134:68: 0x13f5a908c in std.os.getFdPath (build.obj)
              const wide_slice = try windows.GetFinalPathNameByHandle(fd, .{}, wide_buf[0..]);
                                                                     ^
  C:\Users\Ryan\Programming\Zig\zig\lib\std\os.zig:4123:21: 0x13f598118 in std.os.realpathW (build.obj)
      return getFdPath(h_file, out_buffer);
                      ^
  C:\Users\Ryan\Programming\Zig\zig\lib\std\os.zig:4046:25: 0x13f57d53d in std.os.realpath (build.obj)
          return realpathW(pathname_w.span(), out_buffer);
                          ^
  C:\Users\Ryan\Programming\Zig\zig\lib\std\fs.zig:2207:46: 0x13f55d8c1 in std.fs.realpathAlloc (build.obj)
      return allocator.dupe(u8, try os.realpath(pathname, &buf));
                                               ^
  C:\Users\Ryan\Programming\Zig\zig\test\tests.zig:400:25: 0x13f54d70a in std.special.test.tests.addCliTests (build.obj)
          fs.realpathAlloc(b.allocator, b.zig_exe) catch unreachable,
                          ^
  C:\Users\Ryan\Programming\Zig\zig\build.zig:185:39: 0x13f5396bd in std.special.build (build.obj)
      const test_cli = tests.addCliTests(b, test_filter, modes);
                                        ^
  C:\Users\Ryan\Programming\Zig\zig\lib\std\special\build_runner.zig:138:38: 0x13f52deca in runBuild (build.obj)
          .ErrorUnion => try root.build(builder),
                                       ^
  C:\Users\Ryan\Programming\Zig\zig\lib\std\special\build_runner.zig:119:17: 0x13f5144c9 in main (build.obj)
      try runBuild(builder);
                  ^
  C:\Users\Ryan\Programming\Zig\zig\lib\std\start.zig:154:65: 0x13f511abe in std.start.WinMainCRTStartup (build.obj)
      std.os.windows.kernel32.ExitProcess(initEventLoopAndCallMain());
                                                                  ^

I know Zig does not intend to support Windows 7, but before this, there wasn't anything Windows 8+ specific in the build process. Just ran into this error and thought it was worth noting.

@kubkon
Copy link
Member Author

kubkon commented Sep 16, 2020

@squeek502 that was by design on my part. However, if there is a feeling we should have wider support with GetPathNameByHandle, it shouldn't be too difficult to add a switch/if on Windows version and fallback to a different query value than FileNormalizedNameInformation. In fact, AFAIK ReactOS and Wine both use a generic NtQueryObject to query the path. I didn't, however, since I wasn't sure if the returned path is constrained by the 260 chars path limit or not.

@squeek502
Copy link
Collaborator

No worries; Zig is officially incompatible with Windows 7. Just thought I'd make a note of where it actually stopped working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os-windows standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants