[PATCH 0/1] MR3016: dbghelp: Fix memory leak on error path in pe_load_native_module (scan-build).

[PATCH 1/1] dbghelp: Fix memory leak on error path in pe_load_native_module (scan-build).

[Alex Henrie]

From: Alex Henrie

---
 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))
     {

[eric pouech (＠epo)]

(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

[Alex Henrie (＠alexhenrie)]

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?

[eric pouech (＠epo)]

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