[PATCH 0/6] MR2347: Various fixes for dbghelp's ELF integration.
This series fixes a couple of issues related to dbghelp lookup in ELF modules: - get the correct wine loader in 64bit multi-arch setup - don't fail in 32bit multi-arch setup when trying to access 64bit ELF modules above 4G - handle libwine.so removal and let vdso lookup work again - adding a couple of tests -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2347
From: Eric Pouech <eric.pouech(a)gmail.com> Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/tests/dbghelp.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/dlls/dbghelp/tests/dbghelp.c b/dlls/dbghelp/tests/dbghelp.c index f68ca8281de..07dfe6fbf6e 100644 --- a/dlls/dbghelp/tests/dbghelp.c +++ b/dlls/dbghelp/tests/dbghelp.c @@ -29,6 +29,8 @@ static const BOOL is_win64 = sizeof(void*) > sizeof(int); static WCHAR system_directory[MAX_PATH]; static WCHAR wow64_directory[MAX_PATH]; +static BOOL (*WINAPI pIsWow64Process2)(HANDLE, USHORT*, USHORT*); + #if defined(__i386__) || defined(__x86_64__) static DWORD CALLBACK stack_walk_thread(void *arg) @@ -564,13 +566,8 @@ enum process_kind static enum process_kind get_process_kind(HANDLE process) { - HMODULE mod; USHORT m1, m2; - BOOL (WINAPI *pIsWow64Process2)(HANDLE,USHORT*,USHORT*); - const BOOL is_win64 = sizeof(void*) == 8; - mod = GetModuleHandleA("kernel32"); - pIsWow64Process2 = (void*)GetProcAddress(mod, "IsWow64Process2"); if (!pIsWow64Process2) { BOOL is_wow64; @@ -820,6 +817,8 @@ START_TEST(dbghelp) SetEnvironmentVariableA("_NT_SYMBOL_PATH", NULL); SetEnvironmentVariableA("_NT_ALT_SYMBOL_PATH", NULL); + pIsWow64Process2 = (void*)GetProcAddress(GetModuleHandleA("kernel32.dll"), "IsWow64Process2"); + ret = SymInitialize(GetCurrentProcess(), NULL, TRUE); ok(ret, "got error %lu\n", GetLastError()); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2347
From: Eric Pouech <eric.pouech(a)gmail.com> Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/tests/dbghelp.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/dlls/dbghelp/tests/dbghelp.c b/dlls/dbghelp/tests/dbghelp.c index 07dfe6fbf6e..f6cea0125f4 100644 --- a/dlls/dbghelp/tests/dbghelp.c +++ b/dlls/dbghelp/tests/dbghelp.c @@ -564,7 +564,7 @@ enum process_kind PCSKIND_WOW64, /* Wine "new" wow64 configuration, and Windows with wow64 support */ }; -static enum process_kind get_process_kind(HANDLE process) +static enum process_kind get_process_kind_internal(HANDLE process) { USHORT m1, m2; @@ -601,6 +601,14 @@ static enum process_kind get_process_kind(HANDLE process) return PCSKIND_ERROR; } +static enum process_kind get_process_kind(HANDLE process) +{ + DWORD gle = GetLastError(); + enum process_kind pcskind = get_process_kind_internal(process); + SetLastError(gle); + return pcskind; +} + struct loaded_module_aggregation { HANDLE proc; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2347
From: Eric Pouech <eric.pouech(a)gmail.com> Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/module.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index ad8ec22fe32..8c7639dba38 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -134,7 +134,7 @@ WCHAR *get_wine_loader_name(struct process *pcs) unsigned len; name = process_getenv(pcs, L"WINELOADER"); - if (!name) name = pcs->is_64bit ? L"wine64" : L"wine"; + if (!name) name = pcs->is_system_64bit ? L"wine64" : L"wine"; len = lstrlenW(name); /* WINELOADER isn't properly updated in Wow64 process calling inside Windows env block @@ -145,9 +145,14 @@ WCHAR *get_wine_loader_name(struct process *pcs) if (altname) { memcpy(altname, name, len * sizeof(WCHAR)); - if (pcs->is_64bit && len >= 2 && memcmp(name + len - 2, L"64", 2 * sizeof(WCHAR)) != 0) + if (pcs->is_system_64bit && len >= 2 && memcmp(name + len - 2, L"64", 2 * sizeof(WCHAR)) != 0) + { lstrcpyW(altname + len, L"64"); - else if (!pcs->is_64bit && len >= 2 && !memcmp(name + len - 2, L"64", 2 * sizeof(WCHAR))) + /* in multi-arch wow configuration, wine64 doesn't exist */ + if (GetFileAttributesW(altname) == INVALID_FILE_ATTRIBUTES) + altname[len] = L'\0'; + } + else if (!pcs->is_system_64bit && len >= 2 && !memcmp(name + len - 2, L"64", 2 * sizeof(WCHAR))) altname[len - 2] = '\0'; else altname[len] = '\0'; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2347
From: Eric Pouech <eric.pouech(a)gmail.com> Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/tests/dbghelp.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/dlls/dbghelp/tests/dbghelp.c b/dlls/dbghelp/tests/dbghelp.c index f6cea0125f4..78c2d157edc 100644 --- a/dlls/dbghelp/tests/dbghelp.c +++ b/dlls/dbghelp/tests/dbghelp.c @@ -424,6 +424,9 @@ static BOOL test_modules(void) ret = SymInitialize(dummy, NULL, FALSE); ok(ret, "got error %lu\n", GetLastError()); + ret = SymRefreshModuleList(dummy); + ok(!ret, "SymRefreshModuleList should have failed\n"); + count = get_module_count(dummy); ok(count == 0, "Unexpected count (%u instead of 0)\n", count); @@ -731,6 +734,10 @@ static void test_loaded_modules(void) } } + ret = SymRefreshModuleList(pi.hProcess); + todo_wine_if(get_process_kind(pi.hProcess) == PCSKIND_WOW64) + ok(ret, "SymRefreshModuleList failed: %lu\n", GetLastError()); + SymCleanup(pi.hProcess); TerminateProcess(pi.hProcess, 0); @@ -766,6 +773,9 @@ static void test_loaded_modules(void) "Wrong directory aggregation count %u %u\n", aggregation.count_systemdir, aggregation.count_wowdir); + ret = SymRefreshModuleList(pi.hProcess); + ok(ret, "SymRefreshModuleList failed: %lu\n", GetLastError()); + SymCleanup(pi.hProcess); TerminateProcess(pi.hProcess, 0); } @@ -804,6 +814,9 @@ static void test_loaded_modules(void) "Wrong directory aggregation count %u %u\n", aggregation2.count_systemdir, aggregation2.count_wowdir); + ret = SymRefreshModuleList(pi.hProcess); + ok(ret, "SymRefreshModuleList failed: %lu\n", GetLastError()); + SymCleanup(pi.hProcess); TerminateProcess(pi.hProcess, 0); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2347
From: Eric Pouech <eric.pouech(a)gmail.com> For dbghelp in 32-bit, when accessing a live debuggee in multi-arch configuration, Wine's ELF loader is likely mapped above 4G, hence not accessible with 32bit Windows APIs. So don't try to expose the ELF libraries in that case. Introducing a new loader operation class to support live targets, for which system operations are not accessible, but pretending they are successful. Note: - when Wine's loader ELF module isn't registered by dbghelp, any other ELF module will not be registered by dbghelp. - further work may be needed for winedbg in auto mode (launched with AeDebug key on process exception) as in that mode winedbg doesn't relaunch itself in 64bit, so won't be able to access (64bit) ELF information (in multi-arch configuration). Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/dbghelp_private.h | 1 + dlls/dbghelp/elf_module.c | 12 +++++++--- dlls/dbghelp/module.c | 41 +++++++++++++++++++++++++--------- dlls/dbghelp/tests/dbghelp.c | 1 - 4 files changed, 41 insertions(+), 14 deletions(-) diff --git a/dlls/dbghelp/dbghelp_private.h b/dlls/dbghelp/dbghelp_private.h index 1852aef1795..c11ccf81a92 100644 --- a/dlls/dbghelp/dbghelp_private.h +++ b/dlls/dbghelp/dbghelp_private.h @@ -716,6 +716,7 @@ void minidump_add_memory_block(struct dump_context* dc, ULONG64 base, ULONG size extern const WCHAR S_ElfW[] DECLSPEC_HIDDEN; extern const WCHAR S_WineLoaderW[] DECLSPEC_HIDDEN; extern const struct loader_ops no_loader_ops DECLSPEC_HIDDEN; +extern const struct loader_ops empty_loader_ops DECLSPEC_HIDDEN; extern BOOL module_init_pair(struct module_pair* pair, HANDLE hProcess, DWORD64 addr) DECLSPEC_HIDDEN; diff --git a/dlls/dbghelp/elf_module.c b/dlls/dbghelp/elf_module.c index 3c1e225e195..ee8315c4a23 100644 --- a/dlls/dbghelp/elf_module.c +++ b/dlls/dbghelp/elf_module.c @@ -39,7 +39,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(dbghelp); struct elf_info { unsigned flags; /* IN one (or several) of the ELF_INFO constants */ - DWORD_PTR dbg_hdr_addr; /* OUT address of debug header (if ELF_INFO_DEBUG_HEADER is set) */ + DWORD64 dbg_hdr_addr; /* OUT address of debug header (if ELF_INFO_DEBUG_HEADER is set) */ struct module* module; /* OUT loaded module (if ELF_INFO_MODULE is set) */ const WCHAR* module_name; /* OUT found module name (if ELF_INFO_NAME is set) */ }; @@ -1769,8 +1769,14 @@ BOOL elf_read_wine_loader_dbg_info(struct process* pcs, ULONG_PTR addr) HeapFree(GetProcessHeap(), 0, loader); } if (!ret || !elf_info.dbg_hdr_addr) return FALSE; - - TRACE("Found ELF debug header %#Ix\n", elf_info.dbg_hdr_addr); + if (elf_info.dbg_hdr_addr != (ULONG_PTR)elf_info.dbg_hdr_addr) + { + ERR("Unable to access ELF libraries (outside 32bit limit)\n"); + module_remove(pcs, elf_info.module); + pcs->loader = &empty_loader_ops; + return FALSE; + } + TRACE("Found ELF debug header %#I64x\n", elf_info.dbg_hdr_addr); elf_info.module->format_info[DFI_ELF]->u.elf_info->elf_loader = 1; module_set_module(elf_info.module, S_WineLoaderW); pcs->dbg_hdr_addr = elf_info.dbg_hdr_addr; diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index 8c7639dba38..ee559cfd99e 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -1548,11 +1548,6 @@ PVOID WINAPI SymFunctionTableAccess64(HANDLE hProcess, DWORD64 AddrBase) return module->cpu->find_runtime_function(module, AddrBase); } -static BOOL native_synchronize_module_list(struct process* pcs) -{ - return FALSE; -} - static struct module* native_load_module(struct process* pcs, const WCHAR* name, ULONG_PTR addr) { return NULL; @@ -1564,22 +1559,48 @@ static BOOL native_load_debug_info(struct process* process, struct module* modul return FALSE; } -static BOOL native_enum_modules(struct process *process, enum_modules_cb cb, void* user) +static BOOL native_fetch_file_info(struct process* process, const WCHAR* name, ULONG_PTR load_addr, DWORD_PTR* base, + DWORD* size, DWORD* checksum) { return FALSE; } -static BOOL native_fetch_file_info(struct process* process, const WCHAR* name, ULONG_PTR load_addr, DWORD_PTR* base, - DWORD* size, DWORD* checksum) +static BOOL noloader_synchronize_module_list(struct process* pcs) +{ + return FALSE; +} + +static BOOL noloader_enum_modules(struct process *process, enum_modules_cb cb, void* user) { return FALSE; } +static BOOL empty_synchronize_module_list(struct process* pcs) +{ + return TRUE; +} + +static BOOL empty_enum_modules(struct process *process, enum_modules_cb cb, void* user) +{ + return TRUE; +} + +/* to be used when debuggee isn't a live target */ const struct loader_ops no_loader_ops = { - native_synchronize_module_list, + noloader_synchronize_module_list, + native_load_module, + native_load_debug_info, + noloader_enum_modules, + native_fetch_file_info, +}; + +/* to be used when debuggee is a live target, but which system information isn't available */ +const struct loader_ops empty_loader_ops = +{ + empty_synchronize_module_list, native_load_module, native_load_debug_info, - native_enum_modules, + empty_enum_modules, native_fetch_file_info, }; diff --git a/dlls/dbghelp/tests/dbghelp.c b/dlls/dbghelp/tests/dbghelp.c index 78c2d157edc..88f1f0c4eaa 100644 --- a/dlls/dbghelp/tests/dbghelp.c +++ b/dlls/dbghelp/tests/dbghelp.c @@ -735,7 +735,6 @@ static void test_loaded_modules(void) } ret = SymRefreshModuleList(pi.hProcess); - todo_wine_if(get_process_kind(pi.hProcess) == PCSKIND_WOW64) ok(ret, "SymRefreshModuleList failed: %lu\n", GetLastError()); SymCleanup(pi.hProcess); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2347
From: Eric Pouech <eric.pouech(a)gmail.com> - should have been fixed when libwine.so has been removed - code was broken anyway - enhanced protect against error when reading debuggee's memory Introducing helper to correctly read pointer's from debuggee (and other integral values). vdso is now listed in ELF's module list (except if Wine's preloader removes it from auxilary vector) Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/dbghelp_private.h | 9 +++++++ dlls/dbghelp/elf_module.c | 44 +++++++++++++++++++++------------- 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/dlls/dbghelp/dbghelp_private.h b/dlls/dbghelp/dbghelp_private.h index c11ccf81a92..9a9773269af 100644 --- a/dlls/dbghelp/dbghelp_private.h +++ b/dlls/dbghelp/dbghelp_private.h @@ -523,6 +523,15 @@ static inline BOOL read_process_memory(const struct process *process, UINT64 add return ReadProcessMemory(process->handle, (void*)(UINT_PTR)addr, buf, size, NULL); } +static inline BOOL read_process_integral_value(const struct process* process, UINT64 addr, UINT64* pvalue, size_t size) +{ + /* Assuming that debugger and debuggee are little endian. */ + UINT64 value = 0; + if (size > sizeof(value) || !read_process_memory(process, addr, &value, size)) return FALSE; + *pvalue = value; + return TRUE; +} + struct line_info { ULONG_PTR is_first : 1, diff --git a/dlls/dbghelp/elf_module.c b/dlls/dbghelp/elf_module.c index ee8315c4a23..a2a60752a68 100644 --- a/dlls/dbghelp/elf_module.c +++ b/dlls/dbghelp/elf_module.c @@ -1339,39 +1339,51 @@ static BOOL elf_load_file_cb(void *param, HANDLE handle, const WCHAR *filename) /****************************************************************** * elf_search_auxv * - * locate some a value from the debuggee auxiliary vector + * Locate a value from the debuggee auxiliary vector */ static BOOL elf_search_auxv(const struct process* pcs, unsigned type, ULONG_PTR* val) { char buffer[sizeof(SYMBOL_INFO) + MAX_SYM_NAME]; SYMBOL_INFO*si = (SYMBOL_INFO*)buffer; - BYTE* addr; - BYTE* str; - BYTE* str_max; + const unsigned ptr_size = pcs->is_system_64bit ? 8 : 4; + UINT64 envp; + UINT64 addr; + UINT64 str; + UINT64 str_max; si->SizeOfStruct = sizeof(*si); si->MaxNameLen = MAX_SYM_NAME; - if (!SymFromName(pcs->handle, "libwine.so.1!__wine_main_environ", si) || - !(addr = (void*)(DWORD_PTR)si->Address) || - !ReadProcessMemory(pcs->handle, addr, &addr, sizeof(addr), NULL) || - !addr) + if (!SymFromName(pcs->handle, "ntdll.so!main_envp", si) || + !si->Address || + !read_process_integral_value(pcs, si->Address, &envp, ptr_size) || + !envp) { FIXME("can't find symbol in module\n"); return FALSE; } /* walk through envp[] */ /* envp[] strings are located after the auxiliary vector, so protect the walk */ - str_max = (void*)(DWORD_PTR)~0L; - while (ReadProcessMemory(pcs->handle, addr, &str, sizeof(str), NULL) && - (addr = (void*)((DWORD_PTR)addr + sizeof(str))) != NULL && str != NULL) - str_max = min(str_max, str); + str_max = ~(UINT64)0u; + addr = envp; + for (;;) + { + if (!read_process_integral_value(pcs, addr, &str, ptr_size) || (addr += ptr_size) <= ptr_size) + return FALSE; + if (!str) break; + /* It can be some env vars have been changed, pointing to a different location */ + if (str >= envp) + str_max = min(str_max, str); + } /* Walk through the end of envp[] array. * Actually, there can be several NULLs at the end of envp[]. This happens when an env variable is * deleted, the last entry is replaced by an extra NULL. */ - while (addr < str_max && ReadProcessMemory(pcs->handle, addr, &str, sizeof(str), NULL) && str == NULL) - addr = (void*)((DWORD_PTR)addr + sizeof(str)); + for (; addr < str_max; addr += ptr_size) + { + if (!read_process_integral_value(pcs, addr, &str, ptr_size)) return FALSE; + if (str) break; + } if (pcs->is_system_64bit) { @@ -1381,7 +1393,7 @@ static BOOL elf_search_auxv(const struct process* pcs, unsigned type, ULONG_PTR* UINT64 a_val; } auxv; - while (ReadProcessMemory(pcs->handle, addr, &auxv, sizeof(auxv), NULL) && auxv.a_type) + while (read_process_memory(pcs, addr, &auxv, sizeof(auxv)) && auxv.a_type) { if (auxv.a_type == type) { @@ -1399,7 +1411,7 @@ static BOOL elf_search_auxv(const struct process* pcs, unsigned type, ULONG_PTR* UINT32 a_val; } auxv; - while (ReadProcessMemory(pcs->handle, addr, &auxv, sizeof(auxv), NULL) && auxv.a_type) + while (read_process_memory(pcs, addr, &auxv, sizeof(auxv)) && auxv.a_type) { if (auxv.a_type == type) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2347
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=130370 Your paranoid android. === w11pro64 (32 bit report) === dbghelp: dbghelp.c:738: Test failed: SymRefreshModuleList failed: 2147483661
participants (3)
-
Eric Pouech -
eric pouech (＠epo)
-
Marvin