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.
-- v9: ntdll: Also restore rbp before calling user mode callback. user32/tests: Test unwinding through a user callback.
From: Yuxuan Shui yshui@codeweavers.com
Check the state of non-volatile registers before a user callback is entered. --- dlls/user32/tests/win.c | 116 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+)
diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index f899008f394..85a1fbc86d8 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -32,6 +32,8 @@ #include "winuser.h" #include "winreg.h"
+#include "wine/asm.h" +#include "wine/exception.h" #include "wine/test.h"
#ifndef WM_SYSTIMER @@ -43,6 +45,7 @@
void dump_region(HRGN hrgn);
+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 +13753,122 @@ static void test_startupinfo_showwindow( char **argv ) } }
+#if defined(_WIN64) && defined(__WINE_PE_BUILD) +void *unwind_target = NULL; +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); + ok(context.Rbp == 0xdeadbeaf, "unexpected register value, %%rbp = %#I64x\n", context.Rbp); + break; + } + } + ok(found, "couldn't find target frame in parent frames\n"); + break; + default: break; + } + return DefWindowProcA(w, msg, p2, p3); +} + +/* setup register context so later we can check what changed and what hasn't. */ +extern void destroy_window_trampoline(HWND w); +__ASM_GLOBAL_FUNC(destroy_window_trampoline, + "push %rbp\n\t" + __ASM_SEH(".seh_pushreg %rbp\n\t") + "push %r15\n\t" + __ASM_SEH(".seh_pushreg %r15\n\t") + "pushq %r14\n\t" + __ASM_SEH(".seh_pushreg %r14\n\t") + "pushq %r13\n\t" + __ASM_SEH(".seh_pushreg %r13\n\t") + "pushq %r12\n\t" + __ASM_SEH(".seh_pushreg %r12\n\t") + "pushq %rbx\n\t" + __ASM_SEH(".seh_pushreg %rbx\n\t") + "subq $40, %rsp\n\t" /* allocate some random amount */ + __ASM_SEH(".seh_stackalloc 40\n\t") + __ASM_SEH(".seh_endprologue\n\t") + "mov $0xdeadbeaf,%rbp\n\t" + "mov $0xdeadbeaf,%rbx\n\t" + "mov $0xdeadbeaf,%r12\n\t" + "mov $0xdeadbeaf,%r13\n\t" + "mov $0xdeadbeaf,%r14\n\t" + "mov $0xdeadbeaf,%r15\n\t" + "mov %rsp,target_frame(%rip)\n\t" + "lea 1f(%rip),%rdx\n\t" + "mov %rdx,unwind_target(%rip)\n\t" + "call *pDestroyWindow(%rip)\n\t" + "1:nop\n\t" /* some padding before epilogue */ + "addq $40, %rsp\n\t" + "pop %rbx\n\t" + "pop %r12\n\t" + "pop %r13\n\t" + "pop %r14\n\t" + "pop %r15\n\t" + "pop %rbp\n\t" + "ret"); + +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()); + + destroy_window_trampoline(hwnd); + 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 +14056,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);
From: Yuxuan Shui yshui@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 +++++++++--- 1 file changed, 9 insertions(+), 3 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 */
/***********************************************************************
Meanwhile, in this form, I personally find the test fully convincing that Windows doesn't properly unwind the rest of non-volatile regs and we need to just restore %rbp and be good (while zeroing out other regs is maybe not necessary in practice). Also, I tested separately with MSVC and /EHa (to do it quick and avoid any complications which can be affecting a more complicated test) that access violation in WM_CREATE does not get caught and app just aborts this way (so executing SEH handler through user mode callback doesn't work on Windows).
So maybe if Alexandre thinks adding a complicated test for such an unwind is not really necessary we can just go with the change restoring rbp and be good?
while zeroing out other regs is maybe not necessary in practice
i would guess this to probably done to avoid leaking value of kernel registers. it might not be necessary but why don't we follow what native does? that's what the test finds anyway.
executing SEH handler through user mode callback doesn't work on Windows
yeah i was thinking about this. unwinding through user callback doesn't work and raises `STATUS_FATAL_USER_CALLBACK_EXCEPTION`, we should probably do the same.
so i think we:
```diff - ".seh_handler " __ASM_NAME("user_callback_handler") ", @except\n\t" + ".seh_handler " __ASM_NAME("user_callback_handler") ", @except, @unwind\n\t" ```
in `KiUserCallbackDispatcher`. stop the unwind, call `NtCallbackReturn`, then on the kernel side raise `STATUS_FATAL_USER_CALLBACK_EXCEPTION`.
does that sound right?
On Mon Jul 7 18:24:19 2025 +0000, Yuxuan Shui wrote:
while zeroing out other regs is maybe not necessary in practice
i would guess this to probably done to avoid leaking value of kernel registers. it might not be necessary but why don't we follow what native does? that's what the test finds anyway.
executing SEH handler through user mode callback doesn't work on Windows
yeah i was thinking about this. unwinding through user callback doesn't work and raises `STATUS_FATAL_USER_CALLBACK_EXCEPTION`, we should probably do the same. so i think we:
- ".seh_handler " __ASM_NAME("user_callback_handler") ", @except\n\t" + ".seh_handler " __ASM_NAME("user_callback_handler") ", @except, @unwind\n\t"in `KiUserCallbackDispatcher`. stop the unwind, call `NtCallbackReturn`, then on the kernel side raise `STATUS_FATAL_USER_CALLBACK_EXCEPTION`. does that sound right?
also unwinding through user callback doesn't work anyway, it just throws away the kernel stack, which probably breaks things horribly.
i would guess this to probably done to avoid leaking value of kernel registers. it might not be necessary but why don't we follow what native does? that's what the test finds anyway.
Quite possibly, yes. The test still needs a work to do to be included upstream, like we don't write such functions with "asm" at all and specifically for tests we unfortunately have to write those in binary and not as asm for portability (also can't reference global symbols).
So I don't know, from one side, yes, it doesn't hurt to deliver exact values. On the other side, when nothing is known to depend on that *and it looks not much likely that anything will depend specifically on that and not, e. g., on some proper r11 value*, extra code to add that and following Windows internals too closely may be counterproductive. We don't have any reasons to hide Wine register values from the app, we don't have security boundaries like real kernel has.
unfortunately have to write those in binary and not as asm for portability
ok, do i also need to write the seh unwind info in binary?
On Mon Jul 7 19:04:42 2025 +0000, Yuxuan Shui wrote:
unfortunately have to write those in binary and not as asm for portability
ok, do i also need to write the seh unwind info in binary?
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)
-
Paul Gofman (＠gofman) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)