[PATCH 0/1] MR3016: dbghelp: Fix memory leak on error path in pe_load_native_module (scan-build).
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/dbghelp/pe_module.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dlls/dbghelp/pe_module.c b/dlls/dbghelp/pe_module.c index 7aeb1cf8901..79ab8aa9d21 100644 --- a/dlls/dbghelp/pe_module.c +++ b/dlls/dbghelp/pe_module.c @@ -819,7 +819,10 @@ struct module* pe_load_native_module(struct process* pcs, const WCHAR* name, if (name) lstrcpyW(loaded_name, name); } if (!(modfmt = HeapAlloc(GetProcessHeap(), 0, sizeof(struct module_format) + sizeof(struct pe_module_info)))) + { + free(real_path); return NULL; + } modfmt->u.pe_info = (struct pe_module_info*)(modfmt + 1); if (pe_map_file(hFile, &modfmt->u.pe_info->fmap, DMT_PE)) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3016
(un)fortunately, this sheds some light on existing discrepancy in memory management real_path is either allocated by crt (wcsdup in pe_load_native_module) or heap (in search_dll_path), and freed from heap (pe_load_native_module and module_remove) so I'd rather keep this rule of thumb: - long lived allocation: use heap (and even better use module's heap, so we don't have to free them one by one) - short live allocation (inside 1 function): use crt (btw, it looks "interesting" that none of the analysers caught this before) so I'd rather go for replacing the wcsdup with a global heap allocation in pe_load_native_module, and heap_free in what you're proposing in your patch -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3016#note_35084
On Sat Jun 10 19:22:55 2023 +0000, eric pouech wrote:
(un)fortunately, this sheds some light on existing discrepancy in memory management real_path is either allocated by crt (wcsdup in pe_load_native_module) or heap (in search_dll_path), and freed from heap (pe_load_native_module and module_remove) so I'd rather keep this rule of thumb: - long lived allocation: use heap (and even better use module's heap, so we don't have to free them one by one) - short live allocation (inside 1 function): use crt (btw, it looks "interesting" that none of the analysers caught this before) so I'd rather go for replacing the wcsdup with a global heap allocation in pe_load_native_module, and heap_free in what you're proposing in your patch Interesting. That's definitely a problem.
The downside to allocating real_path with `HeapAlloc(GetProcessHeap(), ...)` is that we'd have to reimplement strdup. Are you sure that's better than using the CRT in this case? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3016#note_35338
On Sat Jun 10 19:22:55 2023 +0000, Alex Henrie wrote:
Interesting. That's definitely a problem. The downside to allocating real_path with `HeapAlloc(GetProcessHeap(), ...)` is that we'd have to reimplement strdup. Are you sure that's better than using the CRT in this case? actually, the best way would be to use the pool_ helpers (each module (DLL with debug info avail) in dbghelp has its own heap, where are stored objects which life time is the one of the module: the main benefit is that the objects are not deallocated one by one, but the heap is destroyed when the module is unloaded) the real_path is just another attribute of the module, so allocating inside the module's pool is the best option
you may have to implement pool_wcsdup though, add the module to struct build_search and remove the deallocation of real_path in module_remove -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3016#note_35370
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
eric pouech (＠epo)