[PATCH v5 0/3] MR3855: Fixes for ntlea
Make functions ntlea needs to patch hotpatchable. And add a asm wrapper for `GetWindowLongA` to workaround an assumption ntlea made. -- v5: user32: add hotpatchable wrapper for GetWindowLongA https://gitlab.winehq.org/wine/wine/-/merge_requests/3855
From: Yuxuan Shui <yshui(a)codeweavers.com> Needed for ntlea. --- dlls/gdi32/objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/gdi32/objects.c b/dlls/gdi32/objects.c index 42bf6fcf111..070ce9c3885 100644 --- a/dlls/gdi32/objects.c +++ b/dlls/gdi32/objects.c @@ -418,7 +418,7 @@ HGDIOBJ WINAPI GetCurrentObject( HDC hdc, UINT type ) /*********************************************************************** * GetStockObject (GDI32.@) */ -HGDIOBJ WINAPI GetStockObject( INT obj ) +HGDIOBJ WINAPI DECLSPEC_HOTPATCH GetStockObject( INT obj ) { if (obj < 0 || obj > STOCK_LAST + 1 || obj == 9) return 0; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3855
From: Yuxuan Shui <yshui(a)codeweavers.com> Needed for ntlea. --- dlls/user32/winproc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/user32/winproc.c b/dlls/user32/winproc.c index 5ab85eb81cd..8804199aed1 100644 --- a/dlls/user32/winproc.c +++ b/dlls/user32/winproc.c @@ -860,7 +860,7 @@ BOOL WINAPI User32CallSendAsyncCallback( const struct send_async_params *params, * * ECMA-234, Win32 */ -LRESULT WINAPI CallWindowProcA( WNDPROC func, HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam ) +LRESULT WINAPI DECLSPEC_HOTPATCH CallWindowProcA( WNDPROC func, HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam ) { struct win_proc_params params; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3855
From: Yuxuan Shui <yshui(a)codeweavers.com> ntlea for some reason expects GetWindowLongA to start with a "push $-2", and will try to skip over this instruction. If we don't anticipate this, it will ended up either skipping over critical instructions, or on a desync address. Either way, it would be bad. --- dlls/user32/win.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/dlls/user32/win.c b/dlls/user32/win.c index 28cf40441d9..6fa76faf81c 100644 --- a/dlls/user32/win.c +++ b/dlls/user32/win.c @@ -884,11 +884,33 @@ WORD WINAPI GetWindowWord( HWND hwnd, INT offset ) return NtUserGetWindowWord( hwnd, offset ); } - /********************************************************************** * GetWindowLongA (USER32.@) */ -LONG WINAPI GetWindowLongA( HWND hwnd, INT offset ) + +#ifdef __i386__ + +/* This wrapper is here to workaround a ntlea quirk. First of all, ntlea + * checks whether GetWindowLongA starts with the Win32 hotpatchable prologue, + * if it can find that, it will use a hooking strategy more difficult for us + * to deal with. Secondly, it assumes what follows the prologue is a `pushl $-2`, + * and will try to skip over this instruction when calling `GetWindowLongA`, + * (i.e. it tries to jump to `GetWindowLongA + 7`, 5 bytes for the prologue, 2 + * bytes for the `pushl`.). We have to anticipate that and make sure the result + * of doing this won't be a messed up stack, or a desynced PC. + */ +__ASM_STDCALL_FUNC( GetWindowLongA, 8, + "movl.s %edi, %edi\n" + "pushl %ebp\n" + "movl.s %esp, %ebp\n" + "pushl $-2\n" + "addl $4, %esp\n" + "popl %ebp\n" + "jmp " __ASM_STDCALL("get_window_longA", 8) ) +LONG WINAPI get_window_longA( HWND hwnd, INT offset ) +#else +LONG WINAPI DECLSPEC_HOTPATCH GetWindowLongA( HWND hwnd, INT offset ) +#endif { switch (offset) { @@ -910,7 +932,6 @@ LONG WINAPI GetWindowLongA( HWND hwnd, INT offset ) } } - /********************************************************************** * GetWindowLongW (USER32.@) */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3855
update: fix style -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3855#note_45770
On Mon Sep 18 18:05:38 2023 +0000, Yuxuan Shui wrote:
changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/3855/diffs?diff_id=70002&start_sha=8a4ff678e600331faed0417874cfdc2c3f6ee59c#72b0ca1e62b280db6ccf7d10b3be07261b9c9b7f_912_912) I mean this part:
```c LONG WINAPI get_window_longA( HWND hwnd, INT offset ) #else LONG WINAPI DECLSPEC_HOTPATCH GetWindowLongA(HWND hwnd, INT offset) ``` It's not a big deal, but it would look better to either have a space before `HWND` and after `offset` in both places, or no space before `HWND` and after `offset` in both places. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3855#note_45771
On Mon Sep 18 18:07:32 2023 +0000, Alex Henrie wrote:
I mean this part: ```c LONG WINAPI get_window_longA( HWND hwnd, INT offset ) #else LONG WINAPI DECLSPEC_HOTPATCH GetWindowLongA(HWND hwnd, INT offset) ``` It's not a big deal, but it would look better to either have a space before `HWND` and after `offset` in both places, or no space before `HWND` and after `offset` in both places. thanks for clarifying, fixed!
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3855#note_45772
Stefan Dösinger (@stefan) commented about dlls/user32/win.c:
+ +#ifdef __i386__ + +/* This wrapper is here to workaround a ntlea quirk. First of all, ntlea + * checks whether GetWindowLongA starts with the Win32 hotpatchable prologue, + * if it can find that, it will use a hooking strategy more difficult for us + * to deal with. Secondly, it assumes what follows the prologue is a `pushl $-2`, + * and will try to skip over this instruction when calling `GetWindowLongA`, + * (i.e. it tries to jump to `GetWindowLongA + 7`, 5 bytes for the prologue, 2 + * bytes for the `pushl`.). We have to anticipate that and make sure the result + * of doing this won't be a messed up stack, or a desynced PC. + */ +__ASM_STDCALL_FUNC( GetWindowLongA, 8, + "movl.s %edi, %edi\n" + "pushl %ebp\n" + "movl.s %esp, %ebp\n" movl.s requires a minimum GNU binutils version. I think it's been 13 or so years since I added that feature, but e.g. mac binutils does not have it. I also don't know how e.g. clang handles it when building windows binaries. A configure check might be worth adding though.
In most cases this code will be compiled by mingw-w64 with gcc and gnu binutils, which should be fine even on mac. I think the two possible configurations that could run into problems here_ 1. clang in msvc mode or mingw-clang 2. A 32 bit build targeting MacOS mojave with --without-mingw The latter is pretty tricky to pull off, so not much of a concern. Can you test the first case? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3855#note_45776
On Mon Sep 18 18:45:42 2023 +0000, Stefan Dösinger wrote:
movl.s requires a minimum GNU binutils version. I think it's been 13 or so years since I added that feature, but e.g. mac binutils does not have it. I also don't know how e.g. clang handles it when building windows binaries. A configure check might be worth adding though. In most cases this code will be compiled by mingw-w64 with gcc and gnu binutils, which should be fine even on mac. I think the two possible configurations that could run into problems here_ 1. clang in msvc mode or mingw-clang 2. A 32 bit build targeting MacOS mojave with --without-mingw The latter is pretty tricky to pull off, so not much of a concern. Can you test the first case? Or more simply we could just do '.byte 0x8b, 0xff, 0x55, 0x8b, 0xec'. That's what winebuild does.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3855#note_45779
On Mon Sep 18 20:09:07 2023 +0000, Zebediah Figura wrote:
Or more simply we could just do '.byte 0x8b, 0xff, 0x55, 0x8b, 0xec'. That's what winebuild does. I will go with @zfigura's solution
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3855#note_45785
participants (5)
-
Alex Henrie (＠alexhenrie) -
Stefan Dösinger
-
Yuxuan Shui
-
Yuxuan Shui (＠yshui)
-
Zebediah Figura (＠zfigura)