[PATCH 0/6] Improvements around module handling in dbghelp
The following series implements: - a couple of bug fixes related to module management - a change in collision detection while loading modules: + logic based on module base address (rather than name) + allow loading several modules with same module names (either multiple times the same module, or different modules with same module name) A+ --- Eric Pouech (6): dbghelp: fix module name construction dbghelp: always search current directory when loading PE modules dbghelp: allow 32bit dbghelp to handle 64 addresses dbghelp: move debug info loading out of image backends into SymLoadModuleEx() dbghelp: detect collision by looking at module's base address in SymLoadModuleEx() dbghelp: improve collision handling in SymLoadModuleEx() dlls/dbghelp/elf_module.c | 7 +- dlls/dbghelp/macho_module.c | 4 - dlls/dbghelp/module.c | 169 +++++++++++++++++++----------------- dlls/dbghelp/pe_module.c | 12 +-- 4 files changed, 95 insertions(+), 97 deletions(-)
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/module.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index 302f85b0747..b08a536eefa 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -103,13 +103,13 @@ static void module_fill_module(const WCHAR* in, WCHAR* out, size_t size) const WCHAR *ptr, *endptr; size_t len, l; - ptr = get_filename(in, endptr = in + lstrlenW(in)); + endptr = in + lstrlenW(in); + endptr -= match_ext(in, endptr - in); + ptr = get_filename(in, endptr); len = min(endptr - ptr, size - 1); memcpy(out, ptr, len * sizeof(WCHAR)); out[len] = '\0'; - if (len > 4 && (l = match_ext(out, len))) - out[len - l] = '\0'; - else if (is_wine_loader(out)) + if (is_wine_loader(out)) lstrcpynW(out, S_WineLoaderW, size); else {
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/pe_module.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/dbghelp/pe_module.c b/dlls/dbghelp/pe_module.c index 35db3edf40f..77ae122901a 100644 --- a/dlls/dbghelp/pe_module.c +++ b/dlls/dbghelp/pe_module.c @@ -791,7 +791,8 @@ struct module* pe_load_native_module(struct process* pcs, const WCHAR* name, { assert(name); - if ((hFile = FindExecutableImageExW(name, pcs->search_path, loaded_name, NULL, NULL)) == NULL) + if ((hFile = FindExecutableImageExW(name, pcs->search_path, loaded_name, NULL, NULL)) == NULL && + (hFile = FindExecutableImageExW(name, L".", loaded_name, NULL, NULL)) == NULL) return NULL; opened = TRUE; }
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/module.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index b08a536eefa..8f8820f90a9 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -920,8 +920,6 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam if (Flags & ~(SLMFLAG_VIRTUAL)) FIXME("Unsupported Flags %08x for %s\n", Flags, debugstr_w(wImageName)); - if (!validate_addr64(BaseOfDll)) return 0; - pcs->loader->synchronize_module_list(pcs); /* this is a Wine extension to the API just to redo the synchronisation */ @@ -1408,10 +1406,7 @@ BOOL WINAPI SymGetModuleInfoW64(HANDLE hProcess, DWORD64 dwAddr, */ DWORD WINAPI SymGetModuleBase(HANDLE hProcess, DWORD dwAddr) { - DWORD64 ret; - - ret = SymGetModuleBase64(hProcess, dwAddr); - return validate_addr64(ret) ? ret : 0; + return (DWORD)SymGetModuleBase64(hProcess, dwAddr); } /***********************************************************************
Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/elf_module.c | 7 +--- dlls/dbghelp/macho_module.c | 4 -- dlls/dbghelp/module.c | 74 +++++++++++++++++++++++-------------------- dlls/dbghelp/pe_module.c | 9 +---- 4 files changed, 42 insertions(+), 52 deletions(-) diff --git a/dlls/dbghelp/elf_module.c b/dlls/dbghelp/elf_module.c index dbae4db40d2..def8b444e4e 100644 --- a/dlls/dbghelp/elf_module.c +++ b/dlls/dbghelp/elf_module.c @@ -1261,15 +1261,10 @@ static BOOL elf_load_file_from_fmap(struct process* pcs, const WCHAR* filename, elf_module_info->file_map = *fmap; elf_reset_file_map(fmap); - if (dbghelp_options & SYMOPT_DEFERRED_LOADS) - { - elf_info->module->module.SymType = SymDeferred; - ret = TRUE; - } - else ret = elf_load_debug_info(pcs, elf_info->module); elf_module_info->elf_mark = 1; elf_module_info->elf_loader = 0; + ret = TRUE; } else ret = TRUE; if (elf_info->flags & ELF_INFO_NAME) diff --git a/dlls/dbghelp/macho_module.c b/dlls/dbghelp/macho_module.c index c4a19b63de0..7d0d45b3a5a 100644 --- a/dlls/dbghelp/macho_module.c +++ b/dlls/dbghelp/macho_module.c @@ -1495,10 +1495,6 @@ static BOOL macho_load_file(struct process* pcs, const WCHAR* filename, macho_module_info->file_map = fmap; reset_file_map(&fmap); - if (dbghelp_options & SYMOPT_DEFERRED_LOADS) - macho_info->module->module.SymType = SymDeferred; - else if (!macho_load_debug_info(pcs, macho_info->module)) - ret = FALSE; macho_info->module->format_info[DFI_MACHO]->u.macho_info->in_use = 1; macho_info->module->format_info[DFI_MACHO]->u.macho_info->is_loader = 0; diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index 8f8820f90a9..5c439dd1d3c 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -200,7 +200,7 @@ struct module* module_new(struct process* pcs, const WCHAR* name, module_set_module(module, name); module->module.ImageName[0] = '\0'; lstrcpynW(module->module.LoadedImageName, name, ARRAY_SIZE(module->module.LoadedImageName)); - module->module.SymType = SymNone; + module->module.SymType = SymDeferred; module->module.NumSyms = 0; module->module.TimeDateStamp = stamp; module->module.CheckSum = checksum; @@ -350,54 +350,59 @@ struct module* module_get_containee(const struct process* pcs, const struct modu return NULL; } -/****************************************************************** - * module_get_debug - * - * get the debug information from a module: - * - if the module's type is deferred, then force loading of debug info (and return - * the module itself) - * - if the module has no debug info and has an ELF container, then return the ELF - * container (and also force the ELF container's debug info loading if deferred) - * - otherwise return the module itself if it has some debug info - */ -BOOL module_get_debug(struct module_pair* pair) +BOOL module_load_debug(struct module* module) { IMAGEHLP_DEFERRED_SYMBOL_LOADW64 idslW64; - if (!pair->requested) return FALSE; - /* for a PE builtin, always get info from container */ - if (!(pair->effective = module_get_container(pair->pcs, pair->requested))) - pair->effective = pair->requested; /* if deferred, force loading */ - if (pair->effective->module.SymType == SymDeferred) + if (module->module.SymType == SymDeferred) { BOOL ret; - if (pair->effective->is_virtual) ret = FALSE; - else if (pair->effective->type == DMT_PE) + if (module->is_virtual) ret = FALSE; + else if (module->type == DMT_PE) { idslW64.SizeOfStruct = sizeof(idslW64); - idslW64.BaseOfImage = pair->effective->module.BaseOfImage; - idslW64.CheckSum = pair->effective->module.CheckSum; - idslW64.TimeDateStamp = pair->effective->module.TimeDateStamp; - memcpy(idslW64.FileName, pair->effective->module.ImageName, - sizeof(pair->effective->module.ImageName)); + idslW64.BaseOfImage = module->module.BaseOfImage; + idslW64.CheckSum = module->module.CheckSum; + idslW64.TimeDateStamp = module->module.TimeDateStamp; + memcpy(idslW64.FileName, module->module.ImageName, + sizeof(module->module.ImageName)); idslW64.Reparse = FALSE; idslW64.hFile = INVALID_HANDLE_VALUE; - pcs_callback(pair->pcs, CBA_DEFERRED_SYMBOL_LOAD_START, &idslW64); - ret = pe_load_debug_info(pair->pcs, pair->effective); - pcs_callback(pair->pcs, + pcs_callback(module->process, CBA_DEFERRED_SYMBOL_LOAD_START, &idslW64); + ret = pe_load_debug_info(module->process, module); + pcs_callback(module->process, ret ? CBA_DEFERRED_SYMBOL_LOAD_COMPLETE : CBA_DEFERRED_SYMBOL_LOAD_FAILURE, &idslW64); } - else ret = pair->pcs->loader->load_debug_info(pair->pcs, pair->effective); + else ret = module->process->loader->load_debug_info(module->process, module); - if (!ret) pair->effective->module.SymType = SymNone; - assert(pair->effective->module.SymType != SymDeferred); - pair->effective->module.NumSyms = pair->effective->ht_symbols.num_elts; + if (!ret) module->module.SymType = SymNone; + assert(module->module.SymType != SymDeferred); + module->module.NumSyms = module->ht_symbols.num_elts; } - return pair->effective->module.SymType != SymNone; + return module->module.SymType != SymNone; +} + +/****************************************************************** + * module_get_debug + * + * get the debug information from a module: + * - if the module's type is deferred, then force loading of debug info (and return + * the module itself) + * - if the module has no debug info and has an ELF container, then return the ELF + * container (and also force the ELF container's debug info loading if deferred) + * - otherwise return the module itself if it has some debug info + */ +BOOL module_get_debug(struct module_pair* pair) +{ + if (!pair->requested) return FALSE; + /* for a PE builtin, always get info from container */ + if (!(pair->effective = module_get_container(pair->pcs, pair->requested))) + pair->effective = pair->requested; + return module_load_debug(pair->effective); } /*********************************************************************** @@ -966,7 +971,6 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam WARN("Couldn't locate %s\n", debugstr_w(wImageName)); return 0; } - module->module.NumSyms = module->ht_symbols.num_elts; /* by default module_new fills module.ModuleName from a derivation * of LoadedImageName. Overwrite it, if we have better information */ @@ -974,7 +978,8 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam module_set_module(module, wModuleName); if (wImageName) lstrcpynW(module->module.ImageName, wImageName, ARRAY_SIZE(module->module.ImageName)); - + if ((dbghelp_options & SYMOPT_DEFERRED_LOADS) == 0) + module_load_debug(module); return module->module.BaseOfImage; } @@ -1494,6 +1499,7 @@ static struct module* native_load_module(struct process* pcs, const WCHAR* name, static BOOL native_load_debug_info(struct process* process, struct module* module) { + module->module.SymType = SymNone; return FALSE; } diff --git a/dlls/dbghelp/pe_module.c b/dlls/dbghelp/pe_module.c index 77ae122901a..e0266db0387 100644 --- a/dlls/dbghelp/pe_module.c +++ b/dlls/dbghelp/pe_module.c @@ -751,7 +751,7 @@ BOOL pe_load_debug_info(const struct process* pcs, struct module* module) /* FIXME shouldn't we check that? if (!module_get_debug(pcs, module)) */ if (pe_load_export_debug_info(pcs, module) && !ret) ret = TRUE; - + if (!ret) module->module.SymType = SymNone; return ret; } @@ -797,8 +797,6 @@ struct module* pe_load_native_module(struct process* pcs, const WCHAR* name, opened = TRUE; } else if (name) lstrcpyW(loaded_name, name); - else if (dbghelp_options & SYMOPT_DEFERRED_LOADS) - FIXME("Trouble ahead (no module name passed in deferred mode)\n"); if (!(modfmt = HeapAlloc(GetProcessHeap(), 0, sizeof(struct module_format) + sizeof(struct pe_module_info)))) return NULL; modfmt->u.pe_info = (struct pe_module_info*)(modfmt + 1); @@ -824,12 +822,7 @@ struct module* pe_load_native_module(struct process* pcs, const WCHAR* name, modfmt->module = module; modfmt->remove = pe_module_remove; modfmt->loc_compute = NULL; - module->format_info[DFI_PE] = modfmt; - if (dbghelp_options & SYMOPT_DEFERRED_LOADS) - module->module.SymType = SymDeferred; - else - pe_load_debug_info(pcs, module); module->reloc_delta = base - PE_FROM_OPTHDR(&modfmt->u.pe_info->fmap, ImageBase); } else
this replaces existing collision detection which is based on module name this ensures that there is no other module present at module's load address (that's good as all modules are identified in dbghelp by their base address) this allows loading twice the same module at different addresses (and even two different modules yet with same module name like foo.dll and foo.drv) Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/module.c | 73 ++++++++++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index 5c439dd1d3c..64d4e3113d5 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -900,7 +900,8 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam PMODLOAD_DATA Data, DWORD Flags) { struct process* pcs; - struct module* module = NULL; + struct module* module = NULL; + struct module* altmodule; TRACE("(%p %p %s %s %s %08x %p %08x)\n", hProcess, hFile, debugstr_w(wImageName), debugstr_w(wModuleName), @@ -912,16 +913,6 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam if (!(pcs = process_find_by_handle(hProcess))) return 0; - if (Flags & SLMFLAG_VIRTUAL) - { - if (!wImageName) return 0; - module = module_new(pcs, wImageName, DMT_PE, TRUE, BaseOfDll, SizeOfDll, 0, 0, IMAGE_FILE_MACHINE_UNKNOWN); - if (!module) return 0; - if (wModuleName) module_set_module(module, wModuleName); - module->module.SymType = SymVirtual; - - return module->module.BaseOfImage; - } if (Flags & ~(SLMFLAG_VIRTUAL)) FIXME("Unsupported Flags %08x for %s\n", Flags, debugstr_w(wImageName)); @@ -930,31 +921,18 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam /* this is a Wine extension to the API just to redo the synchronisation */ if (!wImageName && !hFile) return 0; - /* check if the module is already loaded, or if it's a builtin PE module with - * an containing ELF module - */ - if (wImageName) + if (Flags & SLMFLAG_VIRTUAL) { - module = module_is_already_loaded(pcs, wImageName); - if (module) - { - if (module->module.BaseOfImage == BaseOfDll) - SetLastError(ERROR_SUCCESS); - else - { - /* native allows to load the same module at different addresses - * we don't support this for now - */ - SetLastError(ERROR_INVALID_PARAMETER); - FIXME("Reloading %s at different base address isn't supported\n", debugstr_w(module->modulename)); - } - return 0; - } - if (!module && module_is_container_loaded(pcs, wImageName, BaseOfDll)) - { - /* force the loading of DLL as builtin */ - module = pe_load_builtin_module(pcs, wImageName, BaseOfDll, SizeOfDll); - } + if (!wImageName) return 0; + module = module_new(pcs, wImageName, DMT_PE, TRUE, BaseOfDll, SizeOfDll, 0, 0, IMAGE_FILE_MACHINE_UNKNOWN); + if (!module) return 0; + module->module.SymType = SymVirtual; + } + /* check if it's a builtin PE module with a containing ELF module */ + else if (wImageName && module_is_container_loaded(pcs, wImageName, BaseOfDll)) + { + /* force the loading of DLL as builtin */ + module = pe_load_builtin_module(pcs, wImageName, BaseOfDll, SizeOfDll); } if (!module) { @@ -978,6 +956,31 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam module_set_module(module, wModuleName); if (wImageName) lstrcpynW(module->module.ImageName, wImageName, ARRAY_SIZE(module->module.ImageName)); + + for (altmodule = pcs->lmodules; altmodule; altmodule = altmodule->next) + { + if (altmodule != module && altmodule->type == module->type && + module->module.BaseOfImage >= altmodule->module.BaseOfImage && + module->module.BaseOfImage < altmodule->module.BaseOfImage + altmodule->module.ImageSize) + break; + } + if (altmodule) + { + /* we have a conflict as the new module cannot be found by its base address + * we need to get rid of one on the two modules + */ + /* loading same module at same address... don't change anything */ + if (module->module.BaseOfImage == altmodule->module.BaseOfImage) + { + module_remove(pcs, module); + SetLastError(ERROR_SUCCESS); + return 0; + } + module_remove(pcs, module); + SetLastError(ERROR_INVALID_PARAMETER); + return 0; + } + if ((dbghelp_options & SYMOPT_DEFERRED_LOADS) == 0) module_load_debug(module); return module->module.BaseOfImage;
mimic what native does... Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/dbghelp/module.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index 64d4e3113d5..fe65683c6e1 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -969,6 +969,14 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam /* we have a conflict as the new module cannot be found by its base address * we need to get rid of one on the two modules */ + if (lstrcmpW(module->modulename, altmodule->modulename) != 0) + { + /* module overlaps an existing but different module... unload new module and return error */ + WARN("%ls overlaps %ls\n", module->modulename, altmodule->modulename); + module_remove(pcs, module); + SetLastError(ERROR_INVALID_PARAMETER); + return 0; + } /* loading same module at same address... don't change anything */ if (module->module.BaseOfImage == altmodule->module.BaseOfImage) { @@ -976,9 +984,10 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam SetLastError(ERROR_SUCCESS); return 0; } - module_remove(pcs, module); - SetLastError(ERROR_INVALID_PARAMETER); - return 0; + /* replace old module with new one, which will look like a shift of base address */ + WARN("Shift module %ls from %I64x to %I64x\n", + module->modulename, altmodule->module.BaseOfImage, module->module.BaseOfImage); + module_remove(pcs, altmodule); } if ((dbghelp_options & SYMOPT_DEFERRED_LOADS) == 0)
participants (1)
-
Eric Pouech