[PATCH v3 0/1] MR5236: ntdll: Implement NtdllDefWindowProc_A().

[Aida Jonikienė (＠DodoGTA)]

I've tried implementing this before but importing user32 from ntdll isn't a reliable
thing (so I recently rewrote it using the ntdll equivalents of LoadLibrary() and GetProcAddress())

Note: This is a weird way to implement this function (Windows implements this
in ntdll instead) but that would probably require a user32/ntdll rewrite (so this
is the next best tning to get the application working)

--
  v3: ntdll: Implement NtdllDefWindowProc_A().

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

[PATCH v3 1/1] ntdll: Implement NtdllDefWindowProc_A().

[Aida Jonikienė]

From: Aida Jonikienė

The Crinkler linker redirects the DefWindowProcA calls to the
NtdllDefWindowProc_A function (which doesn't exist on Wine)
and so it picks the first export in ntdll (which is A_SHAFinal right now)
which obviously causes a segfault (this is problematic for the application
listed in the bug report).

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53321
---
 dlls/ntdll/misc.c     | 27 +++++++++++++++++++++++++++
 dlls/ntdll/ntdll.spec |  1 +
 2 files changed, 28 insertions(+)

diff --git a/dlls/ntdll/misc.c b/dlls/ntdll/misc.c
index ab01c77531c..5318d445fbd 100644
--- a/dlls/ntdll/misc.c
+++ b/dlls/ntdll/misc.c
@@ -501,3 +501,30 @@ ULONG WINAPIV EtwTraceMessage( TRACEHANDLE handle, ULONG flags, LPGUID guid, /*U
     va_end( valist );
     return ret;
 }
+
+/******************************************************************************
+ *                  NtdllDefWindowProc_A (NTDLL.@)
+ */
+LRESULT WINAPI NtdllDefWindowProc_A( HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam )
+{
+    LPARAM (WINAPI *pDefWindowProcA)(HWND,UINT,WPARAM,LPARAM); /* DefWindowProcA */
+
+    const UNICODE_STRING name = RTL_CONSTANT_STRING( L"user32.dll" );
+    NTSTATUS status;
+    HMODULE module;
+
+    if (( status = LdrGetDllHandle( NULL, 0, &name, &module )))
+    {
+        ERR("Failed to get user32.dll handle, status %ld\n", status);
+        return 1; /* FIXME: Is this a good way to indicate failure? */
+    }
+
+    pDefWindowProcA = RtlFindExportedRoutineByName( module, "DefWindowProcA" );
+    if (!pDefWindowProcA)
+    {
+        ERR("Failed to load the function\n");
+        return 1;
+    }
+
+    return pDefWindowProcA( hwnd, msg, wParam, lParam );
+}
diff --git a/dlls/ntdll/ntdll.spec b/dlls/ntdll/ntdll.spec
index 014c221e59f..79bcfbe2e74 100644
--- a/dlls/ntdll/ntdll.spec
+++ b/dlls/ntdll/ntdll.spec
@@ -455,6 +455,7 @@
 # @ stub NtWriteRequestData
 @ stdcall -syscall NtWriteVirtualMemory(long ptr ptr long ptr)
 @ stdcall -syscall NtYieldExecution()
+@ stdcall NtdllDefWindowProc_A(long long long long)
 @ stub PfxFindPrefix
 @ stub PfxInitialize
 @ stub PfxInsertPrefix

[PATCH v3 1/1] ntdll: Implement NtdllDefWindowProc_A().

[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=144273

Your paranoid android.

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

ntoskrnl.exe:
ntoskrnl.c:1656: Test failed: got hardware IDs "ROOT\WINETEST\0\x00etest#0#{deadbeef-29ef-4538-a5fd-b69573a362c0}\x00\x00\x00\x00$\x00\x00\x00\x00\x00D\xf2!\x00\x00\x00\x00\x00@\xf2!\x00\x00\x00\x00\x00\xa6\xaa\xc6\xff\xffo\x00\x00p\x10A\x00\x00\x00\x00\x00\x04\x00\x00\x00\x02\x00\x00\x00\xd0\xcb\xb6\x00\x00\x00\x00\x00\x00\xf0!\x00#\x00"...
ntoskrnl.c:1671: Test failed: got compatible IDs "ROOT\WINETEST\0\x00etest#0#{deadbeef-29ef-4538-a5fd-b69573a362c0}\x00\x00\x00\x00$\x00\x00\x00\x00\x00D\xf2!\x00\x00\x00\x00\x00@\xf2!\x00\x00\x00\x00\x00\xa6\xaa\xc6\xff\xffo\x00\x00p\x10A\x00\x00\x00\x00\x00\x04\x00\x00\x00\x02\x00\x00\x00\xd0\xcb\xb6\x00\x00\x00\x00\x00\x00\xf0!\x00#\x00"...

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about dlls/ntdll/misc.c:
>  va_end( valist );
>  return ret;
>
> }
> -
>
> +/******************************************************************************
> -
> -
>              NtdllDefWindowProc_A (NTDLL.@)
> - */
>
> +LRESULT WINAPI NtdllDefWindowProc_A( HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam )
> +{
> - LPARAM (WINAPI *pDefWindowProcA)(HWND,UINT,WPARAM,LPARAM); /* DefWindowProcA */
> -

Extraneous blank line.

```suggestion:-0+0
```

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about dlls/ntdll/misc.c:
>  va_end( valist );
>  return ret;
>
> }
> -
>
> +/******************************************************************************
> -
> -
>              NtdllDefWindowProc_A (NTDLL.@)
> - */
>
> +LRESULT WINAPI NtdllDefWindowProc_A( HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam )
> +{
> - LPARAM (WINAPI *pDefWindowProcA)(HWND,UINT,WPARAM,LPARAM); /* DefWindowProcA */
> -
> - const UNICODE_STRING name = RTL_CONSTANT_STRING( L"user32.dll" );
> - NTSTATUS status;
> - HMODULE module;
> -
> - if (( status = LdrGetDllHandle( NULL, 0, &name, &module )))

```suggestion:-0+0
    if ((status = LdrGetDllHandle( NULL, 0, &name, &module )))
```

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about dlls/ntdll/misc.c:
> }
> -
>
> +/******************************************************************************
> -
> -
>              NtdllDefWindowProc_A (NTDLL.@)
> - */
>
> +LRESULT WINAPI NtdllDefWindowProc_A( HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam )
> +{
> - LPARAM (WINAPI *pDefWindowProcA)(HWND,UINT,WPARAM,LPARAM); /* DefWindowProcA */
> -
> - const UNICODE_STRING name = RTL_CONSTANT_STRING( L"user32.dll" );
> - NTSTATUS status;
> - HMODULE module;
> -
> - if (( status = LdrGetDllHandle( NULL, 0, &name, &module )))
> - {
> -
>    ERR("Failed to get user32.dll handle, status %ld\n", status);

Hex should be more useful for NTSTATUS.

```suggestion:-0+0
        ERR("Failed to get user32.dll handle, status %08lx\n", status);
```

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about dlls/ntdll/misc.c:
> -
>
> +/******************************************************************************
> -
> -
>              NtdllDefWindowProc_A (NTDLL.@)
> - */
>
> +LRESULT WINAPI NtdllDefWindowProc_A( HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam )
> +{
> - LPARAM (WINAPI *pDefWindowProcA)(HWND,UINT,WPARAM,LPARAM); /* DefWindowProcA */
> -
> - const UNICODE_STRING name = RTL_CONSTANT_STRING( L"user32.dll" );
> - NTSTATUS status;
> - HMODULE module;
> -
> - if (( status = LdrGetDllHandle( NULL, 0, &name, &module )))
> - {
> -
>    ERR("Failed to get user32.dll handle, status %ld\n", status);
> -
>    return 1; /* FIXME: Is this a good way to indicate failure? */

DefWindowProcA seems to return 0 on failure.

```suggestion:-0+0
        return 0; /* FIXME: Is this a good way to indicate failure? */
```

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about dlls/ntdll/misc.c:
> -
> - const UNICODE_STRING name = RTL_CONSTANT_STRING( L"user32.dll" );
> - NTSTATUS status;
> - HMODULE module;
> -
> - if (( status = LdrGetDllHandle( NULL, 0, &name, &module )))
> - {
> -
>    ERR("Failed to get user32.dll handle, status %ld\n", status);
> -
>    return 1; /* FIXME: Is this a good way to indicate failure? */
> - }
> -
> - pDefWindowProcA = RtlFindExportedRoutineByName( module, "DefWindowProcA" );
> - if (!pDefWindowProcA)
> - {
> -
>    ERR("Failed to load the function\n");
> -
>    return 1;

Ditto. Please duplicate the FIXME here as well.

```suggestion:-0+0
        return 0; /* FIXME: Is this a good way to indicate failure? */
```

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about dlls/ntdll/misc.c:
> - LPARAM (WINAPI *pDefWindowProcA)(HWND,UINT,WPARAM,LPARAM); /* DefWindowProcA */
> -
> - const UNICODE_STRING name = RTL_CONSTANT_STRING( L"user32.dll" );
> - NTSTATUS status;
> - HMODULE module;
> -
> - if (( status = LdrGetDllHandle( NULL, 0, &name, &module )))
> - {
> -
>    ERR("Failed to get user32.dll handle, status %ld\n", status);
> -
>    return 1; /* FIXME: Is this a good way to indicate failure? */
> - }
> -
> - pDefWindowProcA = RtlFindExportedRoutineByName( module, "DefWindowProcA" );
> - if (!pDefWindowProcA)
> - {
> -
>    ERR("Failed to load the function\n");

I'm afraid "load" isn't quite accurate here.

```suggestion:-0+0
        ERR("Cannot find the function\n");
```

[Jinoh Kang (＠iamahuman)]

Thanks! The MR seems mostly good. A few nits and it seems good to go.

[Nikolay Sivov (＠nsivov)]

Is there any evidence this is how it should work?

[Jinoh Kang (＠iamahuman)]

On Thu Mar 21 16:00:14 2024 +0000, Nikolay Sivov wrote:
> Is there any evidence this is how it should work?

No. On Windows it works completely differently: https://stackoverflow.com/questions/30871183/getprocaddressgetmodulehandleuser32-dll-defwindowprocw-returns-address.

IMHO this is a good enough approximation until we can refactor user32 to move DefWindowProc to ntdll.