Skip to content
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

Windows: Deal with NT namespaced paths in GetFinalPathNameByHandle #17541

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

moosichu
Copy link
Contributor

This is an attempted fix for #17535, will validate that once the windows artifacts are generated for the tests for this PR and run them on my machine.

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.

I don't think there should be Wine-specific workarounds in Zig.

Perhaps you could resolve that TODO comment there in a general way that is not Wine-specific and makes the code more correct, and also happens to solve the problem when using Wine?

lib/std/os/windows.zig Outdated Show resolved Hide resolved
@andrewrk
Copy link
Member

andrewrk commented Oct 15, 2023

Please rename the title of the PR too, to help with release notes :) (make it not sound like a wine-specific workaround)

@moosichu moosichu force-pushed the fix/wine-get-final-path-name-by-handle branch 2 times, most recently from 333e1a7 to 21837c9 Compare October 15, 2023 20:38
@moosichu moosichu changed the title Deal with wine paths in GetFinalPathNameByHandle Deal with NT namespaced paths in GetFinalPathNameByHandle Oct 15, 2023
@moosichu moosichu force-pushed the fix/wine-get-final-path-name-by-handle branch from 21837c9 to 3aa302c Compare October 15, 2023 20:41
@moosichu
Copy link
Contributor Author

moosichu commented Oct 15, 2023

I don't think there should be Wine-specific workarounds in Zig.

Perhaps you could resolve that TODO comment there in a general way that is not Wine-specific and makes the code more correct, and also happens to solve the problem when using Wine?

Yeah, I completely agree with this! Hopefully @squeek502's solution is that. My initial goal was to sanity check that the solution could work/was on the right path by (ab)using the PR CI checks to generate a windows zig build for me to run zig on my machine (a mac with a linux vm haha).

Mainly because I don't (currently) have access to a Windows machine to build zig myself.

Thank you so much @squeek502 for the solution!

Please rename the title of the PR too, to help with release notes :) (make it not sound like a wine-specific workaround)

I've done my best on this front! (again, can't stress enough how little I understand about the underlying problem. Just came across it and tried to hack together a workaround before a proper investigation).

@moosichu
Copy link
Contributor Author

In terms of sources for this change, looking at the documention for NtQueryObject, there's nothing useful:
https://learn.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntqueryobject

Indeed... it doesn't even list ObjectNameInformation as one of the OBJECT_INFORMATION_CLASS values!

However, looking at the wine function, it does explicitly call unix_to_nt_file_name and returns that as the path. Sadly there's no documentation here explaining the logic or reasoning behind the decisions for the implementation of that function.

https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/ntdll/unix/file.c#L7062

@moosichu moosichu force-pushed the fix/wine-get-final-path-name-by-handle branch from 3aa302c to a52a3ca Compare October 15, 2023 21:21
@moosichu moosichu requested a review from andrewrk October 15, 2023 21:21
@moosichu
Copy link
Contributor Author

I've tried to edit that TODO because it wasn't really an action... as far as I can find at least there's no easy way to validate we have handled the general case. But hopefully it still keeps things clear for people looking as to where things might have gone wrong if they come across an unhandled case (as it did for me).

lib/std/os/windows.zig Outdated Show resolved Hide resolved
@moosichu moosichu force-pushed the fix/wine-get-final-path-name-by-handle branch from a52a3ca to aaccbf5 Compare October 16, 2023 08:25
@moosichu moosichu changed the title Deal with NT namespaced paths in GetFinalPathNameByHandle Windows: Deal with NT namespaced paths in GetFinalPathNameByHandle Oct 17, 2023
lib/std/os/windows.zig Outdated Show resolved Hide resolved
lib/std/os/windows.zig Outdated Show resolved Hide resolved
lib/std/os/windows.zig Outdated Show resolved Hide resolved
@moosichu moosichu force-pushed the fix/wine-get-final-path-name-by-handle branch 2 times, most recently from 90f76c8 to d1ae96d Compare October 19, 2023 08:31
@moosichu moosichu requested a review from andrewrk October 19, 2023 08:32
@moosichu
Copy link
Contributor Author

Thanks for taking the time to review the changes! I've implemented all the feedback (hopefully in the right way).

lib/std/os/windows.zig Outdated Show resolved Hide resolved
@moosichu moosichu force-pushed the fix/wine-get-final-path-name-by-handle branch from d1ae96d to 4957f7a Compare October 19, 2023 09:02
@andrewrk andrewrk requested a review from squeek502 May 9, 2024 23:11
Copy link
Collaborator

@squeek502 squeek502 left a comment

Choose a reason for hiding this comment

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

I think this is a good stop-gap solution if nothing else. It might be Wine-specific, but as the added comment says, we don't have a full understanding of the possible returns from NtQueryObject with OBJECT_NAME_INFORMATION. Additionally, because NtQueryObject just gives us back a string (without any possible input controlling what form the path should be returned as, and no indication in the return value what form the path is in), it is possible that Windows can return a NT-prefixed (\??\) path here as well in some situtations.

In the future, it would be good to test GetFinalPathNameByHandle/QueryObjectName on as many different filesystems/path types/file types/etc as possible to get a better understanding of what's possible in different environments.

@The-King-of-Toasters
Copy link
Contributor

The-King-of-Toasters commented May 11, 2024

I could whip up a test program for all the NT objects, but another solution is this program from @‍zodiacon's book Windows Native API Programming. I can shoot you an exe but to build it yourself:

  1. Clone this fork of the phnt headers.
  2. Clone the book repo.
  3. Apply the patch below.
  4. In a VS dev shell with msbuild installed, run msbuild /p:Configuration=Release /p:IncludePath='path\to\phnt_nightly'.\Chapter04\Chapter04.sln
  5. Run Chapter04\[arch]\Release\Handles.exe in an elevated command prompt.
diff --git a/Chapter04/Handles/Handles.cpp b/Chapter04/Handles/Handles.cpp
index f3a2bea..8c58f7c 100644
--- a/Chapter04/Handles/Handles.cpp
+++ b/Chapter04/Handles/Handles.cpp
@@ -38,7 +38,7 @@ NTSTATUS GetObjectName(HANDLE h, DWORD pid, std::wstring& name, bool file) {
 			//
 			HANDLE hThread;
 			status = NtCreateThreadEx(&hThread, THREAD_ALL_ACCESS, &procAttr, NtCurrentProcess(),
-				(PTHREAD_START_ROUTINE)[](auto param) -> DWORD {
+				(PUSER_THREAD_START_ROUTINE)[](auto param) -> NTSTATUS {
 					auto h = (HANDLE)param;
 					auto status = NtQueryObject(h, ObjectNameInformation, buffer, sizeof(buffer), nullptr);
 					return status;
@@ -97,18 +97,15 @@ std::wstring const& GetObjectTypeName(USHORT index) {
 
 void DisplayHandle(SYSTEM_HANDLE_TABLE_ENTRY_INFO_EX const& h) {
 	auto const& type = GetObjectTypeName(h.ObjectTypeIndex);
-	printf("PID: %6u H: 0x%X Access: 0x%08X Att: %s Address: 0x%p Type: %ws",
-		(ULONG)h.UniqueProcessId, (ULONG)h.HandleValue, h.GrantedAccess,
-		HandleAttributesToString(h.HandleAttributes).c_str(),
-		h.Object, type.c_str());
 	std::wstring name;
 	auto status = GetObjectName(ULongToHandle((ULONG)h.HandleValue), (ULONG)h.UniqueProcessId, name, type == L"File");
-	if (NT_SUCCESS(status) && !name.empty())
-		printf(" Name: %ws\n", name.c_str());
-	else if (status == STATUS_ACCESS_DENIED)
-		printf(" Name: <access denied>\n");
-	else
-		printf("\n");
+	if(!NT_SUCCESS(status) || name.empty())
+		return;
+
+	printf("PID: %6u H: 0x%X Access: 0x%08X Att: %s Address: 0x%p Type: %ws Name: %ws\n",
+		(ULONG)h.UniqueProcessId, (ULONG)h.HandleValue, h.GrantedAccess,
+		HandleAttributesToString(h.HandleAttributes).c_str(),
+		h.Object, type.c_str(), name.c_str());
 }
 
 void EnumHandles(DWORD pid, PCWSTR type) {

At least on my system it seems that the returned names don't have a namespace prefix. I need to reboot to linux to see what wine returns for this program. UPDATE: After opening some programs/files in Wine it seems that the Device type is the only one that returns a \??\ prefix, which is strange as there aren't Device objects being logged on Windows 11.

@The-King-of-Toasters
Copy link
Contributor

The-King-of-Toasters commented May 13, 2024

@squeek502 I made made a test program to see the results of NtQueryObject on different objects, and the only one with a prefix are filesystem objects (i.e. not a named pipe or others). I think this confirms that Wine's behaviour is different.

@andrewrk andrewrk force-pushed the fix/wine-get-final-path-name-by-handle branch from 4957f7a to 83732ed Compare August 24, 2024 06:05
@andrewrk andrewrk enabled auto-merge (rebase) August 24, 2024 06:05
@alexrp alexrp disabled auto-merge October 10, 2024 12:15
When calling QueryObjectName, NT namespaced paths can be returned. This
change appropriately strips the prefix to turn it into an absolute path.

(The above behaviour was observed at least in Wine so far)

Co-authored-by: Ryan Liptak <[email protected]>
@alexrp alexrp force-pushed the fix/wine-get-final-path-name-by-handle branch from 83732ed to d52f9b7 Compare October 10, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants