[PATCH 0/1] MR6535: ntdll: Set correct MaximumLength when current directory path is longer than MAX_PATH.
Calls to `CreateProcessW` where the current directory is a path that exceeds `MAX_PATH` can result in `Length` exceeding `MaximumLength`, causing corruption in `alloc_process_params` when `append_unicode_string` copies the string according to `Length`, but only allocates `MaximumLength` of storage. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6535
From: Owen Rudge <orudge(a)codeweavers.com> --- dlls/ntdll/env.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ntdll/env.c b/dlls/ntdll/env.c index 720597dcaf9..e0fd75762ec 100644 --- a/dlls/ntdll/env.c +++ b/dlls/ntdll/env.c @@ -614,7 +614,7 @@ NTSTATUS WINAPI RtlCreateProcessParametersEx( RTL_USER_PROCESS_PARAMETERS **resu curdir = cur_params->CurrentDirectory.DosPath; } else curdir = *CurrentDirectoryName; - curdir.MaximumLength = MAX_PATH * sizeof(WCHAR); + curdir.MaximumLength = max(curdir.Length, MAX_PATH * sizeof(WCHAR)); if (!CommandLine) CommandLine = ImagePathName; if (!Environment && cur_params) Environment = cur_params->Environment; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6535
Jinoh Kang (@iamahuman) commented about dlls/ntdll/env.c:
curdir = cur_params->CurrentDirectory.DosPath; } else curdir = *CurrentDirectoryName; - curdir.MaximumLength = MAX_PATH * sizeof(WCHAR); + curdir.MaximumLength = max(curdir.Length, MAX_PATH * sizeof(WCHAR));
Hello! I see that this is the second time contributing to ntdll. Congrats! In case you're not familiar with `UNICODE_STRING`, the `MaximumLength` should be at least `Length + 2`. So I guess the correct code should read: ```suggestion:-0+0 curdir.MaximumLength = max(curdir.Length + 2, MAX_PATH * sizeof(WCHAR)); ``` However, I'm not sure if DOS paths longer than 260 characters are even valid. We need to test how Windows behaves in this case (does it truncate? does it use a short name? does it extend MaximumLength like you did?) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6535#note_82737
On Fri Sep 20 10:13:56 2024 +0000, Jinoh Kang wrote:
Hello! I see that this is the second time contributing to ntdll. Congrats! In case you're not familiar with `UNICODE_STRING`, the `MaximumLength` should be at least `Length + 2`. So I guess the correct code should read: ```suggestion:-0+0 curdir.MaximumLength = max(curdir.Length + 2, MAX_PATH * sizeof(WCHAR)); ``` However, I'm not sure if DOS paths longer than 260 characters are even valid. We need to test how Windows behaves in this case (does it truncate? does it use a short name? does it extend MaximumLength like you did?) It would seem that on Windows 10, the maximum length is 32,767 characters, but that requires a registry setting plus each application having a flag present in the application manifest:
https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-lim... I suspect we're not interested in implementing this same behaviour at the moment. It's certainly not guaranteed that Wine will be happy with it in all places (or indeed the underlying OS - macOS having a practical limit of around 1024 characters), but the application I was testing seems to work OK with longer paths with these fixes applied. The behaviour as it stands is definitely wrong though. I'm happy to go with your suggestion regarding `+ 2`. I don't know how easy it would be to write a useful test for this; I have no idea if Windows stores the parameters in `Environment` in the same way as Wine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6535#note_82738
On Fri Sep 20 10:29:27 2024 +0000, Owen Rudge wrote:
It would seem that on Windows 10, the maximum length is 32,767 characters, but that requires a registry setting plus each application having a flag present in the application manifest: https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-lim... I suspect we're not interested in implementing this same behaviour at the moment. It's certainly not guaranteed that Wine will be happy with it in all places (or indeed the underlying OS - macOS having a practical limit of around 1024 characters), but the application I was testing seems to work OK with longer paths with these fixes applied. The behaviour as it stands is definitely wrong though. I'm happy to go with your suggestion regarding `+ 2`. I don't know how easy it would be to write a useful test for this; I have no idea if Windows stores the parameters in `Environment` in the same way as Wine. A (regression) test in kernel32:process that shows that current directory longer than 260 chars is preserved should be sufficient.
1. Launch itself with new curdir as well as extra argvs (e.g., `pwdchk $NEWDIR`) 2. In main (START\_TEST), if the extra argv is detected, test that GetCurrentDirectoryW() result matches the given `$NEWDIR`. 3. Call `wait_child_process()` on the process handle, and clean up appropriately. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6535#note_82910
participants (3)
-
Jinoh Kang (＠iamahuman) -
Owen Rudge
-
Owen Rudge (＠orudge)