Fix `std.os.windows.GetFinalPathNameByHandle` for os versions before `win10_rs4` by rohlem · Pull Request #7379 · ziglang/zig

rohlem · 2020-12-10T15:42:50Z

This PR fixes std.os.windows.GetFinalPathNameByHandle for older Windows versions, unblocking #7242 (although that might be an ongoing effort, so shouldn't be closed yet I think).

#5993 removed the codepath that called the kernel32.dll implementation. According to some document I found, the new .FileNormalizedNameInformation query value was only added as recently as 2018 (not even 3 years ago), meaning we dropped support for more than just Windows 7.

Originally I just wanted to query the runtime OS version and reintroduce the old codepath, but then I tried the prophesized NtQueryObject function, and that ended up working too. Note though that it's using an undocumented value (ObjectNameInformation = 1 appears in ntifs.h, but not the documentation) and a struct that is named appropriately but also documented somewhere else. And yet, it still works.

If we trust this new implementation, I would be inclined to maybe delete the other codepath (that needs two ntdll calls instead of just one) in favor of this one. (And maybe move the logic for finding DOS volume names into its own function.)
But now that I've worked on this for a day, I thought I would ask for some feedback, to make sure I'm not overlooking anything crucial.

Tested (see new tests in std/os/windows.zig) on my Windows 7 and an up-to-date Windows 10 machine. Things I tried with this implementation:

NTFS junctions are resolved
Symbolic links are resolved
Hard links remain separate paths
Returned paths > 500 characters work without issue

Also let me know if I should squish/reorganize commits in any way, I thought for documentation/discussion (and maybe unexpected troubleshooting) the commits with the kernel32 path were worth keeping for now.

LemonBoy · 2020-12-10T16:34:40Z

lib/std/os/windows.zig

-    Unexpected,
-};
+pub fn QueryObjectName(
+    Handle: HANDLE,


Lowercase parameter name.

LemonBoy · 2020-12-10T16:34:46Z

lib/std/os/windows.zig

+pub fn QueryObjectName(
+    Handle: HANDLE,
+    out_buffer: []u16,
+    ReturnLength: ?PULONG, //returned in bytes


Lowercase parameter name.

LemonBoy · 2020-12-10T16:41:14Z

lib/std/os/windows.zig

+    var info = @ptrCast(*OBJECT_NAME_INFORMATION, aligned_raw_buffer);
+    info.* = undefined; //all of the fields we read will be overwritten, and the result string is placed in the same buffer after the struct
+    const out_buffer_length = std.math.cast(ULONG, aligned_raw_buffer.len) catch |e| std.math.maxInt(u32); //we'll just use as much of it as we can
+    const rc = ntdll.NtQueryObject(Handle, .ObjectNameInformation, out_buffer.ptr, out_buffer_length, ReturnLength); //buffer size is specified in u16...


The code is extremely hairy, it takes an output buffer out_buffer where the pathname is stored and uses it to allocate a OBJECT_NAME_INFORMATION too.

I'd simply allocate a buffer that's hopefully large enough (path_max plus the OBJECT_NAME_INFORMATION overhead) and copy the result over.

Sure, I'll change that. It just seemed more minimalist / less intrusive to directly use the provided buffer.

LemonBoy · 2020-12-10T16:41:49Z

lib/std/os/windows.zig

+    info.* = undefined; //all of the fields we read will be overwritten, and the result string is placed in the same buffer after the struct
+    const out_buffer_length = std.math.cast(ULONG, aligned_raw_buffer.len) catch |e| std.math.maxInt(u32); //we'll just use as much of it as we can
+    const rc = ntdll.NtQueryObject(Handle, .ObjectNameInformation, out_buffer.ptr, out_buffer_length, ReturnLength); //buffer size is specified in u16...
+    switch(rc){


The missing spaces hint you're not using zig fmt, please run it on all the files this PR is touching.

LemonBoy · 2020-12-10T16:42:52Z

lib/std/os/windows.zig

+    if (comptime builtin.os.tag != .windows)
+        return;
+
+    const file: std.fs.File = try std.fs.openSelfExe(.{}); //any file will do; canonicalization works on NTFS junctions and symlinks, hardlinks remain separate paths.


Redundant types are redundant.
And please keep the comments in a line above the one they refer to, not everyone has a huge monitor.

Suggested change

const file: std.fs.File = try std.fs.openSelfExe(.{}); //any file will do; canonicalization works on NTFS junctions and symlinks, hardlinks remain separate paths.

//any file will do; canonicalization works on NTFS junctions and symlinks, hardlinks remain separate paths.

const file = try std.fs.openSelfExe(.{});

LemonBoy · 2020-12-10T16:53:14Z

lib/std/os/windows.zig

+    const S = struct {fn check(file: std.fs.File, buffer: anytype, fmt: std.os.windows.GetFinalPathNameByHandleFormat) !usize {
+        const real = try std.os.windows.GetFinalPathNameByHandle(file.handle, fmt, buffer[0..]);
+        const offset = @divExact((@ptrToInt(real.ptr) - @ptrToInt(@ptrCast([*]u16, buffer))), 2);
+        return real.len + offset;


What's this offset for? Why do you care about what's before real.ptr?

LemonBoy · 2020-12-10T16:54:42Z

lib/std/os/windows.zig

+    const dos_length = try S.check(file, buffer[0..], .{.volume_name = .Dos});
+
+    //check with size insufficient by more than 1 character: always errors
+    std.testing.expect(null == S.check(file, buffer[0..nt_length-1], .{.volume_name = .Nt})


Suggested change

std.testing.expect(null == S.check(file, buffer[0..nt_length-1], .{.volume_name = .Nt})

std.testing.expectEqual(null, S.check(file, buffer[0..nt_length-1], .{.volume_name = .Nt})

Ditto everywhere else, this way if the check fails it'll show you the faulty value.

LemonBoy · 2020-12-10T16:55:16Z

lib/std/os/windows.zig

+    //check with size insufficient for null terminator: errors only on NtQueryObject path
+    const queryObjectPath = !std.os.windows.runtimeVersionIsAtLeast(std.os.windows.WindowsVersion.win10_rs4);
+    std.testing.expect(queryObjectPath == (null == S.check(file, buffer[0..nt_length], .{.volume_name = .Nt})
+        catch |e| switch(e) {error.NameTooLong => null, else => unreachable}));


There's testing.expectError for this.

LemonBoy · 2020-12-10T16:56:18Z

lib/std/os/windows.zig

+
+    switch (std.os.windows.ntdll.RtlGetVersion(&version_info)) {
+        .SUCCESS => {},
+        else => |e| { //only return value STATUS_SUCCESS is documented


Suggested change

else => |e| { //only return value STATUS_SUCCESS is documented

else => unreachable

This API cannot fail according to the docs.

LemonBoy · 2020-12-10T16:58:46Z

lib/std/os/windows.zig

+
+/// Returns whether the runtime os version is at least as new as the argument.
+pub fn runtimeVersionIsAtLeast(requested_version: WindowsVersion) bool {
+    return if(targetVersionIsAtLeast(requested_version)) |fully| fully


Suggested change

return if(targetVersionIsAtLeast(requested_version)) |fully| fully

return if(targetVersionIsAtLeast(requested_version)) orelse

@enumToInt(detectRuntimeVersion()) >= @enumToInt(requested_version);

rohlem · 2020-12-10T18:16:23Z

Thanks for the quick review, I might only get to it tomorrow though.

lib/std/os/windows.zig

lib/std/zig/system.zig

marler8997 · 2020-12-14T21:28:32Z

Just wanted to say thank you to @rohlem for showing windows some love, it desperately needs it.

…patibility The NtQueryInformationFile with .FileNormalizedNameInformation is only available in Windows 10 1803 (rs4) and later, however there is probably still another route we can go via ntdll.

Removes the call to kernel32.GetFinalPathNameByHandleW in favor of NtQueryObject, which means we can reuse the other codepath's logic for DOS naming.

rohlem · 2020-12-17T15:42:02Z

Rebased onto master and integrated all review comments, sorry it took so long. Edit: forgot to un-draft this earlier.

LemonBoy · 2020-12-17T18:42:57Z

lib/std/os/windows.zig

+    //last argument would return the length required for full_buffer, not exposed here
+    const rc = ntdll.NtQueryObject(handle, .ObjectNameInformation, full_buffer[0..], full_buffer_length, null);
+    return switch (rc) {
+        .SUCCESS => if (@ptrCast(?[*]WCHAR, info.Name.Buffer)) |buffer| blk: {


The if (x) |y| else return k; pattern can be rewritten as y = x orelse return k;, you can avoid the whole if.

This said I wouldn't expect Buffer to be null if NtQueryObject returns SUCCESS.
If you want to be extra-safe add a if (info.Name.Length == 0) return &[_]u16{};

The documentation for ObQueryNameString , which I based this on since it seems to be used under the hood, mentions a NULL value under "Remarks" (If the given object is unnamed [...]). So it's not just "being extra-safe", it's documented in the API.
Returning an empty string would require checking for that in the consumer code (GetFinalPathNameByHandle). The object's name isn't the empty string in this case, it has no name - I think either returning an optional or error still makes more sense, but please tell me if you think differently.

info.MaxLength is also set to 0 in that case, so I'm happy to change the check to that - is that an actual improvement in that case though? That is, either

if (info.Name.MaxLength == 0) error.Unexpected else blk: { const buffer = info.Name.Buffer; //... break :blk ...; }

or

blk: { const buffer = if(info.Name.MaxLength > 0) info.Name.Buffer else return error.Unexpected; //... break :blk ...; }

I'm still on the fence with the idea of having error.Unexpected. If something like this happens, shouldn't we simply trigger a nonrecoverable runtime trap instead? Or do we intend for user-space code to be recoverable in those circumstances? If that's the case, anyone's got any ideas for a scenario when that would be the case?

"nonrecoverable runtime trap" (so basically @panic?) sounds very cut-throat to me.
If the file was supplied by a user, say interactively via stdin, maybe you just want to notify the user "this file didn't work, try another / moving it to a different path" (or something more specific if you figure out the underlying problem).
To me this seems like a fitting scenario for an error code: We failed to provide the requested service, but if/how to recover is up to the caller. That's why I made it return error.Unnamed before.
If we don't expect this API to be used with unnamed objects, then this scenario is "unexpected". error.Unexpected is not inherently reserved for kernel failures, so I think it fits.

But these are just my thoughts, whatever you think is best I'll implement.

LemonBoy · 2020-12-17T18:45:08Z

lib/std/os/windows.zig

+    var full_buffer: [@sizeOf(OBJECT_NAME_INFORMATION) + PATH_MAX_WIDE * 2]u8 align(@alignOf(OBJECT_NAME_INFORMATION)) = undefined;
+    var info = @ptrCast(*OBJECT_NAME_INFORMATION, &full_buffer);
+    //buffer size is specified in bytes
+    const full_buffer_length = @intCast(ULONG, @sizeOf(OBJECT_NAME_INFORMATION) + std.math.min(PATH_MAX_WIDE, (out_buffer.len + 1) * 2));


I'd simply use PATH_MAX_WIDE, the NameTooLong check should catch the problem below.

Nit: std.math -> math, it's defined at the top of this file.

Hmm, you're right, though now that I look at it again, with this exact size specification we could conversely remove the size check below, which would be more optimal.
But I guess the explicit check below is better in the sense of readability?

LemonBoy · 2020-12-17T18:47:25Z

lib/std/os/windows.zig

+    //buffer size is specified in bytes
+    const full_buffer_length = @intCast(ULONG, @sizeOf(OBJECT_NAME_INFORMATION) + std.math.min(PATH_MAX_WIDE, (out_buffer.len + 1) * 2));
+    //last argument would return the length required for full_buffer, not exposed here
+    const rc = ntdll.NtQueryObject(handle, .ObjectNameInformation, full_buffer[0..], full_buffer_length, null);


Suggested change

const rc = ntdll.NtQueryObject(handle, .ObjectNameInformation, full_buffer[0..], full_buffer_length, null);

const rc = ntdll.NtQueryObject(handle, .ObjectNameInformation, &full_buffer, full_buffer.len, null);

LemonBoy · 2020-12-17T18:49:21Z

lib/std/os/windows.zig

+            if (out_buffer.len < path_length_unterminated) {
+                return error.NameTooLong;
+            }
+            std.mem.copy(WCHAR, out_buffer[0..path_length_unterminated], buffer[0..path_length_unterminated :0]);


The :0 is not needed and according to the docs may be harmful (and trip the missing sentinel check).

Minor nit, std.mem. -> mem.

Huh, good eye. Strange to document that with the data structure, but I guess that means it could apply anywhere it's used...

LemonBoy · 2020-12-17T18:50:37Z

lib/std/os/windows.zig

+    const file = try std.fs.openSelfExe(.{});
+    defer file.close();
+    //make this large enough for the test runner exe path
+    var out_buffer align(16) = std.mem.zeroes([1 << 10]u16);


That's a huge alignment factor! Is that really needed?
And 1 << 10 => PATH_MAX_WIDE ?

Right, that is a leftover from earlier.
And sure, if we have the stack space for another PATH_MAX_WIDE that's fine by me.

LemonBoy · 2020-12-17T18:57:10Z

lib/std/os/windows.zig

+            if (out_buffer.len < final_path.len) {
+                return error.NameTooLong;
+            }
+            std.mem.copy(u16, out_buffer[0..], final_path[0..]);


Suggested change

std.mem.copy(u16, out_buffer[0..], final_path[0..]);

std.mem.copy(u16, &out_buffer, &final_path);

LemonBoy · 2020-12-17T18:59:34Z

lib/std/os/windows.zig

+                return error.NameTooLong;
+            }
+            std.mem.copy(u16, out_buffer[0..], final_path[0..]);
+            return final_path; //we can directly return the slice we received


final_path comes from QueryObjectName with path_buffer as input and that's stack-allocated.

Ah, right, meant out_buffer of course, thanks.

LemonBoy · 2020-12-17T19:00:34Z

lib/std/os/windows.zig

-            const volume_name_u16 = @ptrCast([*]const u16, &volume_name.FileName[0])[0 .. volume_name.FileNameLength / 2];
+        //otherwise we need to parse the string for volume path for the .Dos logic below to work
+        const expected_prefix = std.unicode.utf8ToUtf16LeStringLiteral("\\Device\\");
+        if (!std.mem.eql(u16, expected_prefix, final_path[0..expected_prefix.len])) {


Given this is a precondition we expect the kernel to respect, an assert is fine.

Please use mem instead of std.mem!

Well, I'm not knowledgeable enough about these NT paths to say whether the returned string always starts with \Device\, it's just the only scenario I've encountered, but I don't have any network sharing set up f.e. .

EDIT: I misread the code below, error.BadPathName is more likely to come from the file path than the DOS volume name we resolved.
So I'll change it back to an assert here.

Rethinking it yet again, the way we just blindly assume the prefix could lead to nonsensical results in unsafe build modes (i.e. returning a path to a different file).
An error return makes much more sense to me. But speak up if you feel differently.

LemonBoy · 2020-12-17T19:02:15Z

lib/std/os/windows.zig

+        volume_name_u16 = final_path[0..index];
+        file_name_u16 = final_path[index..];
+
+        //fallthrough for fmt.volume_name != .Nt


fallthrough? The other case is handled in the other branch.

No, the "other branch" (the else at line 1065 of the if at line 1037) is for the newer windows version... unless I'm missing something?

LemonBoy · 2020-12-17T19:04:41Z

lib/std/os/windows.zig

+        const volume_name_info = @ptrCast(*const FILE_NAME_INFORMATION, &volume_buffer[0]);
+        volume_name_u16 = @ptrCast([*]const u16, &volume_name_info.FileName[0])[0..@divExact(volume_name_info.FileNameLength, 2)];

+        if (fmt.volume_name == .Nt) {


This is in the (fmt.volume_name != .Nt) branch, it'll never be executed.

No, this is the win10_rs4 or newer branch, which is the previous code I kept intact.

Mouvedia · 2020-12-17T23:15:43Z

lib/std/target.zig

+                17134, //win_rs4
+                17763, //win_rs5
+                18362, //win_19h1
+                19041, //win_20h1


You forgot 2 builds:

18363, //win_19h2 19042, //win_20h2

cf 140c599#r44615067

I thought so at first too, but those don't impact the NTDDI header constant we're trying to mirror here. (Also the values are unchanged from the previous implementation.)

This page is dated 2020-06-19, 19042 was released the 2020-10-20 and on the β channel the 2020-06-16.
i.e. it's outdated

Okay, it seems that you're right, this repository's own sdkddkver.h already includes more constants than I found when I was looking for them earlier. Thank you for pointing this out!

FYI the next one will probably be 20231 (21H1)

see https://github.com/hfiref0x/SyscallTables

andrewrk

Thank you for taking on this project, @rohlem. This is good stuff. Needs a bit of cleanup, but the important parts are in place.

If we trust this new implementation, I would be inclined to maybe delete the other codepath (that needs two ntdll calls instead of just one) in favor of this one. (And maybe move the logic for finding DOS volume names into its own function.)
But now that I've worked on this for a day, I thought I would ask for some feedback, to make sure I'm not overlooking anything crucial.

If it works empirically (which it sounds like it does) then I would be in favor of deleting the other codepath and deleting the version checking.

andrewrk · 2020-12-19T02:04:41Z

lib/std/os/windows/bits.zig

 pub const TCHAR = if (UNICODE) WCHAR else u8;
 pub const UINT = c_uint;
 pub const ULONG_PTR = usize;
+pub const PULONG = *ULONG;


Prefer to use *ULONG directly rather than PULONG

Yeah, Zig has more "specific" ways of identifying pointers *ULONG vs [*]ULONG. PULONG is more ambiguous.

andrewrk · 2020-12-19T02:12:13Z

lib/std/os/windows.zig

+//version detection
+usingnamespace std.zig.system.windows;


Prefer to avoid usingnamespace unless it's to set up the public declarations (as above). Here we should give it a name and refer to the functions therein via field access syntax.

andrewrk · 2020-12-19T02:17:17Z

lib/std/os/windows.zig

+    const full_buffer_length = @intCast(ULONG, @sizeOf(OBJECT_NAME_INFORMATION) + std.math.min(PATH_MAX_WIDE, (out_buffer.len + 1) * 2));
+    //last argument would return the length required for full_buffer, not exposed here
+    const rc = ntdll.NtQueryObject(handle, .ObjectNameInformation, full_buffer[0..], full_buffer_length, null);
+    return switch (rc) {


When returning errors, prefer to put return on multiple lines. This will make the error return trace point nicely to the error code that triggered the crash, rather than pointing to return switch (rc) {.

andrewrk · 2020-12-19T02:20:02Z

lib/std/os/windows.zig

+    handle: HANDLE,
+    out_buffer: []u16,
+) ![]u16 {
+    var full_buffer: [@sizeOf(OBJECT_NAME_INFORMATION) + PATH_MAX_WIDE * 2]u8 align(@alignOf(OBJECT_NAME_INFORMATION)) = undefined;


Why 2 buffers with a memcpy? We could pass out_buffer directly to NtQueryObject right?

Yes, that's what the code did originally, which was commented on here, so I changed it.
I can change it back if you want - I don't think there was a particular reason besides readability - but I don't like stepping on anyone's toes, so I'll put that in as the last commit and then we can pick whichever turns out looking nicer.

andrewrk · 2020-12-19T02:22:08Z

lib/std/os/windows.zig

+        return;
+
+    //any file will do; canonicalization works on NTFS junctions and symlinks, hardlinks remain separate paths.
+    const file = try std.fs.openSelfExe(.{});


Better not to entangle this test with the functionality of openSelfExe. I suggest to use std.testing.tmpDir. You can see example usage in std/fs/test.zig.

andrewrk · 2020-12-19T02:23:28Z

lib/std/os/windows.zig

+    const file = try std.fs.openSelfExe(.{});
+    defer file.close();
+    //make this large enough for the test runner exe path
+    var out_buffer align(16) = std.mem.zeroes([1 << 10]u16);


what is the purpose of calling mem.zeroes here?

I think that was a "sane default" during early testing that I forgot to remove. The code shouldn't rely on the value, I'll change it to undefined.

andrewrk · 2020-12-19T02:26:24Z

lib/std/os/windows.zig

+        };

-    const volume_name = @ptrCast(*const FILE_NAME_INFORMATION, &volume_buffer[0]);
+        if (fmt.volume_name == .Nt) {


Prefer to switch on enums rather than if

Side note @kubkon - why is GetFinalPathNameByHandleFormat a struct containing an enum, and not just an enum?

andrewrk · 2020-12-19T02:29:35Z

lib/std/os/windows.zig

+    switch (fmt.volume_name) {
+        .Nt => unreachable, //handled above


This construct can be replaced with assert(fmt.volume_name == .Nt);

andrewrk · 2020-12-19T02:31:25Z

lib/std/zig/system/windows.zig

+pub fn targetVersionIsAtLeast(requested_version: WindowsVersion) ?bool {
+    const requested = @enumToInt(requested_version);
+    const version_range = std.builtin.os.version_range.windows;
+    const target_min = @enumToInt(version_range.min);
+    const target_max = @enumToInt(version_range.max);
+    return if (target_max < requested) false else if (target_min >= requested) true else null;
+}


This looks like duplicated code from std.Target.Os.isAtLeast

kubkon · 2020-12-19T10:32:27Z

I should probably explain what the motivation was for using FileNormalizedNameInformation instead of simply resorting to NtQueryObject. When I originally had a stab at this, I did a series of tests and comparisons to see how Windows user-level APIs actually do it using the Process Monitor to track the syscalls. It turned out that on my version of Win10, GetFinalPathNameByHandleW was calling NtQueryInformationFile with FileNormalizedNameInformation file information class. My guess was that this way we wouldn't have to worry about the maximum path name limitation, i.e., that the newer API would handle this for us. Having said that, it is true that ReactOS uses exclusively NtQueryObject to implement their version of GetFinalPathNameByHandleW. Also, I don't like the fact we have to check Windows version every time we call GetFinalPathNameByHandle. On the other hand, @daurnimator reassures us that it should be a pretty cheap call, so maybe this won't be a huge performance hog, but I'm not an expert here by any means. Anyhow, I'm happy if we remove the original alternate path and use NtQueryObject exclusively. If this poses some problems to do with path length limits down the line, we can always bring it back or come up with a better solution.

…w comments

…code path

... and mem.copy operations. Requires slightly larger input buffers than result length. Add helper functions std.mem.alignInBytes and std.mem.alignInSlice.

rohlem · 2020-12-23T21:29:35Z

Addressed all review comments, besides the one suggested error -> assert I commented on - that looks safer as an error to me (not blindly continuing in non-debug build modes).
The last commit removes all intermediate buffers, as @andrewrk suggested (... iiuc), to make the code more readable I added a helper function std.mem.alignInSlice to do the necessary in-place-alignment.

LemonBoy · 2020-12-24T10:11:08Z

After 83 comments it looks good now!

The last commit removes all intermediate buffers, as @andrewrk suggested (... iiuc), to make the code more readable I added a helper function std.mem.alignInSlice to do the necessary in-place-alignment.

The helper function makes the code much easier to read and review, thanks.

Merges #7379

andrewrk · 2021-01-12T00:50:44Z

Landed in 56c0388

rohlem force-pushed the fix-GetFinalPathNameByHandle-before-win10_rs4 branch 2 times, most recently from 0217461 to 7539116 Compare December 10, 2020 16:20

LemonBoy reviewed Dec 10, 2020

View reviewed changes

rohlem marked this pull request as draft December 10, 2020 18:16