[PATCH v3 0/1] MR2152: dbghelp: Fix failing test on Windows 7.

[eric pouech (＠epo)]

Using old versions of the module information structure.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54465

Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>

--
  v3: dbghelp: Fix failing test on Windows 7.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2152

[PATCH v3 1/1] dbghelp: Fix failing test on Windows 7.

[Eric Pouech]

From: Eric Pouech <eric.pouech(a)gmail.com>

Using old versions of the module information structure.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54465

Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com>
---
 dlls/dbghelp/tests/dbghelp.c | 95 +++++++++++++++++++++---------------
 1 file changed, 56 insertions(+), 39 deletions(-)

diff --git a/dlls/dbghelp/tests/dbghelp.c b/dlls/dbghelp/tests/dbghelp.c
index 713c93b0a02..bcae5c1f875 100644
--- a/dlls/dbghelp/tests/dbghelp.c
+++ b/dlls/dbghelp/tests/dbghelp.c
@@ -193,35 +193,58 @@ static void test_search_path(void)
 
 static USHORT get_module_machine(const char* path)
 {
-    BOOL ret;
     HANDLE hFile, hMap;
     void* mapping;
     IMAGE_NT_HEADERS *nthdr;
-    USHORT machine;
+    USHORT machine = IMAGE_FILE_MACHINE_UNKNOWN;
 
     hFile = CreateFileA(path, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL);
-    ok(hFile != INVALID_HANDLE_VALUE, "Couldn't open file %s (%lu)\n", path, GetLastError());
-    hMap = CreateFileMappingW(hFile, NULL, PAGE_READONLY, 0, 0, NULL);
-    ok(hMap != NULL, "Couldn't create map (%lu)\n", GetLastError());
-    mapping = MapViewOfFile(hMap, FILE_MAP_READ, 0, 0, 0);
-    ok(mapping != NULL, "Couldn't map (%lu)\n", GetLastError());
-    nthdr = RtlImageNtHeader(mapping);
-    ok(nthdr != NULL, "Cannot get NT headers out of %s\n", path);
-    machine = nthdr ? nthdr->FileHeader.Machine : IMAGE_FILE_MACHINE_UNKNOWN;
-    ret = UnmapViewOfFile(mapping);
-    ok(ret, "Couldn't unmap (%lu)\n", GetLastError());
-    CloseHandle(hMap);
-    CloseHandle(hFile);
+    if (hFile != INVALID_HANDLE_VALUE)
+    {
+        hMap = CreateFileMappingW(hFile, NULL, PAGE_READONLY, 0, 0, NULL);
+        if (hMap != NULL)
+        {
+            mapping = MapViewOfFile(hMap, FILE_MAP_READ, 0, 0, 0);
+            if (mapping != NULL)
+            {
+                nthdr = RtlImageNtHeader(mapping);
+                if (nthdr != NULL) machine = nthdr->FileHeader.Machine;
+                UnmapViewOfFile(mapping);
+            }
+            CloseHandle(hMap);
+        }
+        CloseHandle(hFile);
+    }
     return machine;
 }
 
+static BOOL skip_too_old_dbghelp(HANDLE proc, DWORD64 base)
+{
+    IMAGEHLP_MODULE im0 = {sizeof(im0)};
+    BOOL ret;
+
+    /* test if get module info succeeds with oldest structure format */
+    ret = SymGetModuleInfo(proc, base, &im0);
+    if (ret)
+    {
+        skip("Too old dbghelp. Skipping module tests.\n");
+        ret = SymCleanup(proc);
+        ok(ret, "SymCleanup failed: %lu\n", GetLastError());
+        return TRUE;
+    }
+    ok(ret, "SymGetModuleInfo failed: %lu\n", GetLastError());
+    return FALSE;
+}
+
 static void test_modules(void)
 {
     BOOL ret;
-    DWORD attr;
+    char file_system[MAX_PATH];
+    char file_wow64[MAX_PATH];
     const DWORD64 base1 = 0x00010000;
     const DWORD64 base2 = 0x08010000;
     IMAGEHLP_MODULEW64 im;
+    USHORT machine_wow, machine2;
 
     im.SizeOfStruct = sizeof(im);
 
@@ -231,48 +254,42 @@ static void test_modules(void)
     ret = SymInitialize(GetCurrentProcess(), 0, FALSE);
     ok(ret, "SymInitialize failed: %lu\n", GetLastError());
 
+    GetSystemWow64DirectoryA(file_wow64, MAX_PATH);
+    strcat(file_wow64, "\\notepad.exe");
+
     /* not always present */
-    attr = GetFileAttributesA("C:\\windows\\syswow64\\notepad.exe");
-    if (attr != INVALID_FILE_ATTRIBUTES)
+    machine_wow = get_module_machine(file_wow64);
+    if (machine_wow != IMAGE_FILE_MACHINE_UNKNOWN)
     {
-        ret = SymLoadModule(GetCurrentProcess(), NULL, "C:\\windows\\syswow64\\notepad.exe", NULL, base2, 0);
+        ret = SymLoadModule(GetCurrentProcess(), NULL, file_wow64, NULL, base2, 0);
         ok(ret, "SymLoadModule failed: %lu\n", GetLastError());
         ret = SymGetModuleInfoW64(GetCurrentProcess(), base2, &im);
+        if (!ret && skip_too_old_dbghelp(GetCurrentProcess(), base2)) return;
         ok(ret, "SymGetModuleInfoW64 failed: %lu\n", GetLastError());
         ok(im.BaseOfImage == base2, "Wrong base address\n");
-        ok(im.MachineType == get_module_machine("C:\\windows\\syswow64\\notepad.exe"),
-           "Wrong machine %lx\n", im.MachineType);
+        ok(im.MachineType == machine_wow, "Wrong machine %lx (expecting %u)\n", im.MachineType, machine_wow);
     }
 
-    ret = SymLoadModule(GetCurrentProcess(), NULL, "C:\\windows\\system32\\notepad.exe", NULL, base1, 0);
+    GetSystemDirectoryA(file_system, MAX_PATH);
+    strcat(file_system, "\\notepad.exe");
+
+    ret = SymLoadModule(GetCurrentProcess(), NULL, file_system, NULL, base1, 0);
     ok(ret, "SymLoadModule failed: %lu\n", GetLastError());
     ret = SymGetModuleInfoW64(GetCurrentProcess(), base1, &im);
-    /* we want to access IMAGEHLP_MODULE.MachineType, so ensure that error stems from a too old
-     * dbghelp (on Windows), not supporting new enlarged IMAGEHLP_MODULE structures.
-     */
-    if (broken(!ret && GetLastError() == ERROR_INVALID_PARAMETER))
-    {
-        IMAGEHLP_MODULE im0 = {sizeof(im0)};
-        ret = SymGetModuleInfo(GetCurrentProcess(), base1, &im0);
-        ok(ret, "Unexpected error: %lu\n", GetLastError());
-        skip("Too old dbghelp. Skipping module tests\n");
-        ret = SymCleanup(GetCurrentProcess());
-        ok(ret, "SymCleanup failed: %lu\n", GetLastError());
-        return;
-    }
+    if (!ret && skip_too_old_dbghelp(GetCurrentProcess(), base1)) return;
     ok(ret, "SymGetModuleInfoW64 failed: %lu\n", GetLastError());
     ok(im.BaseOfImage == base1, "Wrong base address\n");
-    ok(im.MachineType == get_module_machine("C:\\windows\\system32\\notepad.exe"),
-       "Wrong machine %lx\n", im.MachineType);
+    machine2 = get_module_machine(file_system);
+    ok(machine2 != IMAGE_FILE_MACHINE_UNKNOWN, "Unexpected machine %u\n", machine2);
+    ok(im.MachineType == machine2, "Wrong machine %lx (expecting %u)\n", im.MachineType, machine2);
 
     /* still can access first module after loading second */
-    if (attr != INVALID_FILE_ATTRIBUTES)
+    if (machine_wow != IMAGE_FILE_MACHINE_UNKNOWN)
     {
         ret = SymGetModuleInfoW64(GetCurrentProcess(), base2, &im);
         ok(ret, "SymGetModuleInfoW64 failed: %lu\n", GetLastError());
         ok(im.BaseOfImage == base2, "Wrong base address\n");
-        ok(im.MachineType == get_module_machine("C:\\windows\\syswow64\\notepad.exe"),
-           "Wrong machine %lx\n", im.MachineType);
+        ok(im.MachineType == machine_wow, "Wrong machine %lx (expecting %u)\n", im.MachineType, machine_wow);
     }
 
     ret = SymCleanup(GetCurrentProcess());
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/2152

[eric pouech (＠epo)]

the latest failure stems out of ntdll:threadpool, which is a known failure
(https://bugs.winehq.org//show_bug.cgi?id=54064)

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2152#note_23697

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/dbghelp/tests/dbghelp.c:
> +    {
> +        hMap = CreateFileMappingW(hFile, NULL, PAGE_READONLY, 0, 0, NULL);
> +        if (hMap != NULL)
> +        {
> +            mapping = MapViewOfFile(hMap, FILE_MAP_READ, 0, 0, 0);
> +            if (mapping != NULL)
> +            {
> +                nthdr = RtlImageNtHeader(mapping);
> +                if (nthdr != NULL) machine = nthdr->FileHeader.Machine;
> +                UnmapViewOfFile(mapping);
> +            }
> +            CloseHandle(hMap);
> +        }
> +        CloseHandle(hFile);
> +    }
>      return machine;
> Why do this? This part doesn't fail.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2152#note_23766

[Eric Pouech]

I'm not sure to get your point.
- do you mean that you'd like to ok() test the return values of all the
calls in the helper? I didn't do it as a) the helper can be called with an
inexistant file, b) the cases of failure are ok() tested later on (machine
not being unknown)
- do you mean the calls in the helper can't fail? (I don't see a reason
they wouldn't)
- something else?

Le ven. 10 févr. 2023 à 19:23, Zebediah Figura (@zfigura) <
wine(a)gitlab.winehq.org> a écrit :
> Zebediah Figura (@zfigura) commented about dlls/dbghelp/tests/dbghelp.c:
> > +    {
> > +        hMap = CreateFileMappingW(hFile, NULL, PAGE_READONLY, 0, 0,
> > NULL);
> > +        if (hMap != NULL)
> > +        {
> > +            mapping = MapViewOfFile(hMap, FILE_MAP_READ, 0, 0, 0);
> > +            if (mapping != NULL)
> > +            {
> > +                nthdr = RtlImageNtHeader(mapping);
> > +                if (nthdr != NULL) machine = nthdr->FileHeader.Machine;
> > +                UnmapViewOfFile(mapping);
> > +            }
> > +            CloseHandle(hMap);
> > +        }
> > +        CloseHandle(hFile);
> > +    }
> >      return machine;
> > Why do this? This part doesn't fail.
>
> --
> https://gitlab.winehq.org/wine/wine/-/merge_requests/2152#note_23766

[Zebediah Figura]

On 2/10/23 13:13, Eric Pouech wrote:
> I'm not sure to get your point.
> - do you mean that you'd like to ok() test the return values of all the
> calls in the helper? I didn't do it as a) the helper can be called with an
> inexistant file, b) the cases of failure are ok() tested later on (machine
> not being unknown)
> - do you mean the calls in the helper can't fail? (I don't see a reason
> they wouldn't)
> - something else?

If a call in a test shouldn't fail, we should use ok(). Creating the 
file might fail if it doesn't exist, but I don't see any reason that 
CreateFileMapping() or MapViewOfFile() would fail. I.e. there was 
nothing wrong with the original code here, right?