[PATCH v6 0/2] MR8466: ntdll: Also restore rbp before calling user mode callback.
If wine dlls are built with frame pointers enabled, the frame pointer will be used during unwinding. If we don't restore frame pointer before calling the user mode callback, then later when the unwinder encounters the user mode callback frame, it will set the frame pointer to something unexpected (depends on what it was during `call_user_mode_callback`). Then for the subsequent frame it adjusts the stack pointer based on the frame pointer, thus derailing the unwinding process. -- v6: ntdll: Also restore rbp before calling user mode callback. user32/tests: Test unwinding through a user callback. https://gitlab.winehq.org/wine/wine/-/merge_requests/8466
From: Yuxuan Shui <yshui(a)codeweavers.com> 1. Check if we can use RtlVirtualUnwind to unwind into the parent frame of a user callback. Most importantly, we want to know if %rbp is recovered when doing so. To do that, we use the .seh_setframe directive in an assembly function, so its %rsp must be recovered from %rbp. If the value of %rbp is incorrect, this should derail the unwinding and can be detected. 2. We check whether non-volatile registers other than %rbp is also recovered. --- dlls/user32/tests/win.c | 112 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index f899008f394..b9866ee294d 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -32,6 +32,7 @@ #include "winuser.h" #include "winreg.h" +#include "wine/exception.h" #include "wine/test.h" #ifndef WM_SYSTIMER @@ -43,6 +44,7 @@ void dump_region(HRGN hrgn); +static BOOL (WINAPI *pDestroyWindow)(HWND); static BOOL (WINAPI *pGetWindowInfo)(HWND,WINDOWINFO*); static UINT (WINAPI *pGetWindowModuleFileNameA)(HWND,LPSTR,UINT); static BOOL (WINAPI *pGetLayeredWindowAttributes)(HWND,COLORREF*,BYTE*,DWORD*); @@ -13750,12 +13752,119 @@ static void test_startupinfo_showwindow( char **argv ) } } +#if defined(_WIN64) && defined(__WINE_PE_BUILD) +static void *unwind_target = NULL; +static void *target_frame; + +static LRESULT unwinding_wnd_proc(HWND w, UINT msg, WPARAM p2, LPARAM p3) +{ + CONTEXT context; + int frames; + UNWIND_HISTORY_TABLE table; + RUNTIME_FUNCTION *func; + ULONG_PTR frame, base; + void *data; + BOOL found = FALSE; + + RtlCaptureContext(&context); + + switch (msg) + { + case WM_NCDESTROY: + for (frames = 0; frames < 16; frames++) + { + func = RtlLookupFunctionEntry(context.Rip, &base, &table); + if (RtlVirtualUnwind(UNW_FLAG_NHANDLER, base, context.Rip, func, &context, &data, &frame, NULL)) + break; + if (!context.Rip) break; + if (!frame) break; + if (context.Rip == (DWORD64)unwind_target || frame == (DWORD64)target_frame) + { + found = TRUE; + + /* check that non-volatile registers are zeroed before entering user callback. */ + ok(!context.Rbx, "unexpected register value, %%rbx = %#I64x\n", context.Rbx); + ok(!context.R12, "unexpected register value, %%r12 = %#I64x\n", context.R12); + ok(!context.R13, "unexpected register value, %%r13 = %#I64x\n", context.R13); + ok(!context.R14, "unexpected register value, %%r14 = %#I64x\n", context.R14); + ok(!context.R15, "unexpected register value, %%r15 = %#I64x\n", context.R15); + break; + } + } + todo_wine ok(found, "couldn't find target frame in parent frames\n"); + break; + default: break; + } + return DefWindowProcA(w, msg, p2, p3); +} + +/* add one more level of stack frame so problems during unwinding are more likely to show up. */ +asm("\t.globl destroy_window_trampoline\n\t" + ".def destroy_window_trampoline\n\t" + ".scl 2\n\t" + ".type 32\n\t" + ".endef\n\t" + ".seh_proc destroy_window_trampoline\n" + "destroy_window_trampoline:\n\t" + "push %rbp\n\t" + ".seh_pushreg %rbp\n\t" + "subq $40, %rsp\n\t" /* allocate some random amount */ + ".seh_stackalloc 40\n\t" + "leaq 32(%rsp), %rbp\n\t" + ".seh_setframe %rbp, 32\n\t" /* force the unwinder to unwind %rsp from %rbp */ + ".seh_endprologue\n\t" + "call *pDestroyWindow(%rip)\n\t" + "nop\n\t" /* some padding before epilogue */ + "addq $40, %rsp\n\t" + "pop %rbp\n\t" + "ret\n\t" + ".seh_endproc"); + +static void test_user_callback_registers(void) +{ + WNDCLASSA cls = {0}; + HWND hwnd; + ATOM atom; + + cls.style = CS_HREDRAW | CS_VREDRAW; + cls.lpfnWndProc = unwinding_wnd_proc; + cls.hInstance = GetModuleHandleA(0); + cls.lpszClassName = "test_user_callback_registers_class"; + + atom = RegisterClassA(&cls); + ok(!!atom, "RegisterClassA failed, error %#lx.\n", GetLastError()); + + hwnd = CreateWindowExA(WS_EX_TOPMOST, cls.lpszClassName, "", WS_POPUP | WS_VISIBLE, 100, 100, + 100, 100, NULL, NULL, 0, NULL); + ok(!!hwnd, "CreateWindowA failed, error %#lx.\n", GetLastError()); + + asm("mov %[poison], %%rbx\n\t" + "mov %[poison], %%r12\n\t" + "mov %[poison], %%r13\n\t" + "mov %[poison], %%r14\n\t" + "mov %[poison], %%r15\n\t" + "mov %%rsp, %0\n\t" + "lea 1f(%%rip), %%rdx\n\t" + "mov %%rdx, %1\n\t" + "mov %[hwnd], %%rcx\n\t" + "call destroy_window_trampoline\n\t" + "1:nop\n\t" + : "=m"(target_frame), "=m"(unwind_target) + : [poison] "i"(0xdeadbeafULL), [hwnd] "rm"(hwnd), "m"(pDestroyWindow) + : "rax", "rcx", "rdx", "rsi", "rdi", "r8", "r9", "r10", "r11", "rbx", "r12", "r13", "r14", "r15", "memory"); + + UnregisterClassA(cls.lpszClassName, GetModuleHandleA(0)); +} + +#endif + START_TEST(win) { char **argv; int argc = winetest_get_mainargs( &argv ); HMODULE user32 = GetModuleHandleA( "user32.dll" ); HMODULE gdi32 = GetModuleHandleA("gdi32.dll"); + pDestroyWindow = (void *)GetProcAddress( user32, "DestroyWindow" ); pGetWindowInfo = (void *)GetProcAddress( user32, "GetWindowInfo" ); pGetWindowModuleFileNameA = (void *)GetProcAddress( user32, "GetWindowModuleFileNameA" ); pGetLayeredWindowAttributes = (void *)GetProcAddress( user32, "GetLayeredWindowAttributes" ); @@ -13943,6 +14052,9 @@ START_TEST(win) test_ReleaseCapture(); test_SetProcessLaunchForegroundPolicy(); test_startupinfo_showwindow(argv); +#if defined(_WIN64) && defined(__WINE_PE_BUILD) + test_user_callback_registers(); +#endif /* add the tests above this line */ if (hhook) UnhookWindowsHookEx(hhook); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8466
From: Yuxuan Shui <yshui(a)codeweavers.com> If wine dlls are built with frame pointers enabled, the frame pointer will be used during unwinding. If we don't restore frame pointer before calling the user mode callback, then later when the unwinder encounters the user mode callback frame, it will set the frame pointer to something unexpected (depends on what it was during `call_user_mode_callback`). Then for the subsequent frame it adjusts the stack pointer based on the frame pointer, thus derailing the unwinding process. --- dlls/ntdll/unix/signal_x86_64.c | 12 +++++++++--- dlls/user32/tests/win.c | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 6ec662b9850..4694b949071 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -1642,8 +1642,8 @@ __ASM_GLOBAL_FUNC( call_user_mode_callback, "andq $~63,%rsp\n\t" "leaq 0x10(%rbp),%rax\n\t" "movq %rax,0xa8(%rsp)\n\t" /* frame->syscall_cfa */ - "movq 0x378(%r13),%r10\n\t" /* thread_data->syscall_frame */ - "movq %r10,0xa0(%rsp)\n\t" /* frame->prev_frame */ + "movq 0x378(%r13),%r14\n\t" /* thread_data->syscall_frame */ + "movq %r14,0xa0(%rsp)\n\t" /* frame->prev_frame */ "movq %rsp,0x378(%r13)\n\t" /* thread_data->syscall_frame */ "testl $1,0x380(%r13)\n\t" /* thread_data->syscall_trace */ "jz 1f\n\t" @@ -1658,6 +1658,7 @@ __ASM_GLOBAL_FUNC( call_user_mode_callback, "movq %r15,%rcx\n" /* func */ /* switch to user stack */ "1:\tmovq %rdi,%rsp\n\t" /* user_rsp */ + "movq 0x98(%r14),%rbp\n\t" /* user_frame->Rbp */ #ifdef __linux__ "movw 0x338(%r13),%ax\n" /* amd64_thread_data()->fs */ "testw %ax,%ax\n\t" @@ -1677,7 +1678,12 @@ __ASM_GLOBAL_FUNC( call_user_mode_callback, "test %r10,%r10\n\t" "jz 1f\n\t" "xchgq %rcx,%r10\n\t" - "1\t:jmpq *%rcx" ) /* func */ + "1:\txor %rbx,%rbx\n\t" /* zero non-volatile registers. */ + "xor %r12,%r12\n\t" + "xor %r13,%r13\n\t" + "xor %r14,%r14\n\t" + "xor %r15,%r15\n\t" + "jmpq *%rcx" ) /* func */ /*********************************************************************** diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index b9866ee294d..8cdd7f93ad6 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -13791,7 +13791,7 @@ static LRESULT unwinding_wnd_proc(HWND w, UINT msg, WPARAM p2, LPARAM p3) break; } } - todo_wine ok(found, "couldn't find target frame in parent frames\n"); + ok(found, "couldn't find target frame in parent frames\n"); break; default: break; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8466
changes: - non-volatile registers are now zeroed based on test findings. - add test case (is user32:win the right place for this?) ping @gofman -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108990
Yeah thanks I will look in details a bit later, there are some things to edit (like "asm", probably a bit of portability issues, separate #ifdefs for which we have common places) which I am going to look through in details. Quick question: why not checking rbp value explicitly along with the other registers and instead making that checked indirectly through using rbp based frame? IMO instead not using rbp frame and checking explicitly would make more sense. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108992
On Mon Jul 7 16:22:14 2025 +0000, Paul Gofman wrote:
Yeah thanks I will look in details a bit later, there are some things to edit (like "asm", probably a bit of portability issues, separate #ifdefs for which we have common places) which I am going to look through in details. Quick question: why not checking rbp value explicitly along with the other registers and instead making that checked indirectly through using rbp based frame? IMO instead not using rbp frame and checking explicitly would make more sense. you mean "%rbp = 0xdeadbeef" then check for that?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108995
On Mon Jul 7 16:22:14 2025 +0000, Yuxuan Shui wrote:
you mean "%rbp = 0xdeadbeef" then check for that? Yes. But there are more things to do, if you look at different tests. I was going to look in details how that can be done in line with other tests before suggesting any changes. This is just a question at this point.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108996
On Mon Jul 7 16:35:17 2025 +0000, Paul Gofman wrote:
Yes. But there are more things to do, if you look at different tests. I was going to look in details how that can be done in line with other tests before suggesting any changes. This is just a question at this point. i was testing it this way because it's closer to a real use case, and this is how i discovered this bug.
but yeah, i can do it the way you suggested. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108997
participants (3)
-
Paul Gofman (＠gofman) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)