[PATCH v10 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. -- v10: 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> Check which non-volatile registers are set to their saved values before a user callback is entered. --- dlls/user32/tests/win.c | 129 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index f899008f394..358a529e245 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 @@ -13750,6 +13751,131 @@ static void test_startupinfo_showwindow( char **argv ) } } +#ifdef __x86_64__ +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 set properly before entering user callback. */ + ok(context.Rbx != 0xdeadbeef, "unexpected register value, %%rbx = %#I64x\n", context.Rbx); + ok(context.R12 != 0xdeadbeef, "unexpected register value, %%r12 = %#I64x\n", context.R12); + ok(context.R13 != 0xdeadbeef, "unexpected register value, %%r13 = %#I64x\n", context.R13); + ok(context.R14 != 0xdeadbeef, "unexpected register value, %%r14 = %#I64x\n", context.R14); + ok(context.R15 != 0xdeadbeef, "unexpected register value, %%r15 = %#I64x\n", context.R15); + todo_wine ok(context.Rbp == 0xdeadbeef, "unexpected register value, %%rbp = %#I64x\n", context.Rbp); + + /* native seems to zero these registers, unclear whether wine should do the same. */ + todo_wine { + 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; + } + } + ok(found, "couldn't find target frame in parent frames\n"); + break; + default: break; + } + return DefWindowProcA(w, msg, p2, p3); +} + +static void test_user_callback_registers(void) +{ + WNDCLASSA cls = {0}; + HWND hwnd; + ATOM atom; + UINT64 patch; + BYTE *code_mem = VirtualAlloc( NULL, 65536, MEM_RESERVE | MEM_COMMIT, PAGE_EXECUTE_READWRITE ); + void (*pdestroy_window_trampoline)(HWND) = (void (*)(HWND))code_mem; + + /* setup register context so later we can check what changed and what hasn't. */ + static const BYTE trampoline[] = + { + 0x55, /* 00: push %rbp */ + 0x41, 0x57, /* 01: push %r15 */ + 0x41, 0x56, /* 03: push %r14 */ + 0x41, 0x55, /* 05: push %r13 */ + 0x41, 0x54, /* 07: push %r12 */ + 0x53, /* 09: push %rbx */ + 0x48, 0x83, 0xec, 0x28, /* 0a: sub 0x28,%rsp */ + 0xbd, 0xef, 0xbe, 0xad, 0xde, /* 0e: mov $0xdeadbeef,%rbp */ + 0xbb, 0xef, 0xbe, 0xad, 0xde, /* 13: mov $0xdeadbeef,%rbx */ + 0x41, 0xbc, 0xef, 0xbe, 0xad, 0xde, /* 18: mov $0xdeadbeef,%r12 */ + 0x41, 0xbd, 0xef, 0xbe, 0xad, 0xde, /* 1e: mov $0xdeadbeef,%r13 */ + 0x41, 0xbe, 0xef, 0xbe, 0xad, 0xde, /* 24: mov $0xdeadbeef,%r14 */ + 0x41, 0xbf, 0xef, 0xbe, 0xad, 0xde, /* 2a: mov $0xdeadbeef,%r15 */ + 0x48, 0xb8, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, /* 30: mov ?,%rax # &target_frame */ + 0x48, 0x89, 0x20, /* 3a: mov %rsp,(%rax) */ + 0x48, 0xb8, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, /* 3d: mov ?,%rax # DestroyWindow */ + 0xff, 0xd0, /* 47: call *%rax */ + 0x90, /* 49: nop */ + 0x48, 0x83, 0xc4, 0x28, /* 4a: add $0x28,%rsp */ + 0x5b, /* 4e: pop %rbx */ + 0x41, 0x5c, /* 4f: pop %r12 */ + 0x41, 0x5d, /* 51: pop %r13 */ + 0x41, 0x5e, /* 53: pop %r14 */ + 0x41, 0x5f, /* 55: pop %r15 */ + 0x5d, /* 57: pop %rbp */ + 0xc3, /* 58: ret */ + }; + + memcpy(code_mem, trampoline, ARRAYSIZE(trampoline)); + + patch = (ULONG_PTR)&target_frame; + memcpy(code_mem + 0x30 + 2, &patch, 8); + patch = (ULONG_PTR)DestroyWindow; + memcpy(code_mem + 0x3d + 2, &patch, 8); + + unwind_target = code_mem + 0x49; + + 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()); + + pdestroy_window_trampoline(hwnd); + + VirtualFree(code_mem, 0, MEM_RELEASE); + UnregisterClassA(cls.lpszClassName, GetModuleHandleA(0)); +} + +#endif + START_TEST(win) { char **argv; @@ -13943,6 +14069,9 @@ START_TEST(win) test_ReleaseCapture(); test_SetProcessLaunchForegroundPolicy(); test_startupinfo_showwindow(argv); +#ifdef __x86_64__ + 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 | 7 ++++--- dlls/user32/tests/win.c | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 6ec662b9850..e75b7205b07 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,7 @@ __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:\tjmpq *%rcx" ) /* func */ /*********************************************************************** diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index 358a529e245..8885f1354eb 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -13787,7 +13787,7 @@ static LRESULT unwinding_wnd_proc(HWND w, UINT msg, WPARAM p2, LPARAM p3) ok(context.R13 != 0xdeadbeef, "unexpected register value, %%r13 = %#I64x\n", context.R13); ok(context.R14 != 0xdeadbeef, "unexpected register value, %%r14 = %#I64x\n", context.R14); ok(context.R15 != 0xdeadbeef, "unexpected register value, %%r15 = %#I64x\n", context.R15); - todo_wine ok(context.Rbp == 0xdeadbeef, "unexpected register value, %%rbp = %#I64x\n", context.Rbp); + ok(context.Rbp == 0xdeadbeef, "unexpected register value, %%rbp = %#I64x\n", context.Rbp); /* native seems to zero these registers, unclear whether wine should do the same. */ todo_wine { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8466
On Mon Jul 7 19:11:41 2025 +0000, Paul Gofman wrote:
I think you don't need .seh unwind in that function, since you are unwinding up to the IP right after the call and not upper (~~or it would be easier to write a function in a way that doesn't need unwind info~~ that would apparently be impossible since it modifies non-volatile regs but I hope you don't need that anyway) i've converted the asm function into binary, and removed the register zeroings for now.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_109319
Looks good, but the test should go in ntdll:exception. Also if we decide that nothing depends on the registers being cleared then we shouldn't test them, since they won't have a reliable value. OTOH since you already have the tests, we might as well clear them. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_109521
participants (3)
-
Alexandre Julliard (＠julliard) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)