[PATCH 0/2] MR2578: Fixing module paths in LdrPath (corner case).
The serie intent is to fix unexpected paths in module's list: This happens: - when running under old / (new) wow64 - when main module is located under the syswow64 directory - the 32 bit modules are stored in LdrData (and then exposed through a couple of ways) under syswow64 (they are normally stored under system32, letting the redirection come into play when needed) This triggers a couple of errors in winetest (as we're using c:\windows\syswow64\msinfo32.exe in many tests to trigger a wow64 process from a winetest program). This is the fix awaited in MR!2497. @julliard: I'm not 100% happy with the fix itself by reintroducting ref to the redirected DLLs in ntdll/PE but couldn't find a better idea. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2578
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/kernel32/tests/loader.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 19f112efcd8..ea66e997ea4 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -4049,8 +4049,9 @@ static void test_InMemoryOrderModuleList(void) static void test_wow64_redirection_for_dll(const char *libname) { - HMODULE lib; + HMODULE lib, lib2; char buf[256]; + DWORD ret; if (!GetModuleHandleA(libname)) { @@ -4060,12 +4061,16 @@ static void test_wow64_redirection_for_dll(const char *libname) if (lib) { /* Win 7/2008R2 return the un-redirected path (i.e. c:\windows\system32\dwrite.dll), test loading it. */ - GetModuleFileNameA(lib, buf, sizeof(buf)); + ret = GetModuleFileNameA(lib, buf, sizeof(buf)); + ok(ret, "Expecting to get the module name\n"); + lib2 = LoadLibraryExA(buf, NULL, 0); + ok(lib2 != NULL, "Loading %s from full path should succeed with WOW64 redirection disabled\n", libname); + if (lib2) + { + ok(lib == lib2, "Shouldn't have reloaded another module for %s\n", libname); + FreeLibrary(lib2); + } FreeLibrary(lib); - lib = LoadLibraryExA(buf, NULL, 0); - ok(lib != NULL, "Loading %s from full path should succeed with WOW64 redirection disabled\n", libname); - if (lib) - FreeLibrary(lib); } } else -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2578
From: Eric Pouech <epouech(a)codeweavers.com> The code in ntdll/loader delegates redirection handling to the unix part. So keep it that way. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/dbghelp/tests/dbghelp.c | 7 +----- dlls/ntdll/loader.c | 42 +++++++++++++++++++++++++++++++++-- dlls/psapi/tests/psapi_main.c | 2 -- 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/dlls/dbghelp/tests/dbghelp.c b/dlls/dbghelp/tests/dbghelp.c index 276faf0101a..d6cd1c9da70 100644 --- a/dlls/dbghelp/tests/dbghelp.c +++ b/dlls/dbghelp/tests/dbghelp.c @@ -719,7 +719,7 @@ static void test_loaded_modules(void) break; case PCSKIND_64BIT: case PCSKIND_WOW64: - todo_wine + case PCSKIND_WINE_OLD_WOW64: ok(aggregation.count_systemdir > 2 && aggregation.count_wowdir == 1, "Wrong directory aggregation count %u %u\n", aggregation.count_systemdir, aggregation.count_wowdir); break; @@ -727,10 +727,6 @@ static void test_loaded_modules(void) ok(aggregation.count_systemdir > 2 && aggregation.count_wowdir == 0, "Wrong directory aggregation count %u %u\n", aggregation.count_systemdir, aggregation.count_wowdir); break; - case PCSKIND_WINE_OLD_WOW64: - ok(aggregation.count_systemdir == 1 && aggregation.count_wowdir > 2, "Wrong directory aggregation count %u %u\n", - aggregation.count_systemdir, aggregation.count_wowdir); - break; } } @@ -767,7 +763,6 @@ static void test_loaded_modules(void) aggregation.count_32bit, aggregation.count_64bit); ok(aggregation.count_exe == 1 && aggregation.count_ntdll == 1, "Wrong kind aggregation count %u %u\n", aggregation.count_exe, aggregation.count_ntdll); - todo_wine ok(aggregation.count_systemdir > 2 && aggregation.count_64bit == aggregation.count_systemdir && aggregation.count_wowdir == 1, "Wrong directory aggregation count %u %u\n", aggregation.count_systemdir, aggregation.count_wowdir); diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index f05cd1b4fd9..fb274d572fe 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2358,20 +2358,55 @@ static inline WCHAR *append_path( WCHAR *p, const WCHAR *str, int len ) } +/* Helper for get_dll_load_path. Detect if a directories might be redirected. */ +static BOOL does_path_match_directory( const UNICODE_STRING *nt_name, const WCHAR *dir, SIZE_T dir_len) +{ + return nt_name->Length / sizeof(WCHAR) >= dir_len && + !wcsnicmp(nt_name->Buffer, dir, dir_len) && + (nt_name->Length / sizeof(WCHAR) == dir_len || nt_name->Buffer[dir_len] == '\\'); +} + +static BOOL shall_redirect_path( const WCHAR* name, SIZE_T len) +{ + static const WCHAR syswow64[] = {'\\','?','?','\\','C',':','\\','w','i','n','d','o','w','s','\\','s','y','s','w','o','w','6','4'}; + static const WCHAR sysarm32[] = {'\\','?','?','\\','C',':','\\','w','i','n','d','o','w','s','\\','s','y','s','a','r','m','3','2'}; + static const WCHAR sysx8664[] = {'\\','?','?','\\','C',':','\\','w','i','n','d','o','w','s','\\','s','y','s','x','8','6','6','4'}; + static const WCHAR sysarm64[] = {'\\','?','?','\\','C',':','\\','w','i','n','d','o','w','s','\\','s','y','s','a','r','m','6','4'}; + UNICODE_STRING nt_name; + WCHAR tmp[MAX_PATH]; + BOOL ret; + + if (sizeof(void*) > sizeof(int)) return FALSE; + if (len + 1 > ARRAY_SIZE(tmp)) return FALSE; + memcpy(tmp, name, len * sizeof(WCHAR)); + tmp[len] = L'\0'; + if (!RtlDosPathNameToNtPathName_U(tmp, &nt_name, NULL, NULL)) return FALSE; + + ret = does_path_match_directory( &nt_name, syswow64, ARRAY_SIZE(syswow64)) || + does_path_match_directory( &nt_name, sysarm32, ARRAY_SIZE(sysarm32)) || + does_path_match_directory( &nt_name, sysx8664, ARRAY_SIZE(sysx8664)) || + does_path_match_directory( &nt_name, sysarm64, ARRAY_SIZE(sysarm64)); + RtlFreeUnicodeString( &nt_name ); + return ret; +} + /****************************************************************** * get_dll_load_path */ static NTSTATUS get_dll_load_path( LPCWSTR module, LPCWSTR dll_dir, ULONG safe_mode, WCHAR **path ) { + static const WCHAR system32[] = {'C',':','\\','w','i','n','d','o','w','s','\\','s','y','s','t','e','m','3','2'}; const WCHAR *mod_end = module; UNICODE_STRING name = RTL_CONSTANT_STRING( L"PATH" ), value; WCHAR *p, *ret; int len = ARRAY_SIZE(system_path) + 1, path_len = 0; + BOOL redirect = FALSE; if (module) { mod_end = get_module_path_end( module ); - len += (mod_end - module) + 1; + redirect = shall_redirect_path( module, mod_end - module ); + len += (redirect ? ARRAY_SIZE(system32) : mod_end - module) + 1; } value.Length = 0; @@ -2385,7 +2420,10 @@ static NTSTATUS get_dll_load_path( LPCWSTR module, LPCWSTR dll_dir, ULONG safe_m if (!(p = ret = RtlAllocateHeap( GetProcessHeap(), 0, path_len + len * sizeof(WCHAR) ))) return STATUS_NO_MEMORY; - p = append_path( p, module, mod_end - module ); + if (redirect) + p = append_path( p, system32, ARRAY_SIZE(system32) ); + else + p = append_path( p, module, mod_end - module ); if (dll_dir) p = append_path( p, dll_dir, -1 ); else if (!safe_mode) p = append_path( p, L".", -1 ); p = append_path( p, system_path, -1 ); diff --git a/dlls/psapi/tests/psapi_main.c b/dlls/psapi/tests/psapi_main.c index 860598c39c5..3ec024e6dc9 100644 --- a/dlls/psapi/tests/psapi_main.c +++ b/dlls/psapi/tests/psapi_main.c @@ -491,12 +491,10 @@ static void test_EnumProcessModulesEx(void) ret = GetSystemWow64DirectoryA(buffer, sizeof(buffer)); ok(ret, "GetSystemWow64DirectoryA failed: %lu\n", GetLastError()); count = snapshot_count_in_dir(snap, pi.hProcess, buffer); - todo_wine ok(count <= 1, "Wrong count %u in %s\n", count, buffer); /* msinfo32 can be from system wow64 */ ret = GetSystemDirectoryA(buffer, sizeof(buffer)); ok(ret, "GetSystemDirectoryA failed: %lu\n", GetLastError()); count = snapshot_count_in_dir(snap, pi.hProcess, buffer); - todo_wine ok(count > 2, "Wrong count %u in %s\n", count, buffer); /* in fact, this error is only returned when (list & 3 == 0), otherwise the corresponding -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2578
I'm not 100% happy with the fix itself by reintroducting ref to the redirected DLLs in ntdll/PE but couldn't find a better idea.
Yes, that doesn't look right. There has to be a better way. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2578#note_28876
On Thu Apr 6 12:11:43 2023 +0000, Alexandre Julliard wrote:
I'm not 100% happy with the fix itself by reintroducting ref to the redirected DLLs in ntdll/PE but couldn't find a better idea. Yes, that doesn't look right. There has to be a better way. hmm...
some more tests show that loading from a wow64 process c:\windows\syswow64\mydll.dll (with absolute path) ends up with a path reported from system32 in LdrData chain (tested with regular system DLL, but also by adding a dummy DLL in a created subdir of syswow64 => same rewrite to system32 in LdrData) (so this rules out predefined list of dll) (and a main image path from syswow64 actually shows up in the various Ldr* and Rtl* functions, which likely indicates that process image path isn't modified) which means that my initial patch is wrong in changing the default load path, but this looks like that DLLs paths *must* be "unredirected" when being inserted in LdrData (except main module) I'll resubmit with including some of the tests. (and moving "unredirection" logic to ntdll.so will require a new syscall) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2578#note_29157
participants (3)
-
Alexandre Julliard (＠julliard) -
Eric Pouech
-
eric pouech (＠epo)