[PATCH v5 0/1] 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. -- v5: ntdll: Also restore rbp before calling user mode callback. 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 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 6ec662b9850..3bed0c3fada 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" -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8466
On Sat Jul 5 01:15:57 2025 +0000, Yuxuan Shui wrote:
then the test can randomly fail. is there a way to test this that's no probabilistic? If you set that to 0xdeadbeef before going through callback dispatcher, then modify to something else before unwind to initial frame where they are supposed to be 0xdeadbeef if restoed, such chance is very low. The test I see in snippet doesn't look right for other reasons. You can't be modifying non-volatile registers in the middle of C code, that unfortunately has to be binary function which will modify those, call CreateWindow and restore the registers back before returning. If the same compiled code is running on Windows that of course hints that it might indeed not be restored on Windows but it is quite hard to follow guessing which exact way such code can be broken and which it can't.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108872
// unwinding out of a syscall seems to break some internal state?`
More likely those registers (which hold the code's variables) being voluntary altered break something (and thus hard to judge if the test results are correct in general or this breakage triggers some unrelated difference). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108873
You can't be modifying non-volatile registers in the middle of C code
You just need to put them in the [output list](https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#OutputOperands) and the compiler knows you are modifying them, so that should be fine. (albeit i mixed up output and input operands). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108874
On Sat Jul 5 01:35:22 2025 +0000, Yuxuan Shui wrote:
You can't be modifying non-volatile registers in the middle of C code You just need to put them in the [output list](https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#OutputOperands) and the compiler knows you are modifying them, so that should be fine. (albeit i mixed up output and input operands). but what is going to prevent compiler from using those immdeiately after you modified them? The only way to interpret such test is to compile, run, and carefully study its disassembly.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108875
On Sat Jul 5 01:39:17 2025 +0000, Paul Gofman wrote:
but what is going to prevent compiler from using those immdeiately after you modified them? The only way to interpret such test is to compile, run, and carefully study its disassembly. And breakage on function exit clearly indicates problems around that (while maybe exactly that part can be fixed by properly indicating registers as output). Also, even for temporary ad-hoc testing, why not use explicit register names as in %r12 and mark those in output as clobbered also by name? I don't know if maybe everyone knows what are those 'j', 'k' correspond at once, but I find it difficult to even find anywhere what those are :)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108876
compiler from using those immdeiately after you modified them?
that's fair. this is meant to be a quick test, and many registers did retain 0xdeadbeef.
And breakage on function exit.
that's an old comment i forgot to delete. that happened when i was using RtlUnwindEx and was actually unwinding out of the syscall. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108877
On Sat Jul 5 01:54:30 2025 +0000, Yuxuan Shui wrote:
compiler from using those immdeiately after you modified them? that's fair. this is meant to be a quick test, and many registers did retain 0xdeadbeef. And breakage on function exit. that's an old comment i forgot to delete. that happened when i was using RtlUnwindEx and was actually unwinding out of the syscall. Actually, [looks like](https://testbot.winehq.org/JobDetails.pl?Key=159099) all non-volatile registers are actually all _zeroed_ before calling user callback.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108878
On Sat Jul 5 01:55:26 2025 +0000, Yuxuan Shui wrote:
Actually, [looks like](https://testbot.winehq.org/JobDetails.pl?Key=159099) all non-volatile registers are actually all _zeroed_ before calling user callback. But your test shows (Win11):
rip=00007ff750e81a12 rsp=00000010617ff900 rbp=00000010617ffa70 eflags=00000202
rax=00007ffd89112a00 rbx=0000000000000000 rcx=00000010617ff0f0 rdx=0000000000000082
rsi=0000000000000000 rdi=0000000000000000 r8=0000000000000000 r9=0000000000000000
r10=cf18061710d6ea70 r11=0000000000000246 r12=0000000000000000 r13=0000000000000000
r14=0000000000000000 r15=0000000000000000 mxcsr=00001f80
rip=00007ff750e81a25 rsp=00000010617ff900 rbp=00000010617ffa70 eflags=00000246
rax=00007ffd89112a00 rbx=00000000deadbeaf rcx=00007ff750e87040 rdx=0000000000000000
rsi=00000000deadbeaf rdi=00000000deadbeaf r8=00000010617ff8c8 r9=0000000000000000
r10=0000000000000000 r11=0000000000000246 r12=00000000deadbeaf r13=00000000deadbeaf
r14=00000000deadbeaf r15=0000000000000000 mxcsr=00001f80
I see many non-volatile registers actually restored to 0xdeadbeef. For others, I am not immediately fully sure if they were actually deadbeef'ed ("i", "j", "k" thing is hard to follow), or werent' altered after before getting to the unwind target place. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108879
On Sat Jul 5 01:59:45 2025 +0000, Paul Gofman wrote:
But your test shows (Win11): ``` rip=00007ff750e81a12 rsp=00000010617ff900 rbp=00000010617ffa70 eflags=00000202 rax=00007ffd89112a00 rbx=0000000000000000 rcx=00000010617ff0f0 rdx=0000000000000082 rsi=0000000000000000 rdi=0000000000000000 r8=0000000000000000 r9=0000000000000000 r10=cf18061710d6ea70 r11=0000000000000246 r12=0000000000000000 r13=0000000000000000 r14=0000000000000000 r15=0000000000000000 mxcsr=00001f80 rip=00007ff750e81a25 rsp=00000010617ff900 rbp=00000010617ffa70 eflags=00000246 rax=00007ffd89112a00 rbx=00000000deadbeaf rcx=00007ff750e87040 rdx=0000000000000000 rsi=00000000deadbeaf rdi=00000000deadbeaf r8=00000010617ff8c8 r9=0000000000000000 r10=0000000000000000 r11=0000000000000246 r12=00000000deadbeaf r13=00000000deadbeaf r14=00000000deadbeaf r15=0000000000000000 mxcsr=00001f80 ``` I see many non-volatile registers actually restored to 0xdeadbeef. For others, I am not immediately fully sure if they were actually deadbeef'ed ("i", "j", "k" thing is hard to follow), or werent' altered after before getting to the unwind target place. Also, you'd need to "spoil" the current register values before doing unwind, or it may err on this side if those registers are just preseved as deadbeef throughout the call chain.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108880
I see many non-volatile registers actually restored to 0xdeadbeef.
er, sorry, the print out might be confusing. the upper half are registers returned by RtlVirtualUnwind, and I don't see any deadbeefs there? the lower half and registers before calling destroywindow. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108894
On Sat Jul 5 08:03:26 2025 +0000, Yuxuan Shui wrote:
I see many non-volatile registers actually restored to 0xdeadbeef. er, sorry, the print out might be confusing. the upper half are registers returned by RtlVirtualUnwind, and I don't see any deadbeefs there? the lower half and registers before calling destroywindow. I updated the test slightly:
```diff diff --git c/unwind test.c w/unwind test.c index 9fc6c12..0c2eced 100644 --- c/unwind test.c +++ w/unwind test.c @@ -136,13 +136,12 @@ int WINAPI mainCRTStartup(HINSTANCE hinstance, HINSTANCE hPrevInstance, LPSTR lp "mov $0xdeadbeaf, %10\n\t" "mov $0xdeadbeaf, %11\n\t" "mov $0xdeadbeaf, %12\n\t" - : : "r"(a), "r"(b), "r"(c), "r"(d), "r"(e), "r"(f), "r"(g), "r"(h), "r"(i), "r"(j), "r"(k), - "r"(l), "r"(m): ); - RtlCaptureContext(&prev_context); + : "=r"(a), "=r"(b), "=r"(c), "=r"(d), "=r"(e), "=r"(f), "=r"(g), "=r"(h), "=r"(i), "=r"(j), "=r"(k), + "=r"(l), "=r"(m): : ); destroy_window_launch_pad(hwnd, __builtin_frame_address(0)); + RtlCaptureContext(&prev_context); TRACE_CONTEXT(&prev_context); - // unwinding out of a syscall seems to break some internal state? TerminateProcess(GetCurrentProcess(), 0); __builtin_unreachable(); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8466#note_108895
participants (3)
-
Paul Gofman (＠gofman) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)