[PATCH 0/3] MR6825: Implement dbghelp.SymRefreshModuleList().

[eric pouech (＠epo)]

This MR main goal is to implement properly dbghelp's
SymRefreshModuleList.

It also includes potential fixes for some spurious failures
in dbghelp tests on Windows 11 (to be verified).

It also tidies up the loader detection (ELF and Mach-O should
now behave in the same way).

[PATCH 1/3] dbghelp/tests: Add retry wrapper around SymRefreshModuleList().

[Eric Pouech]

From: Eric Pouech

Signed-off-by: Eric Pouech 
---
 dlls/dbghelp/tests/dbghelp.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/dlls/dbghelp/tests/dbghelp.c b/dlls/dbghelp/tests/dbghelp.c
index ae757e1ae08..573d9cd70d9 100644
--- a/dlls/dbghelp/tests/dbghelp.c
+++ b/dlls/dbghelp/tests/dbghelp.c
@@ -424,6 +424,28 @@ static BOOL wrapper_EnumerateLoadedModulesW64(HANDLE proc, PENUMLOADED_MODULES_C
     return ret;
 }

+/* wrapper around SymRefreshModuleList which sometimes fails (it's very likely implemented on top
+ * of EnumerateLoadedModulesW64 on native too)
+ */
+static BOOL wrapper_SymRefreshModuleList(HANDLE proc)
+{
+    BOOL ret;
+    int retry;
+    int retry_count = !strcmp(winetest_platform, "wine") ? 1 : 5;
+
+    for (retry = retry_count - 1; retry >= 0; retry--)
+    {
+        ret = SymRefreshModuleList(proc);
+        if (ret || (GetLastError() != STATUS_INFO_LENGTH_MISMATCH && GetLastError() == STATUS_PARTIAL_COPY))
+            break;
+        Sleep(10);
+    }
+    if (retry + 1 < retry_count)
+        trace("used wrapper retry: ret=%d retry=%d top=%d\n", ret, retry, retry_count);
+
+    return ret;
+}
+
 static BOOL test_modules(void)
 {
     BOOL ret;
@@ -492,6 +514,8 @@ static BOOL test_modules(void)

ret = SymRefreshModuleList(dummy);
     ok(!ret, "SymRefreshModuleList should have failed\n");
+    todo_wine
+    ok(GetLastError() == STATUS_INVALID_CID, "Unexpected last error %lx\n", GetLastError());

count = get_module_count(dummy);
     ok(count == 0, "Unexpected count (%u instead of 0)\n", count);
@@ -885,9 +909,11 @@ static void test_loaded_modules(void)

pcskind = get_process_kind(pi.hProcess);

-    ret = SymRefreshModuleList(pi.hProcess);
+    ret = wrapper_SymRefreshModuleList(pi.hProcess);
     todo_wine_if(pcskind == PCSKIND_WOW64)
-    ok(ret || broken(GetLastError() == STATUS_PARTIAL_COPY /* Win11 in some cases */), "SymRefreshModuleList failed: %lu\n", GetLastError());
+    ok(ret || broken(GetLastError() == STATUS_PARTIAL_COPY /* Win11 in some cases */ ||
+                             GetLastError() == STATUS_INFO_LENGTH_MISMATCH /* Win11 in some cases */),
+       "SymRefreshModuleList failed: %lu\n", GetLastError());

if (!strcmp(winetest_platform, "wine"))
     {
@@ -945,7 +971,7 @@ static void test_loaded_modules(void)
                    "Wrong directory aggregation count %u %u\n",
                    aggregation.count_systemdir, aggregation.count_wowdir);
             }
-            ret = SymRefreshModuleList(pi.hProcess);
+            ret = wrapper_SymRefreshModuleList(pi.hProcess);
             ok(ret, "SymRefreshModuleList failed: %lu\n", GetLastError());

if (!strcmp(winetest_platform, "wine"))
@@ -1007,7 +1033,7 @@ static void test_loaded_modules(void)
                 break;
             }

-            ret = SymRefreshModuleList(pi.hProcess);
+            ret = wrapper_SymRefreshModuleList(pi.hProcess);
             ok(ret, "SymRefreshModuleList failed: %lu\n", GetLastError());

if (!strcmp(winetest_platform, "wine"))

[PATCH 2/3] dbghelp/tests: Add tests for SymRefreshModuleList().

[Eric Pouech]

From: Eric Pouech

Signed-off-by: Eric Pouech 
---
 dlls/dbghelp/tests/dbghelp.c | 112 ++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 3 deletions(-)

diff --git a/dlls/dbghelp/tests/dbghelp.c b/dlls/dbghelp/tests/dbghelp.c
index 573d9cd70d9..fab1c670b5d 100644
--- a/dlls/dbghelp/tests/dbghelp.c
+++ b/dlls/dbghelp/tests/dbghelp.c
@@ -372,6 +372,29 @@ static unsigned get_native_module_count(HANDLE proc)
     return count;
 }

+struct module_present
+{
+    const WCHAR* module_name;
+    BOOL found;
+};
+
+static BOOL CALLBACK is_module_present_cb(const WCHAR* name, DWORD64 base, void* usr)
+{
+    struct module_present* present = usr;
+    if (!wcsicmp(name, present->module_name))
+    {
+        present->found = TRUE;
+        return FALSE;
+    }
+    return TRUE;
+}
+
+static BOOL is_module_present(HANDLE proc, const WCHAR* module_name)
+{
+    struct module_present present = { .module_name = module_name };
+    return SymEnumerateModulesW64(proc, is_module_present_cb, &present) && present.found;
+}
+
 struct nth_module
 {
     HANDLE              proc;
@@ -849,6 +872,12 @@ static void test_loaded_modules(void)
     ok(ret, "got error %lu\n", GetLastError());
     strcat(buffer, "\msinfo32.exe");

+    /* testing invalid process handle */
+    ret = wrapper_EnumerateLoadedModulesW64((HANDLE)(ULONG_PTR)0xffffffc0, NULL, FALSE);
+    ok(!ret, "EnumerateLoadedModulesW64 should have failed\n");
+    todo_wine
+    ok(GetLastError() == STATUS_INVALID_CID, "Unexpected last error %lx\n", GetLastError());
+
     /* testing with child process of different machines */
     ret = CreateProcessA(NULL, buffer, NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi);
     ok(ret, "CreateProcess failed: %lu\n", GetLastError());
@@ -913,7 +942,7 @@ static void test_loaded_modules(void)
     todo_wine_if(pcskind == PCSKIND_WOW64)
     ok(ret || broken(GetLastError() == STATUS_PARTIAL_COPY /* Win11 in some cases */ ||
                              GetLastError() == STATUS_INFO_LENGTH_MISMATCH /* Win11 in some cases */),
-       "SymRefreshModuleList failed: %lu\n", GetLastError());
+       "SymRefreshModuleList failed: %lx\n", GetLastError());

if (!strcmp(winetest_platform, "wine"))
     {
@@ -972,7 +1001,7 @@ static void test_loaded_modules(void)
                    aggregation.count_systemdir, aggregation.count_wowdir);
             }
             ret = wrapper_SymRefreshModuleList(pi.hProcess);
-            ok(ret, "SymRefreshModuleList failed: %lu\n", GetLastError());
+            ok(ret, "SymRefreshModuleList failed: %lx\n", GetLastError());

if (!strcmp(winetest_platform, "wine"))
             {
@@ -1034,7 +1063,7 @@ static void test_loaded_modules(void)
             }

ret = wrapper_SymRefreshModuleList(pi.hProcess);
-            ok(ret, "SymRefreshModuleList failed: %lu\n", GetLastError());
+            ok(ret, "SymRefreshModuleList failed: %lx\n", GetLastError());

if (!strcmp(winetest_platform, "wine"))
             {
@@ -1555,6 +1584,82 @@ static void test_function_tables(void)
     SymCleanup(GetCurrentProcess());
 }

+static void test_refresh_modules(void)
+{
+    BOOL ret;
+    unsigned count, count_current;
+    HMODULE hmod;
+    IMAGEHLP_MODULEW64 module_info = { .SizeOfStruct = sizeof(module_info) };
+
+    /* pick a DLL: which isn't already loaded by test, and that will not load other DLLs for deps */
+    static const WCHAR* unused_dll = L"psapi";
+
+    ret = SymInitialize(GetCurrentProcess(), 0, TRUE);
+    ok(ret, "SymInitialize failed: %lu\n", GetLastError());
+
+    count = get_module_count(GetCurrentProcess());
+    ok(count, "Unexpected module count %u\n", count);
+
+    ret = SymCleanup(GetCurrentProcess());
+    ok(ret, "SymCleanup failed: %lu\n", GetLastError());
+
+    ret = SymInitialize(GetCurrentProcess(), 0, FALSE);
+    ok(ret, "SymInitialize failed: %lu\n", GetLastError());
+
+    count_current = get_module_count(GetCurrentProcess());
+    ok(!count_current, "Unexpected module count %u\n", count_current);
+
+    ret = wrapper_SymRefreshModuleList(GetCurrentProcess());
+    ok(ret, "SymRefreshModuleList failed: %lx\n", GetLastError());
+
+    count_current = get_module_count(GetCurrentProcess());
+    todo_wine
+    ok(count == count_current, "Unexpected module count %u, %u\n", count, count_current);
+
+    hmod = GetModuleHandleW(unused_dll);
+    ok(hmod == NULL, "Expecting DLL %ls not be loaded\n", unused_dll);
+
+    hmod = LoadLibraryW(unused_dll);
+    ok(hmod != NULL, "LoadLibraryW failed: %lu\n", GetLastError());
+
+    count_current = get_module_count(GetCurrentProcess());
+    todo_wine
+    ok(count == count_current, "Unexpected module count %u, %u\n", count, count_current);
+    ret = is_module_present(GetCurrentProcess(), unused_dll);
+    ok(!ret, "Couldn't find module %ls\n", unused_dll);
+
+    ret = wrapper_SymRefreshModuleList(GetCurrentProcess());
+    ok(ret, "SymRefreshModuleList failed: %lx\n", GetLastError());
+
+    count_current = get_module_count(GetCurrentProcess());
+    todo_wine
+    ok(count + 1 == count_current, "Unexpected module count %u, %u\n", count, count_current);
+    ret = is_module_present(GetCurrentProcess(), unused_dll);
+    todo_wine
+    ok(ret, "Couldn't find module %ls\n", unused_dll);
+
+    ret = FreeLibrary(hmod);
+    ok(ret, "LoadLibraryW failed: %lu\n", GetLastError());
+
+    count_current = get_module_count(GetCurrentProcess());
+    todo_wine
+    ok(count + 1 == count_current, "Unexpected module count %u, %u\n", count, count_current);
+
+    ret = wrapper_SymRefreshModuleList(GetCurrentProcess());
+    ok(ret, "SymRefreshModuleList failed: %lx\n", GetLastError());
+
+    /* SymRefreshModuleList() doesn't remove the unloaded modules... */
+    count_current = get_module_count(GetCurrentProcess());
+    todo_wine
+    ok(count + 1 == count_current, "Unexpected module count %u != %u\n", count, count_current);
+    ret = is_module_present(GetCurrentProcess(), unused_dll);
+    todo_wine
+    ok(ret, "Couldn't find module %ls\n", unused_dll);
+
+    ret = SymCleanup(GetCurrentProcess());
+    ok(ret, "SymCleanup failed: %lu\n", GetLastError());
+}
+
 START_TEST(dbghelp)
 {
     BOOL ret;
@@ -1585,6 +1690,7 @@ START_TEST(dbghelp)
         test_modules_overlap();
         test_loaded_modules();
         test_live_modules();
+        test_refresh_modules();
     }
     test_function_tables();
 }

[PATCH 3/3] dbghelp: Implement SymRefreshModuleList().

[Eric Pouech]

From: Eric Pouech

Signed-off-by: Eric Pouech 
---
 dlls/dbghelp/dbghelp.c         | 22 ++++++--------------
 dlls/dbghelp/dbghelp_private.h |  1 +
 dlls/dbghelp/elf_module.c      |  1 -
 dlls/dbghelp/module.c          | 37 ++++++++++++++++++++++++++++++----
 dlls/dbghelp/tests/dbghelp.c   | 10 ---------
 5 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/dlls/dbghelp/dbghelp.c b/dlls/dbghelp/dbghelp.c
index cc1b91d5e77..5a023152ec4 100644
--- a/dlls/dbghelp/dbghelp.c
+++ b/dlls/dbghelp/dbghelp.c
@@ -300,20 +300,6 @@ BOOL WINAPI SymGetSearchPath(HANDLE hProcess, PSTR szSearchPath,
     return ret;
 }

-/******************************************************************
- *		invade_process
- *
- * SymInitialize helper: loads in dbghelp all known (and loaded modules)
- * this assumes that hProcess is a handle on a valid process
- */
-static BOOL WINAPI process_invade_cb(PCWSTR name, ULONG64 base, ULONG size, PVOID user)
-{
-    HANDLE      hProcess = user;
-
-    SymLoadModuleExW(hProcess, 0, name, NULL, base, size, NULL, 0);
-    return TRUE;
-}
-
 const WCHAR *process_getenv(const struct process *process, const WCHAR *name)
 {
     size_t name_len;
@@ -432,7 +418,10 @@ static BOOL check_live_target(struct process* pcs, BOOL wow64, BOOL child_wow64)

TRACE("got debug info address %#I64x from PEB %p\n", base, pbi.PebBaseAddress);
     if (!elf_read_wine_loader_dbg_info(pcs, base) && !macho_read_wine_loader_dbg_info(pcs, base))
+    {
         WARN("couldn't load process debug info at %#I64x\n", base);
+        pcs->loader = &empty_loader_ops;
+    }
     return TRUE;
 }

@@ -510,8 +499,9 @@ BOOL WINAPI SymInitializeW(HANDLE hProcess, PCWSTR UserSearchPath, BOOL fInvadeP
     if (check_live_target(pcs, wow64, child_wow64))
     {
         if (fInvadeProcess)
-            EnumerateLoadedModulesW64(hProcess, process_invade_cb, hProcess);
-        if (pcs->loader) pcs->loader->synchronize_module_list(pcs);
+            module_refresh_list(pcs);
+        else
+            pcs->loader->synchronize_module_list(pcs);
     }
     else if (fInvadeProcess)
     {
diff --git a/dlls/dbghelp/dbghelp_private.h b/dlls/dbghelp/dbghelp_private.h
index 33714e73bc1..d8761bd599a 100644
--- a/dlls/dbghelp/dbghelp_private.h
+++ b/dlls/dbghelp/dbghelp_private.h
@@ -743,6 +743,7 @@ extern BOOL         module_remove(struct process* pcs,
 extern void         module_set_module(struct module* module, const WCHAR* name);
 extern WCHAR*       get_wine_loader_name(struct process *pcs) __WINE_DEALLOC(HeapFree, 3) __WINE_MALLOC;
 extern BOOL         module_is_wine_host(const WCHAR* module_name, const WCHAR* ext);
+extern BOOL         module_refresh_list(struct process *pcs);

/* msc.c */
 extern BOOL         pe_load_debug_directory(const struct process* pcs,
diff --git a/dlls/dbghelp/elf_module.c b/dlls/dbghelp/elf_module.c
index da4486a00c8..a4cf37942f0 100644
--- a/dlls/dbghelp/elf_module.c
+++ b/dlls/dbghelp/elf_module.c
@@ -1785,7 +1785,6 @@ BOOL elf_read_wine_loader_dbg_info(struct process* pcs, ULONG_PTR 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);
diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c
index 0ade470961b..4b4dbbaf595 100644
--- a/dlls/dbghelp/module.c
+++ b/dlls/dbghelp/module.c
@@ -24,6 +24,8 @@
 #include <string.h>
 #include <assert.h>

+#include "ntstatus.h"
+#define WIN32_NO_STATUS
 #include "dbghelp_private.h"
 #include "image_private.h"
 #include "psapi.h"
@@ -1317,7 +1319,11 @@ BOOL  WINAPI EnumerateLoadedModulesW64(HANDLE process,
     size_t              sysdir_len = 0, wowdir_len = 0;

/* process might not be a handle to a live process */
-    if (!IsWow64Process2(process, &pcs_machine, &native_machine)) return FALSE;
+    if (!IsWow64Process2(process, &pcs_machine, &native_machine))
+    {
+        SetLastError(STATUS_INVALID_CID);
+        return FALSE;
+    }
     with_32bit_modules = sizeof(void*) > sizeof(int) &&
         pcs_machine != IMAGE_FILE_MACHINE_UNKNOWN &&
         (dbghelp_options & SYMOPT_INCLUDE_32BIT_MODULES);
@@ -1600,18 +1606,41 @@ void module_reset_debug_info(struct module* module)
     module->sources = NULL;
 }

+static BOOL WINAPI process_invade_cb(PCWSTR name, ULONG64 base, ULONG size, PVOID user)
+{
+    HANDLE      hProcess = user;
+
+    /* Note: this follows native behavior:
+     * If a PE module has been unloaded from debuggee, it's not immediately removed
+     * from module list in dbghelp.
+     * Removal may eventually happen when loading a another module with SymLoadModule:
+     * if the module to be loaded overlaps an existing one, SymLoadModule will
+     * automatically unload the eldest one.
+     */
+    SymLoadModuleExW(hProcess, 0, name, NULL, base, size, NULL, 0);
+    return TRUE;
+}
+
+BOOL module_refresh_list(struct process *pcs)
+{
+    BOOL ret;
+
+    ret = pcs->loader->synchronize_module_list(pcs);
+    ret = EnumerateLoadedModulesW64(pcs->handle, process_invade_cb, pcs->handle) && ret;
+    return ret;
+}
+
 /******************************************************************
  *              SymRefreshModuleList (DBGHELP.@)
  */
 BOOL WINAPI SymRefreshModuleList(HANDLE hProcess)
 {
-    struct process*     pcs;
+    struct process *pcs;

TRACE("(%p)\n", hProcess);

if (!(pcs = process_find_by_handle(hProcess))) return FALSE;
-
-    return pcs->loader->synchronize_module_list(pcs);
+    return module_refresh_list(pcs);
 }

/***********************************************************************
diff --git a/dlls/dbghelp/tests/dbghelp.c b/dlls/dbghelp/tests/dbghelp.c
index fab1c670b5d..f7313c3ab4f 100644
--- a/dlls/dbghelp/tests/dbghelp.c
+++ b/dlls/dbghelp/tests/dbghelp.c
@@ -537,7 +537,6 @@ static BOOL test_modules(void)

ret = SymRefreshModuleList(dummy);
     ok(!ret, "SymRefreshModuleList should have failed\n");
-    todo_wine
     ok(GetLastError() == STATUS_INVALID_CID, "Unexpected last error %lx\n", GetLastError());

count = get_module_count(dummy);
@@ -875,7 +874,6 @@ static void test_loaded_modules(void)
     /* testing invalid process handle */
     ret = wrapper_EnumerateLoadedModulesW64((HANDLE)(ULONG_PTR)0xffffffc0, NULL, FALSE);
     ok(!ret, "EnumerateLoadedModulesW64 should have failed\n");
-    todo_wine
     ok(GetLastError() == STATUS_INVALID_CID, "Unexpected last error %lx\n", GetLastError());

/* testing with child process of different machines */
@@ -939,7 +937,6 @@ static void test_loaded_modules(void)
     pcskind = get_process_kind(pi.hProcess);

ret = wrapper_SymRefreshModuleList(pi.hProcess);
-    todo_wine_if(pcskind == PCSKIND_WOW64)
     ok(ret || broken(GetLastError() == STATUS_PARTIAL_COPY /* Win11 in some cases */ ||
                              GetLastError() == STATUS_INFO_LENGTH_MISMATCH /* Win11 in some cases */),
        "SymRefreshModuleList failed: %lx\n", GetLastError());
@@ -1613,7 +1610,6 @@ static void test_refresh_modules(void)
     ok(ret, "SymRefreshModuleList failed: %lx\n", GetLastError());

count_current = get_module_count(GetCurrentProcess());
-    todo_wine
     ok(count == count_current, "Unexpected module count %u, %u\n", count, count_current);

hmod = GetModuleHandleW(unused_dll);
@@ -1623,7 +1619,6 @@ static void test_refresh_modules(void)
     ok(hmod != NULL, "LoadLibraryW failed: %lu\n", GetLastError());

count_current = get_module_count(GetCurrentProcess());
-    todo_wine
     ok(count == count_current, "Unexpected module count %u, %u\n", count, count_current);
     ret = is_module_present(GetCurrentProcess(), unused_dll);
     ok(!ret, "Couldn't find module %ls\n", unused_dll);
@@ -1632,17 +1627,14 @@ static void test_refresh_modules(void)
     ok(ret, "SymRefreshModuleList failed: %lx\n", GetLastError());

count_current = get_module_count(GetCurrentProcess());
-    todo_wine
     ok(count + 1 == count_current, "Unexpected module count %u, %u\n", count, count_current);
     ret = is_module_present(GetCurrentProcess(), unused_dll);
-    todo_wine
     ok(ret, "Couldn't find module %ls\n", unused_dll);

ret = FreeLibrary(hmod);
     ok(ret, "LoadLibraryW failed: %lu\n", GetLastError());

count_current = get_module_count(GetCurrentProcess());
-    todo_wine
     ok(count + 1 == count_current, "Unexpected module count %u, %u\n", count, count_current);

ret = wrapper_SymRefreshModuleList(GetCurrentProcess());
@@ -1650,10 +1642,8 @@ static void test_refresh_modules(void)

/* SymRefreshModuleList() doesn't remove the unloaded modules... */
     count_current = get_module_count(GetCurrentProcess());
-    todo_wine
     ok(count + 1 == count_current, "Unexpected module count %u != %u\n", count, count_current);
     ret = is_module_present(GetCurrentProcess(), unused_dll);
-    todo_wine
     ok(ret, "Couldn't find module %ls\n", unused_dll);

ret = SymCleanup(GetCurrentProcess());

[PATCH 3/3] dbghelp: Implement SymRefreshModuleList().

[Marvin]

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=149702

Your paranoid android.

=== debian11b (64 bit WoW report) ===

user32:
win.c:4073: Test succeeded inside todo block: Expected active window 0000000004760178, got 0000000004760178.
win.c:4074: Test succeeded inside todo block: Expected focus window 0000000004760178, got 0000000004760178.