[PATCH v16 0/7] MR9371: kernel32: Implement TH32CS_SNAPMODULE32 support for CreateToolhelp32Snapshot.
Intention is to use `CreateToolhelp32Snapshot` in `dbghelp.dll: EnumerateLoadedModulesW64`. Right now we can't because `CreateToolhelp32Snapshot` can't capture 32bit modules in wow64 processes, which is needed to support `SYMOPT_INCLUDE_32BIT_MODULES`. The advantage of using `CreateToolhelp32Snapshot` in mainly performance. Right now `EnumerateLoadedModulesW64` is `O(n^2)`, because of each module it calls `GetModuleInformation` which is itself `O(n)` w.r.t. number of modules. Whereas with `CreateToolhelp32Snapshot` there is no need for `GetModuleInformation`, thus reduce the complexity to just `O(n)`. * * * P.S. I am also uncomfortable with the amount of similar code between `CreateToolhelp32Snapshot` and `EnumProcessModulesEx`. Can something be done here? -- v16: kernel32: Implement TH32CS_SNAPMODULE32 support for CreateToolhelp32Snapshot. kernel32: Use GetCurrentProcess() handle in CreateToolhelp32Snapshot if possible. kernel32/tests: Test CreateToolhelp32Snapshot with TH32CS_SNAPMODULE32. kernel32: Fix CreateToolhelp32Snapshot on old WoW64. include: Add TH32CS_SNAPMODULE32. kernel32/tests: Handle ERROR_BAD_LENGTH from CreateToolhelp32Snapshot. kernel32/tests: Fix CreateToolhelp32Snapshot failure check. https://gitlab.winehq.org/wine/wine/-/merge_requests/9371
From: Yuxuan Shui <yshui(a)codeweavers.com> CreateToolhelp32Snapshot returns INVALID_HANDLE_VALUE in case of failure, not NULL. --- dlls/kernel32/tests/toolhelp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/toolhelp.c b/dlls/kernel32/tests/toolhelp.c index 02ec0a30b81..79dbb04ac88 100644 --- a/dlls/kernel32/tests/toolhelp.c +++ b/dlls/kernel32/tests/toolhelp.c @@ -388,7 +388,7 @@ static void test_module(DWORD pid, const char* expected[], unsigned num_expected ok(ARRAY_SIZE(found) >= num_expected, "Internal: bump found[] size\n"); hSnapshot = pCreateToolhelp32Snapshot( TH32CS_SNAPMODULE, pid ); - ok(hSnapshot != NULL, "Cannot create snapshot\n"); + ok(hSnapshot != INVALID_HANDLE_VALUE, "Cannot create snapshot\n"); for (i = 0; i < num_expected; i++) found[i] = 0; me.dwSize = sizeof(me); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9371
From: Yuxuan Shui <yshui(a)codeweavers.com> According to Microsoft's documentation on CreateToolhelp32Snapshot:
If the function fails with ERROR_BAD_LENGTH, retry the function until it succeeds.
dlls/kernel32/tests/toolhelp.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/toolhelp.c b/dlls/kernel32/tests/toolhelp.c index 79dbb04ac88..8760b8cd052 100644 --- a/dlls/kernel32/tests/toolhelp.c +++ b/dlls/kernel32/tests/toolhelp.c @@ -375,6 +375,21 @@ static const char* sub_expected_modules[] = "ntdll.dll" }; +static HANDLE create_toolhelp_snapshot( DWORD flags, DWORD pid ) +{ + HANDLE hSnapshot; + int retries = winetest_platform_is_wine ? 1 : 5; + + do + { + hSnapshot = pCreateToolhelp32Snapshot( flags, pid ); + if (hSnapshot != INVALID_HANDLE_VALUE || GetLastError() != ERROR_BAD_LENGTH) + break; + } while (--retries); + + return hSnapshot; +} + static void test_module(DWORD pid, const char* expected[], unsigned num_expected) { HANDLE hSnapshot; @@ -387,7 +402,7 @@ static void test_module(DWORD pid, const char* expected[], unsigned num_expected ok(ARRAY_SIZE(found) >= num_expected, "Internal: bump found[] size\n"); - hSnapshot = pCreateToolhelp32Snapshot( TH32CS_SNAPMODULE, pid ); + hSnapshot = create_toolhelp_snapshot( TH32CS_SNAPMODULE, pid ); ok(hSnapshot != INVALID_HANDLE_VALUE, "Cannot create snapshot\n"); for (i = 0; i < num_expected; i++) found[i] = 0; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9371
From: Yuxuan Shui <yshui(a)codeweavers.com> Reference: https://learn.microsoft.com/en-us/windows/win32/api/tlhelp32/nf-tlhelp32-cre... --- include/tlhelp32.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/tlhelp32.h b/include/tlhelp32.h index 4b4d224ce28..65e3c12f88b 100644 --- a/include/tlhelp32.h +++ b/include/tlhelp32.h @@ -31,6 +31,7 @@ extern "C" { #define TH32CS_SNAPPROCESS 0x00000002 #define TH32CS_SNAPTHREAD 0x00000004 #define TH32CS_SNAPMODULE 0x00000008 +#define TH32CS_SNAPMODULE32 0x00000010 #define TH32CS_SNAPALL (TH32CS_SNAPHEAPLIST | TH32CS_SNAPPROCESS | TH32CS_SNAPTHREAD | TH32CS_SNAPMODULE) #define TH32CS_INHERIT 0x80000000 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9371
From: Yuxuan Shui <yshui(a)codeweavers.com> On old WoW64 PEB->LdrData is NULL, so later ReadProcessMemory(&NULL->InLoadOrderModuleList.Flink) fails. Don't return failure in this case. --- dlls/kernel32/toolhelp.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/dlls/kernel32/toolhelp.c b/dlls/kernel32/toolhelp.c index 5e1ec84d100..8d8920703bd 100644 --- a/dlls/kernel32/toolhelp.c +++ b/dlls/kernel32/toolhelp.c @@ -92,11 +92,16 @@ static BOOL fetch_module( DWORD process, DWORD flags, LDR_DATA_TABLE_ENTRY **ldr if (set_ntstatus( NtQueryInformationProcess( hProcess, ProcessBasicInformation, &pbi, sizeof(pbi), NULL ))) { - if (ReadProcessMemory( hProcess, &pbi.PebBaseAddress->LdrData, - &pLdrData, sizeof(pLdrData), NULL ) && - ReadProcessMemory( hProcess, - &pLdrData->InLoadOrderModuleList.Flink, - &curr, sizeof(curr), NULL )) + if (!ReadProcessMemory( hProcess, &pbi.PebBaseAddress->LdrData, + &pLdrData, sizeof(pLdrData), NULL ) || + (pLdrData && !ReadProcessMemory( hProcess, &pLdrData->InLoadOrderModuleList.Flink, + &curr, sizeof(curr), NULL ))) + { + goto out; + } + + /* pLdrData is NULL on "old" wow64. Don't fail, just return an empty modules list. */ + if (pLdrData) { head = &pLdrData->InLoadOrderModuleList; @@ -124,10 +129,11 @@ static BOOL fetch_module( DWORD process, DWORD flags, LDR_DATA_TABLE_ENTRY **ldr else HeapFree( GetProcessHeap(), 0, (*ldr_mod)[*num].BaseDllName.Buffer ); } - ret = TRUE; } + ret = TRUE; } +out: if (process) CloseHandle( hProcess ); return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9371
From: Yuxuan Shui <yshui(a)codeweavers.com> --- dlls/kernel32/tests/toolhelp.c | 237 +++++++++++++++++++++++++++++---- 1 file changed, 212 insertions(+), 25 deletions(-) diff --git a/dlls/kernel32/tests/toolhelp.c b/dlls/kernel32/tests/toolhelp.c index 8760b8cd052..c3fbac1fa0f 100644 --- a/dlls/kernel32/tests/toolhelp.c +++ b/dlls/kernel32/tests/toolhelp.c @@ -30,6 +30,7 @@ #include "wine/test.h" #include "winuser.h" #include "winternl.h" +#include "winnls.h" static char selfname[MAX_PATH]; @@ -360,21 +361,56 @@ static void test_thread(DWORD curr_pid, DWORD sub_pcs_pid) ok(!pThread32First( hSnapshot, &te ), "shouldn't return a thread\n"); } -static const char* curr_expected_modules[] = +struct expected_module { + const char *module; + + /* + * 0 = C:\windows\system32\ + * 1 = C:\windows\syswow64\ + * 2 = don't check + */ + int wow64; +}; + +static struct expected_module curr_expected_modules[] = +{ + {"kernel32_test.exe", 2}, + {"kernel32.dll"}, + {"ntdll.dll"}, +}; + +static struct expected_module sub_expected_modules[] = +{ + {"kernel32_test.exe", 2}, + {"kernel32.dll"}, + {"shell32.dll"}, + {"ntdll.dll"}, +}; + +static struct expected_module msinfo32_32_expected_modules[] = { - "kernel32_test.exe", - "kernel32.dll", - "ntdll.dll" + {"msinfo32.exe", 1}, + {"kernel32.dll", 1}, + {"shell32.dll", 1}, + {"ntdll.dll", 1}, + {"ntdll.dll"}, + {"wow64.dll"}, + {"wow64win.dll"}, + {"wow64cpu.dll"}, }; -static const char* sub_expected_modules[] = +static struct expected_module msinfo32_64_expected_modules[] = { - "kernel32_test.exe", - "kernel32.dll", - "shell32.dll", - "ntdll.dll" + {"msinfo32.exe", 1}, + {"ntdll.dll"}, + {"wow64.dll"}, + {"wow64win.dll"}, + {"wow64cpu.dll"}, }; +static char syswow64[MAX_PATH], system32[MAX_PATH]; +static int syswow64_len, system32_len; + static HANDLE create_toolhelp_snapshot( DWORD flags, DWORD pid ) { HANDLE hSnapshot; @@ -390,20 +426,87 @@ static HANDLE create_toolhelp_snapshot( DWORD flags, DWORD pid ) return hSnapshot; } -static void test_module(DWORD pid, const char* expected[], unsigned num_expected) +static BOOL match_module(const MODULEENTRY32 *module, const struct expected_module *pattern) +{ + if (lstrcmpiA(module->szModule, pattern->module)) return FALSE; + if (pattern->wow64 == 2) return TRUE; + if (pattern->wow64 == 1) + { + if (lstrlenA(module->szExePath) < syswow64_len) return FALSE; + return CompareStringA(LOCALE_INVARIANT, NORM_IGNORECASE, module->szExePath, syswow64_len, + syswow64, syswow64_len) == CSTR_EQUAL; + } + if (lstrlenA(module->szExePath) < system32_len) return FALSE; + return CompareStringA(LOCALE_INVARIANT, NORM_IGNORECASE, module->szExePath, system32_len, + system32, system32_len) == CSTR_EQUAL; +} + +static const BOOL is_win64 = sizeof(void*) > sizeof(int); + +static BOOL is_old_wow_pid(DWORD pid) { + PROCESS_BASIC_INFORMATION pbi; + PPEB_LDR_DATA pLdrData = NULL; + HANDLE hProcess = OpenProcess( PROCESS_VM_READ | PROCESS_QUERY_INFORMATION, FALSE, pid ); + NTSTATUS status = NtQueryInformationProcess( hProcess, ProcessBasicInformation, &pbi, sizeof(pbi), + NULL ); + BOOL ret = FALSE; + + if (status != STATUS_SUCCESS) return FALSE; + + if (!IsWow64Process( hProcess, &ret ) || !ret) return FALSE; + + if (!ReadProcessMemory( hProcess, &pbi.PebBaseAddress->LdrData, &pLdrData, sizeof(pLdrData), NULL )) + { + CloseHandle( hProcess ); + return FALSE; + } + + ret = !pLdrData; + CloseHandle( hProcess ); + return ret; +} + +/* Test to ensure no module is returned when TH32CS_SNAPMODULE32 is set without TH32CS_SNAPMODULE */ +static void test_module32_only(DWORD pid) +{ + HANDLE hSnapshot; + MODULEENTRY32 me; + + hSnapshot = create_toolhelp_snapshot( TH32CS_SNAPMODULE32, pid ); + todo_wine ok(hSnapshot != INVALID_HANDLE_VALUE, "Cannot create snapshot\n"); + if (hSnapshot == INVALID_HANDLE_VALUE && GetLastError() == ERROR_CALL_NOT_IMPLEMENTED) + { + skip("Cannot create snapshot handle\n"); + return; + } + + ok(!pModule32First( hSnapshot, &me ), "Got unexpected module entry\n"); + CloseHandle( hSnapshot ); +} + +static void test_module(DWORD pid, struct expected_module expected[], unsigned num_expected, BOOL module32) +{ + const BOOL is_old_wow = is_old_wow_pid(pid); HANDLE hSnapshot; PROCESSENTRY32 pe; THREADENTRY32 te; MODULEENTRY32 me; + DWORD snapshot_flags = TH32CS_SNAPMODULE | (module32 ? TH32CS_SNAPMODULE32 : 0); unsigned found[32]; unsigned i; + int expected_main_exe_count = is_win64 && module32 ? 2 : 1; int num = 0; ok(ARRAY_SIZE(found) >= num_expected, "Internal: bump found[] size\n"); - hSnapshot = create_toolhelp_snapshot( TH32CS_SNAPMODULE, pid ); - ok(hSnapshot != INVALID_HANDLE_VALUE, "Cannot create snapshot\n"); + hSnapshot = create_toolhelp_snapshot( snapshot_flags, pid ); + ok(hSnapshot != INVALID_HANDLE_VALUE, "Cannot create snapshot %#lx\n", GetLastError()); + if (hSnapshot == INVALID_HANDLE_VALUE && GetLastError() == ERROR_CALL_NOT_IMPLEMENTED) + { + skip("CreateToolhelp32Snapshot doesn't support requested flags\n"); + return; + } for (i = 0; i < num_expected; i++) found[i] = 0; me.dwSize = sizeof(me); @@ -415,13 +518,16 @@ static void test_module(DWORD pid, const char* expected[], unsigned num_expected me.th32ProcessID, me.modBaseAddr, me.modBaseSize, me.szExePath, me.szModule); ok(me.th32ProcessID == pid, "wrong returned process id\n"); for (i = 0; i < num_expected; i++) - if (!lstrcmpiA(expected[i], me.szModule)) found[i]++; + if (match_module(&me, &expected[i])) found[i]++; num++; } while (pModule32Next( hSnapshot, &me )); } - for (i = 0; i < num_expected; i++) + todo_if(winetest_platform_is_wine && module32 && is_win64) + ok(found[0] == expected_main_exe_count, "Main exe was found %d time(s)\n", found[0]); + for (i = 1; i < num_expected; i++) + todo_if((winetest_platform_is_wine && expected[i].wow64 == 1) || (is_old_wow && !expected[i].wow64)) ok(found[i] == 1, "Module %s is %s\n", - expected[i], found[i] ? "listed more than once" : "not listed"); + expected[i].module, found[i] ? "listed more than once" : "not listed"); /* check that first really resets the enumeration */ for (i = 0; i < num_expected; i++) found[i] = 0; @@ -433,13 +539,16 @@ static void test_module(DWORD pid, const char* expected[], unsigned num_expected trace("PID=%lx base=%p size=%lx %s %s\n", me.th32ProcessID, me.modBaseAddr, me.modBaseSize, me.szExePath, me.szModule); for (i = 0; i < num_expected; i++) - if (!lstrcmpiA(expected[i], me.szModule)) found[i]++; + if (match_module(&me, &expected[i])) found[i]++; num--; } while (pModule32Next( hSnapshot, &me )); } - for (i = 0; i < num_expected; i++) + todo_if(winetest_platform_is_wine && module32 && is_win64) + ok(found[0] == expected_main_exe_count, "Main exe was found %d time(s)\n", found[0]); + for (i = 1; i < num_expected; i++) + todo_if((winetest_platform_is_wine && expected[i].wow64 == 1) || (is_old_wow && !expected[i].wow64)) ok(found[i] == 1, "Module %s is %s\n", - expected[i], found[i] ? "listed more than once" : "not listed"); + expected[i].module, found[i] ? "listed more than once" : "not listed"); ok(!num, "mismatch in counting\n"); pe.dwSize = sizeof(pe); @@ -452,6 +561,42 @@ static void test_module(DWORD pid, const char* expected[], unsigned num_expected ok(!pModule32First( hSnapshot, &me ), "shouldn't return a module\n"); } +struct startup_cb +{ + DWORD pid; + HWND wnd; +}; + +static BOOL CALLBACK startup_cb_window(HWND wnd, LPARAM lParam) +{ + struct startup_cb *info = (struct startup_cb*)lParam; + DWORD pid; + + if (GetWindowThreadProcessId(wnd, &pid) && info->pid == pid && IsWindowVisible(wnd)) + { + info->wnd = wnd; + return FALSE; + } + return TRUE; +} + +static BOOL wait_process_window_visible(HANDLE proc, DWORD pid, DWORD timeout) +{ + DWORD max_tc = GetTickCount() + timeout; + BOOL ret = WaitForInputIdle(proc, timeout); + struct startup_cb info = {pid, NULL}; + + if (!ret) + { + do + { + if (EnumWindows(startup_cb_window, (LPARAM)&info)) + Sleep(100); + } while (!info.wnd && GetTickCount() < max_tc); + } + return info.wnd != NULL; +} + START_TEST(toolhelp) { DWORD pid = GetCurrentProcessId(); @@ -459,12 +604,21 @@ START_TEST(toolhelp) char *p, module[MAX_PATH]; char buffer[MAX_PATH + 21]; SECURITY_ATTRIBUTES sa; - PROCESS_INFORMATION info; - STARTUPINFOA startup; + PROCESS_INFORMATION info, info32 = {0}; + STARTUPINFOA startup, startup32 = {0}; HANDLE ev1, ev2; DWORD w; HANDLE hkernel32 = GetModuleHandleA("kernel32"); HANDLE hntdll = GetModuleHandleA("ntdll.dll"); + BOOL ret; + + if (is_win64) + { + syswow64_len = GetSystemWow64DirectoryA(syswow64, ARRAY_SIZE(syswow64)); + ok(syswow64_len, "Can't GetSystemWow64DirectoryA, %#lx\n", GetLastError()); + } + system32_len = GetSystemDirectoryA(system32, ARRAY_SIZE(system32)); + ok(system32_len, "Can't GetSystemDirectoryA, %#lx\n", GetLastError()); pCreateToolhelp32Snapshot = (VOID *) GetProcAddress(hkernel32, "CreateToolhelp32Snapshot"); pModule32First = (VOID *) GetProcAddress(hkernel32, "Module32First"); @@ -510,15 +664,48 @@ START_TEST(toolhelp) GetModuleFileNameA( 0, module, sizeof(module) ); if (!(p = strrchr( module, '\\' ))) p = module; else p++; - curr_expected_modules[0] = p; - sub_expected_modules[0] = p; + curr_expected_modules[0].module = p; + sub_expected_modules[0].module = p; test_process(pid, info.dwProcessId); test_thread(pid, info.dwProcessId); test_main_thread(pid, GetCurrentThreadId()); - test_module(pid, curr_expected_modules, ARRAY_SIZE(curr_expected_modules)); - test_module(info.dwProcessId, sub_expected_modules, ARRAY_SIZE(sub_expected_modules)); + test_module(pid, curr_expected_modules, ARRAY_SIZE(curr_expected_modules), FALSE); + test_module(info.dwProcessId, sub_expected_modules, ARRAY_SIZE(sub_expected_modules), FALSE); + test_module32_only(pid); + test_module32_only(info.dwProcessId); + if (is_win64) + { + lstrcpyA(buffer, syswow64); + strcat(buffer, "\\msinfo32.exe"); + ret = CreateProcessA(NULL, buffer, NULL, NULL, FALSE, 0, NULL, NULL, &startup32, &info32); + if (ret) + { + ret = wait_process_window_visible(info32.hProcess, info32.dwProcessId, 5000); + ok(ret, "wait timed out\n"); + + trace("testing 32bit\n"); + test_module(info32.dwProcessId, msinfo32_32_expected_modules, + ARRAY_SIZE(msinfo32_32_expected_modules), TRUE); + test_module(info32.dwProcessId, msinfo32_64_expected_modules, + ARRAY_SIZE(msinfo32_64_expected_modules), FALSE); + test_module32_only(pid); + TerminateProcess(info32.hProcess, 0); + } + else + { + if (GetLastError() == ERROR_FILE_NOT_FOUND) + skip("Skip wow64 test on non compatible platform\n"); + else + ok(ret, "wow64 CreateProcess failed: %#lx\n", GetLastError()); + } + } + else + { + /* what happens if TH32CS_SNAPMODULE32 is set on 32bit? */ + test_module(info.dwProcessId, sub_expected_modules, ARRAY_SIZE(sub_expected_modules), TRUE); + } SetEvent(ev2); - wait_child_process( &info ); + wait_child_process(&info); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9371
From: Yuxuan Shui <yshui(a)codeweavers.com> Reference: 4ea0354ecaf6c601d304cce4f7e5a4e6a8542490 --- dlls/kernel32/toolhelp.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/toolhelp.c b/dlls/kernel32/toolhelp.c index 8d8920703bd..adcad129b06 100644 --- a/dlls/kernel32/toolhelp.c +++ b/dlls/kernel32/toolhelp.c @@ -85,6 +85,11 @@ static BOOL fetch_module( DWORD process, DWORD flags, LDR_DATA_TABLE_ENTRY **ldr { hProcess = OpenProcess( PROCESS_VM_READ | PROCESS_QUERY_INFORMATION, FALSE, process ); if (!hProcess) return FALSE; + if (RtlIsCurrentProcess( hProcess )) + { + CloseHandle( hProcess ); + hProcess = GetCurrentProcess(); + } } else hProcess = GetCurrentProcess(); @@ -134,7 +139,7 @@ static BOOL fetch_module( DWORD process, DWORD flags, LDR_DATA_TABLE_ENTRY **ldr } out: - if (process) CloseHandle( hProcess ); + if (hProcess != GetCurrentProcess()) CloseHandle( hProcess ); return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9371
From: Yuxuan Shui <yshui(a)codeweavers.com> --- dlls/kernel32/tests/toolhelp.c | 19 +--- dlls/kernel32/toolhelp.c | 160 ++++++++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 18 deletions(-) diff --git a/dlls/kernel32/tests/toolhelp.c b/dlls/kernel32/tests/toolhelp.c index c3fbac1fa0f..0995137ebf1 100644 --- a/dlls/kernel32/tests/toolhelp.c +++ b/dlls/kernel32/tests/toolhelp.c @@ -474,13 +474,7 @@ static void test_module32_only(DWORD pid) MODULEENTRY32 me; hSnapshot = create_toolhelp_snapshot( TH32CS_SNAPMODULE32, pid ); - todo_wine ok(hSnapshot != INVALID_HANDLE_VALUE, "Cannot create snapshot\n"); - if (hSnapshot == INVALID_HANDLE_VALUE && GetLastError() == ERROR_CALL_NOT_IMPLEMENTED) - { - skip("Cannot create snapshot handle\n"); - return; - } - + ok(hSnapshot != INVALID_HANDLE_VALUE, "Cannot create snapshot\n"); ok(!pModule32First( hSnapshot, &me ), "Got unexpected module entry\n"); CloseHandle( hSnapshot ); } @@ -502,11 +496,6 @@ static void test_module(DWORD pid, struct expected_module expected[], unsigned n hSnapshot = create_toolhelp_snapshot( snapshot_flags, pid ); ok(hSnapshot != INVALID_HANDLE_VALUE, "Cannot create snapshot %#lx\n", GetLastError()); - if (hSnapshot == INVALID_HANDLE_VALUE && GetLastError() == ERROR_CALL_NOT_IMPLEMENTED) - { - skip("CreateToolhelp32Snapshot doesn't support requested flags\n"); - return; - } for (i = 0; i < num_expected; i++) found[i] = 0; me.dwSize = sizeof(me); @@ -522,10 +511,9 @@ static void test_module(DWORD pid, struct expected_module expected[], unsigned n num++; } while (pModule32Next( hSnapshot, &me )); } - todo_if(winetest_platform_is_wine && module32 && is_win64) ok(found[0] == expected_main_exe_count, "Main exe was found %d time(s)\n", found[0]); for (i = 1; i < num_expected; i++) - todo_if((winetest_platform_is_wine && expected[i].wow64 == 1) || (is_old_wow && !expected[i].wow64)) + todo_if(is_old_wow && !expected[i].wow64) ok(found[i] == 1, "Module %s is %s\n", expected[i].module, found[i] ? "listed more than once" : "not listed"); @@ -543,10 +531,9 @@ static void test_module(DWORD pid, struct expected_module expected[], unsigned n num--; } while (pModule32Next( hSnapshot, &me )); } - todo_if(winetest_platform_is_wine && module32 && is_win64) ok(found[0] == expected_main_exe_count, "Main exe was found %d time(s)\n", found[0]); for (i = 1; i < num_expected; i++) - todo_if((winetest_platform_is_wine && expected[i].wow64 == 1) || (is_old_wow && !expected[i].wow64)) + todo_if(is_old_wow && !expected[i].wow64) ok(found[i] == 1, "Module %s is %s\n", expected[i].module, found[i] ? "listed more than once" : "not listed"); ok(!num, "mismatch in counting\n"); diff --git a/dlls/kernel32/toolhelp.c b/dlls/kernel32/toolhelp.c index adcad129b06..62226136f2d 100644 --- a/dlls/kernel32/toolhelp.c +++ b/dlls/kernel32/toolhelp.c @@ -66,21 +66,61 @@ static WCHAR *fetch_string( HANDLE hProcess, UNICODE_STRING* us) } } us->Buffer = local; + us->MaximumLength = us->Length; return local; } +typedef struct _LDR_DATA_TABLE_ENTRY32 +{ + LIST_ENTRY32 InLoadOrderLinks; + LIST_ENTRY32 InMemoryOrderLinks; + LIST_ENTRY32 InInitializationOrderLinks; + DWORD DllBase; + DWORD EntryPoint; + ULONG SizeOfImage; + UNICODE_STRING32 FullDllName; + UNICODE_STRING32 BaseDllName; +} LDR_DATA_TABLE_ENTRY32; + +typedef LIST_ENTRY32 *PLIST_ENTRY32; + +static inline void ldr_data_table_entry_32to64(LDR_DATA_TABLE_ENTRY *dst, LDR_DATA_TABLE_ENTRY32 *src) +{ + dst->BaseDllName.Buffer = (PWSTR)(DWORD_PTR)src->BaseDllName.Buffer; + dst->BaseDllName.Length = src->BaseDllName.Length; + dst->FullDllName.Buffer = (PWSTR)(DWORD_PTR)src->FullDllName.Buffer; + dst->FullDllName.Length = src->FullDllName.Length; + dst->DllBase = (void *)(DWORD_PTR)src->DllBase; + dst->SizeOfImage = src->SizeOfImage; +} + static BOOL fetch_module( DWORD process, DWORD flags, LDR_DATA_TABLE_ENTRY **ldr_mod, ULONG *num ) { + static const BOOL is_win64 = (sizeof(void *) > sizeof(int)); HANDLE hProcess; PROCESS_BASIC_INFORMATION pbi; PPEB_LDR_DATA pLdrData; PLIST_ENTRY head, curr; + PLIST_ENTRY32 head32, curr32; BOOL ret = FALSE; + BOOL target_wow64; + /* On old WoW64, if TH32CS_SNAPMODULE32 is not set, we will return nothing. But other similar + * functions, like EnumProcessModules, will at least return the main module for the 64-bit + * portion of the modules. Here we do the same to be consistent. */ + BOOL old_wow64_need_main_module = FALSE; + BOOL wow64_32bit_module_requested; + BOOL is_wow64; + PEB32* peb32; + DWORD pLdrData32, tmp; + WCHAR system32[MAX_PATH] = {}, syswow64[MAX_PATH] = {}; + int system32_len = 0, syswow64_len = 0; *num = 0; if (!(flags & TH32CS_SNAPMODULE)) return TRUE; + if (!IsWow64Process( GetCurrentProcess(), &is_wow64 )) return FALSE; + if (process) { hProcess = OpenProcess( PROCESS_VM_READ | PROCESS_QUERY_INFORMATION, FALSE, process ); @@ -94,6 +134,20 @@ static BOOL fetch_module( DWORD process, DWORD flags, LDR_DATA_TABLE_ENTRY **ldr else hProcess = GetCurrentProcess(); + if (hProcess != GetCurrentProcess()) + { + if (!IsWow64Process( hProcess, &target_wow64 )) return FALSE; + } + else target_wow64 = is_wow64; + + if (is_wow64 && !target_wow64) + { + SetLastError( ERROR_PARTIAL_COPY ); + goto out; + } + + wow64_32bit_module_requested = (flags & TH32CS_SNAPMODULE32) && is_win64 && target_wow64; + if (set_ntstatus( NtQueryInformationProcess( hProcess, ProcessBasicInformation, &pbi, sizeof(pbi), NULL ))) { @@ -105,7 +159,7 @@ static BOOL fetch_module( DWORD process, DWORD flags, LDR_DATA_TABLE_ENTRY **ldr goto out; } - /* pLdrData is NULL on "old" wow64. Don't fail, just return an empty modules list. */ + /* pLdrData is NULL on "old" wow64 configuration. Don't fail. */ if (pLdrData) { head = &pLdrData->InLoadOrderModuleList; @@ -135,8 +189,110 @@ static BOOL fetch_module( DWORD process, DWORD flags, LDR_DATA_TABLE_ENTRY **ldr HeapFree( GetProcessHeap(), 0, (*ldr_mod)[*num].BaseDllName.Buffer ); } } + else old_wow64_need_main_module = TRUE; + } + + if (!wow64_32bit_module_requested && !old_wow64_need_main_module) + { ret = TRUE; + goto out; + } + + /* need system directory paths for path rewrites when querying 32bit modules from a 64bit + * process */ + if (!(system32_len = GetSystemDirectoryW(system32, ARRAY_SIZE(system32))) || + !(syswow64_len = GetSystemWow64DirectoryW(syswow64, ARRAY_SIZE(syswow64)))) + { + goto out; + } + + if (!set_ntstatus( NtQueryInformationProcess( hProcess, ProcessWow64Information, + &peb32, sizeof(peb32), NULL )) || + !ReadProcessMemory( hProcess, &peb32->LdrData, &pLdrData32, sizeof(pLdrData32), NULL ) || + !ReadProcessMemory( hProcess, + &((PPEB_LDR_DATA32)(DWORD_PTR)pLdrData32)->InLoadOrderModuleList.Flink, + &tmp, sizeof(tmp), NULL )) + { + goto out; + } + + curr32 = (PLIST_ENTRY32)(DWORD_PTR)tmp; + head32 = &((PPEB_LDR_DATA32)(DWORD_PTR)pLdrData32)->InLoadOrderModuleList; + while (curr32 != head32) + { + LDR_DATA_TABLE_ENTRY32 entry32; + LDR_DATA_TABLE_ENTRY* out_entry; + int full_dll_name_len; + if (!*num) + *ldr_mod = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(LDR_DATA_TABLE_ENTRY) ); + else + *ldr_mod = HeapReAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, *ldr_mod, + (*num + 1) * sizeof(LDR_DATA_TABLE_ENTRY) ); + out_entry = &(*ldr_mod)[*num]; + if (!ReadProcessMemory( hProcess, + CONTAINING_RECORD(curr32, LDR_DATA_TABLE_ENTRY32, InLoadOrderLinks), + &entry32, sizeof(entry32), NULL )) + break; + + curr32 = (PLIST_ENTRY32)(DWORD_PTR)entry32.InLoadOrderLinks.Flink; + ldr_data_table_entry_32to64(out_entry, &entry32); + if (!fetch_string( hProcess, &out_entry->BaseDllName )) continue; + if (!fetch_string( hProcess, &out_entry->FullDllName )) + { + HeapFree( GetProcessHeap(), 0, out_entry->BaseDllName.Buffer ); + continue; + } + + /* rewrite path in system32 into syswow64 for 32bit modules */ + full_dll_name_len = out_entry->FullDllName.Length / sizeof(WCHAR); + if (full_dll_name_len >= system32_len && + CompareStringW( LOCALE_INVARIANT, NORM_IGNORECASE, + system32, system32_len, + out_entry->FullDllName.Buffer, system32_len ) == CSTR_EQUAL) + { + int new_len = full_dll_name_len - system32_len + syswow64_len; + WCHAR *new_path = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, new_len * sizeof(WCHAR) ); + + if (!new_path) + { + HeapFree( GetProcessHeap(), 0, out_entry->BaseDllName.Buffer ); + HeapFree( GetProcessHeap(), 0, out_entry->FullDllName.Buffer ); + continue; + } + + lstrcpyW( new_path, syswow64 ); + memcpy( new_path + syswow64_len, out_entry->FullDllName.Buffer + system32_len, + out_entry->FullDllName.Length - system32_len * sizeof(WCHAR) ); + HeapFree( GetProcessHeap(), 0, out_entry->FullDllName.Buffer ); + out_entry->FullDllName.Buffer = new_path; + out_entry->FullDllName.MaximumLength = out_entry->FullDllName.Length = new_len * sizeof(WCHAR); + } + + (*num)++; + if (!wow64_32bit_module_requested) + /* In this case, we only needed the main module once, and none of the 32-bit modules. */ + break; + if (old_wow64_need_main_module && *num == 1) + { + /* When 32bit modules are requested on old WoW64, we are expected to return the main + * module twice, once for the 64-bit portion, once for the 32-bit portion */ + LDR_DATA_TABLE_ENTRY *prev_entry; + *ldr_mod = HeapReAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, *ldr_mod, + (*num + 1) * sizeof(LDR_DATA_TABLE_ENTRY) ); + out_entry = &(*ldr_mod)[1]; + prev_entry = &(*ldr_mod)[0]; + *out_entry = *prev_entry; + + out_entry->BaseDllName.Buffer = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, + out_entry->BaseDllName.MaximumLength ); + RtlCopyUnicodeString(&out_entry->BaseDllName, &prev_entry->BaseDllName); + out_entry->FullDllName.Buffer = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, + out_entry->FullDllName.MaximumLength ); + RtlCopyUnicodeString(&out_entry->FullDllName, &prev_entry->FullDllName); + (*num)++; + } } + ret = TRUE; out: if (hProcess != GetCurrentProcess()) CloseHandle( hProcess ); @@ -302,7 +458,7 @@ HANDLE WINAPI CreateToolhelp32Snapshot( DWORD flags, DWORD process ) HANDLE hSnapShot = 0; TRACE("%lx,%lx\n", flags, process ); - if (!(flags & (TH32CS_SNAPPROCESS|TH32CS_SNAPTHREAD|TH32CS_SNAPMODULE))) + if (!(flags & (TH32CS_SNAPPROCESS|TH32CS_SNAPTHREAD|TH32CS_SNAPMODULE|TH32CS_SNAPMODULE32))) { FIXME("flags %lx not implemented\n", flags ); SetLastError( ERROR_CALL_NOT_IMPLEMENTED ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9371
@epo hi, can this MR move forward? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9371#note_127629
Hi Yuxuan thanks for updating the MR the overall output looks ok to me the last tiny bit I'm still concerned with is the 6th patch (the one checking if pid is the current process); i theory it shouldn't be harmful, unless we end up with different access rights in toolhelp's openprocess vs the current process pseudo handle ones but it could (theorically) change existing behavior (for a regular module snapshot) considering that: * there's no report (at least that I know of) of an app making use of 32bit module snapshot * the only real client of the API (with 32bits) will be dbghelp so I'd rather not include the 6th patch (until we get evidence we need it), and rather ensures dbghelp passes the 0 pid when dealing with current process (and testing accurate access rights) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9371#note_127697
On Tue Jan 27 16:32:33 2026 +0000, eric pouech wrote:
Hi Yuxuan thanks for updating the MR the overall output looks ok to me the last tiny bit I'm still concerned with is the 6th patch (the one checking if pid is the current process); i theory it shouldn't be harmful, unless we end up with different access rights in toolhelp's openprocess vs the current process pseudo handle ones but it could (theorically) change existing behavior (for a regular module snapshot) considering that: * there's no report (at least that I know of) of an app making use of 32bit module snapshot * the only real client of the API (with 32bits) will be dbghelp so I'd rather not include the 6th patch (until we get evidence we need it), and rather ensures dbghelp passes the 0 pid when dealing with current process (and testing accurate access rights) hmm, so we keep the check added in !8841, and in that case pass 0 as pid? sounds reasonable.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9371#note_128159
participants (4)
-
eric pouech (＠epo) -
Yuxuan Shui -
Yuxuan Shui (＠yshui)
-
Yuxuan Shui (＠yshui)