[Bug 53682] New: wineboot shows "user_check_not_lock BUG: holding USER lock" on aarch64 since wine-7.14
https://bugs.winehq.org/show_bug.cgi?id=53682
Bug ID: 53682 Summary: wineboot shows "user_check_not_lock BUG: holding USER lock" on aarch64 since wine-7.14 Product: Wine Version: 7.14 Hardware: aarch64 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: ntdll Assignee: wine-bugs@winehq.org Reporter: PuetzKevinA@JohnDeere.com Distribution: ---
I recently updated our builds to past wine-7.0, and began encountering a wineboot failure on aarch64
0040:err:system:user_check_not_lock BUG: holding USER lock(64) s\system32\rundll32.exe: /home/yukondev/workspace/wine/dlls/win32u/sysparams.c:400: user_check_not_lock: Assertion `0' failed. 0040:err:seh:call_function_handlers invalid frame 21f410 (0000000000022000-0000000000120000) 0040:err:seh:NtRaiseException Exception frame is not in stack limits => unable to dispatch exception.
After some bisecting, I narrowed this down to being a regression between 7.13 and 7.14, and then further narrowed it to https://source.winehq.org/git/wine.git/commit/d50112b4b6e82782d3924a8dbd443f...
Somehow the call to KeUserModeCallback( NtUserLoadSysMenu,... ) at https://gitlab.winehq.org/wine/wine/-/blob/d50112b4b6e82782d3924a8dbd443f82f... does not properly return.
I think that commit is mostly a false lead though, the translation of syscall NtUserCreateWindowEx to a syscall just exposed a latent bug in KeUserModeCallback/__wine_syscall_dispatcher on aarch64, since having NtUserCreateWindowEx be a syscall means KeUserModeCallback can no longer use its "if we have no syscall frame, call the callback directly" simple path https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/ntdll/unix/signal_arm...
What seems to be actually at fault is that, inside of User32LoadSysMenu (the actual function invoked by KeUserModeCallback), is a call to NtUserCreateMenu - which is *also* a syscall. This call doesn't crash, and in fact all of User32LoadSysMenu runs to completion. But when it goes through __wine_syscall_dispatcher, the stack pointer is restored to be `$sp = arm64_thread_data()->syscall_frame` https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/ntdll/unix/signal_arm... - i.e. it points to &callback_frame, back on the stack of KeUserModeCallback. And this is *not* the bottom of the stack; there's compiler-generated prologue/epilogue to restore various non-volatile registers.
0x0000ffffaa0247b4 <KeUserModeCallback+0>: sub sp, sp, #0x450 0x0000ffffaa0247b8 <KeUserModeCallback+4>: stp x29, x30, [sp] 0x0000ffffaa0247bc <KeUserModeCallback+8>: mov x29, sp 0x0000ffffaa0247c0 <KeUserModeCallback+12>: stp x19, x20, [sp, #16] 0x0000ffffaa0247c4 <KeUserModeCallback+16>: stp x21, x22, [sp, #32] 0x0000ffffaa0247c8 <KeUserModeCallback+20>: str w0, [sp, #56] 0x0000ffffaa0247cc <KeUserModeCallback+24>: str x1, [sp, #48] 0x0000ffffaa0247d0 <KeUserModeCallback+28>: str w2, [sp, #60] 0x0000ffffaa0247d4 <KeUserModeCallback+32>: mov x21, x3 0x0000ffffaa0247d8 <KeUserModeCallback+36>: mov x20, x4 0x0000ffffaa0247dc <KeUserModeCallback+40>: add x19, sp, #0x40 // x19=&callback_frame
So any code that runs inside this syscall that uses the first 0x40 bytes of stack is trampling these variables in the frame of KeUserModeCallback. Eventually User32LoadSysMenu returns back into KiUserCallbackDispatcher, which passes it into NtCallbackReturn, which does a __wine_longjmp back into the KeUserModeCallback,and we exit from the __wine_setjmp for the second time (returning 0) and get to `return callback_frame.status` https://gitlab.winehq.org/wine/wine/-/blob/wine-7.17/dlls/ntdll/unix/signal_.... But then the epilogue starts peeling off the stack
=> 0x0000ffffaa024864 <+176>: ldr w0, [sp, #1088] 0x0000ffffaa024868 <+180>: ldp x19, x20, [sp, #16] 0x0000ffffaa02486c <+184>: ldp x21, x22, [sp, #32] 0x0000ffffaa024870 <+188>: ldp x29, x30, [sp] 0x0000ffffaa024874 <+192>: add sp, sp, #0x450 0x0000ffffaa024878 <+196>: ret
and the link register $x30 = (void *) 0xffffaa024984 <NtCallbackReturn+104>, rather than 0xffffa8f0ccdc <copy_sys_popup+44> as it was when it was pushed in the prologue.
It's overwritten several times along the way, but I don't think any of these call sites are at fault; they are just writing to what they think is their own stack frame, unaware that __wine_syscall_dispatcher adjusted $sp to too-high a value and they are overwriting space that belongs to KeUserModeCallback.
The specific places that overwrote this entry on the stack were #0 0x0000ffffa8f37a48 in NtUserCallOneParam (arg=0, code=2) at /home/yukondev/workspace/wine/dlls/win32u/sysparams.c:5357 #1 0x0000ffffaa022cf0 in __wine_syscall_dispatcher () from /opt/wine/bin/../lib/wine/aarch64-unix/ntdll.so which set it to 0xffffaa022cf0 <__wine_syscall_dispatcher+272>
#0 insert_menu_item (ret_pos=0x21f538, flags=1024, id=4294967295, handle=0x10042) at /home/yukondev/workspace/wine/dlls/win32u/menu.c:438 #1 NtUserThunkedMenuItemInfo (handle=0x10042, pos=4294967295, flags=1024, method=1, info=0x11f528 <opengl_func_names+680>, str=<optimized out>) at /home/yukondev/workspace/wine/dlls/win32u/menu.c:1297 #2 0x0000ffffaa022cf0 in __wine_syscall_dispatcher () from /opt/wine/bin/../lib/wine/aarch64-unix/ntdll.so
which set it to NULL
and 0x0000ffffaa023a0c in NtCurrentTeb () at /home/yukondev/workspace/wine/dlls/ntdll/unix/signal_arm64.c:1449 1449 { (gdb) bt #0 0x0000ffffaa023a0c in NtCurrentTeb () at /home/yukondev/workspace/wine/dlls/ntdll/unix/signal_arm64.c:1449 #1 0x0000ffffaa02493c in ntdll_get_thread_data () at /home/yukondev/workspace/wine/dlls/ntdll/unix/unix_private.h:70 #2 arm64_thread_data () at /home/yukondev/workspace/wine/dlls/ntdll/unix/signal_arm64.c:163 #3 NtCallbackReturn (ret_ptr=0x0, ret_len=0, status=65602) at /home/yukondev/workspace/wine/dlls/ntdll/unix/signal_arm64.c:784 #4 0x0000ffffaa022cf0 in __wine_syscall_dispatcher () from /opt/wine/bin/../lib/wine/aarch64-unix/ntdll.so
which set it to 0xffffaa02493c <NtCallbackReturn+32>, and then 0xffffaa024978 <NtCallbackReturn+92> , then 0xffffaa024984 <NtCallbackReturn+104>
The same sort of thing happens in the x86_64 dispatcher, but there it turns out to be pretty harmless. The key difference is that the function prologue on x86_64 used `push` instructions, and did so prior to the `sub` where it allocated space for locals, so the things popped by the epilogue ended up above callback_frame, rather than below it, and so are not smashed.
0x00007fd17b6dc1d2 <+0>: push %rbp 0x00007fd17b6dc1d3 <+1>: mov %rsp,%rbp 0x00007fd17b6dc1d6 <+4>: push %r14 0x00007fd17b6dc1d8 <+6>: push %r13 0x00007fd17b6dc1da <+8>: push %r12 0x00007fd17b6dc1dc <+10>: push %rdi 0x00007fd17b6dc1dd <+11>: push %rsi 0x00007fd17b6dc1de <+12>: push %rbx 0x00007fd17b6dc1df <+13>: sub $0xa0,%rsp 0x00007fd17b6dc1e6 <+20>: and $0xffffffffffffffc0,%rsp 0x00007fd17b6dc1ea <+24>: sub $0x560,%rsp
There is still a 32-byte gap between the $rsp of KeUserModeCallback and ¤t_frame that is briefly at risk, but 1. This seems to be the register parameter area of the Windows x64 ABI, which is actually volatile space that belongs to the callee even though it's allocated by the caller, so the compiler is not expecting it to survive across function calls (in this case __wine_syscall_dispatcher_return). https://docs.microsoft.com/en-us/cpp/build/stack-usage?view=msvc-170 discusses how this "contains at least 4 entries", i.e. 32 bytes: https://eli.thegreenplace.net/2011/09/06/stack-frame-layout-on-x86-64 2. It doesn't actually get smashed, because __wine_syscall_dispatcher subtracts 0x20 from the restored $rsp before actually making the syscall (https://gitlab.winehq.org/wine/wine/-/blob/d50112b4b6e82782d3924a8dbd443f82f...). Which is presumably since the SysV ABI does *not* make any such reservation, and so __wine_syscall_dispatcher needs to do so to be following the ms_abi.
So together, these mean that on x86_64, it would be OK if __wine_syscall_dispatcher used these 32 bytes between the correct $rsp of KeUserModeCallback and frame->syscall_table (though it doesn't seem to). And the eventual callee gets a $rsp that does *not* overlap with KeUserModeCallback (even though it would be legal for it to do so). I don't know that we have any actual guarantee that callback_frame will be at the bottom, but in practice it seems to be.
But on aarch64, there's important stuff below callback_frame, so this doesn't work out.
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #1 from Kevin Puetz PuetzKevinA@JohnDeere.com --- Oh, I guess I should explain how this leads to that "BUG: holding USER lock".
After we SEGV on the bad return address, this gets translated into SEH, and then swallowed by the syscall wrapper, and then rundll continues on. Next CreateWindow call balks on the lock still held by the one that didn't finish or unwind correctly (because it crashed on this tangled mess of return addresses)
https://bugs.winehq.org/show_bug.cgi?id=53682
Zeb Figura z.figura12@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |z.figura12@gmail.com
--- Comment #2 from Zeb Figura z.figura12@gmail.com --- Created attachment 73209 --> https://bugs.winehq.org/attachment.cgi?id=73209 hack...
This fixes it, right?
It feels very ugly, because the noinline is necessary, but the alternatives I see are
(b) build the whole syscall_frame in assembly (not terrible, but why make more assembly?) or
(c) don't put the unix frame right under the syscall_frame, which means more indirection in the syscall handler (which means the syscall handler is potentially slower).
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #3 from Kevin Puetz PuetzKevinA@JohnDeere.com --- Well, no, that patch won't since NtCallbackReturn assumes the arm64_thread_data()->syscall_frame pointer is actually a user_callback_frame *`, and casts it accordingly to get the jmpbuf. Which call_user_mode_callback didn't copy (or even allocate), so it just jumps off into space.
But yeah, you're on the same track I've been for the shape of a possible solution. And this could be made to work, it just has to copy even more.
The other idea I've been trying think of a way to actually accomplish is to make KeUserModeCallback->__wine_setjmpex into a tail call. i.e. we'd get the jmpbuf to have the return bp/lr values for the *caller* of KeUserModeCallback. Then NtCallbackReturn could just pass status to __wine_longjmp (to make that the return value) and we never KeUserModeCallback and trashing its stack (below the syscall_frame) is fine.
Problem for that is that we need __wine_jmpbuf to save the callee-saved register values from the very beginning of KeUserModeCallback, not somewhere in the middle of it. There's plenty of scratch registers on aarch64 to pull that off, just hard to get the code inserted in the right place.
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #4 from Zeb Figura z.figura12@gmail.com --- (In reply to Kevin Puetz from comment #3)
Well, no, that patch won't since NtCallbackReturn assumes the arm64_thread_data()->syscall_frame pointer is actually a user_callback_frame *`, and casts it accordingly to get the jmpbuf. Which call_user_mode_callback didn't copy (or even allocate), so it just jumps off into space.
But yeah, you're on the same track I've been for the shape of a possible solution. And this could be made to work, it just has to copy even more.
Ech, yeah, right. I probably should have actually tested it, but didn't get the time.
The other idea I've been trying think of a way to actually accomplish is to make KeUserModeCallback->__wine_setjmpex into a tail call. i.e. we'd get the jmpbuf to have the return bp/lr values for the *caller* of KeUserModeCallback. Then NtCallbackReturn could just pass status to __wine_longjmp (to make that the return value) and we never KeUserModeCallback and trashing its stack (below the syscall_frame) is fine.
Problem for that is that we need __wine_jmpbuf to save the callee-saved register values from the very beginning of KeUserModeCallback, not somewhere in the middle of it. There's plenty of scratch registers on aarch64 to pull that off, just hard to get the code inserted in the right place.
Potentially, yeah, although then it comes down to not really using setjmp at all, and rewriting them in assembly. I doubt that's better than solution (b).
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #5 from Kevin Puetz PuetzKevinA@JohnDeere.com ---
Ech, yeah, right. I probably should have actually tested it, but didn't get the time.
No, that's fine. I'm set up to do the test, so I figured you were asking me to do so. And you're helping on my bug report, so that's totally fair :-).
Potentially, yeah, although then it comes down to not really using setjmp at all, and rewriting them in assembly. I doubt that's better than solution (b).
Well, except wine has already written its own __wine_setjmpex/__wine_longjmp in assembly. So that's differnet assembly, but not necessarily more.
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #6 from Kevin Puetz PuetzKevinA@JohnDeere.com --- Created attachment 73231 --> https://bugs.winehq.org/attachment.cgi?id=73231 Working patch
Here's a maybe-less-hacky patch based on your initial approach, but taken farther. This actually does get wineboot to succeed and cmd.exe will launch; haven't tested it further than that yet.
I changed it so the __wine_jmp_buf and NTSTATUS belong to KeUserModeCallback, and are passed in (and represented in the user_callback_frame) as pointers so we don't have to deep-copy anything. This actually makes them a bit more consistent the usage pattern of ret_ptr and ret_len in NtCallbackReturn, so that part seems OK.
I was then getting SIGBUS errors due to the stack not being properly aligned; I thought this was due to call_user_mode_callback lacking WINAPI, but that didn't help (I add it though, since that seems right; KeUserModeCallback had it). It turns out the actual reason is that it was the __wine_jmpbuf that had been giving user_callback_frame its alignment, and with that gone the compiler didn't see any reason to. the syscall_frame becomes $sp, so it has its own reason to need 16 byte alignment, so I added that DECLSPEC_ALIGN.
I think my fudging of the STATUS_STACK_OVERFLOW path is probably still dubious, but all it was really checking before was "some pointer on the stack of KeUserModeCallback", since we already know &callback_frame wasn't actually the bottom of that stack frame. Suggestions welcome.
https://bugs.winehq.org/show_bug.cgi?id=53682
André H. nerv@dawncrow.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |nerv@dawncrow.de
https://bugs.winehq.org/show_bug.cgi?id=53682
Martin Storsjö martin@martin.st changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |martin@martin.st
--- Comment #7 from Martin Storsjö martin@martin.st --- FYI I see others mentioning this issue, but I've successfully run most versions of Wine after 7.14 on aarch64 without problems. I primarily run Wine headless (with X disabled, built with "--without-freetype --without-x"), but I did a recent build now with X enabled too, and I can open GUI apps (tested with explorer.exe) just fine. The mentioned codepaths do get executed in my tests.
I don't quite follow exactly what the issue at hand is - the stack frame of KeUserModeCallback gets clobbered/overlapped by another stack frame?
Can you provide a patch that adds verbose TRACE messages that show exactly where this happens, so I can understand why this seems to work fine for me? (I'm not saying the current code is correct, but I'm curious since I don't run into the issue.) It's easy to grab the current SP register within functions with inline assembly snippets like this:
DWORD64 sp; __asm__ __volatile__ ("mov %0, sp" : "=r"(sp)); TRACE("sp %llx\n", sp);
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #8 from Kevin Puetz PuetzKevinA@JohnDeere.com --- I can try to make something show it with TRACE, but to summarize:
The problem arises in __wine_syscall_dispatcher_return, which sets `$sp = &callback_frame`. Specifially, the path is that after it branches backwards `b 3b` (https://gitlab.winehq.org/wine/wine/-/blob/wine-7.17/dlls/ntdll/unix/signal_...) we reach the `mov sp, x10` (https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/ntdll/unix/signal_arm...); x10 at this point is the `arm64_thread_data()->syscall_frame` (https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/ntdll/unix/signal_arm...). Which is the pointer `arm64_thread_data()->syscall_frame = &callback_frame.frame;` assigned at https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/ntdll/unix/signal_arm....
So far, this seems quite intentional; the arguments are copied below it, etc. The problem is that the C compiler's prologue/epilogue for KeUserModeCallback spilled callee-save registers (lr,fp, and others), and has placed these below callback_frame on the stack. So when the longjmp in NtCallbackReturn brings control flow back into KeUserModeCallback, the code of User32LoadSysMenu (or whatever call was made in the meantime) has smashed them, and `return callback_frame.status` reloads wrong fp/lr values (and then things just go haywire).
I suppose different compilers might generate the prologue/epilogue of KeUserModeCallback; as far as I can tell what's saving it on x86/x86_64 is that the compiler did these things first, with `push`, so the spills end up safely above callback_frame, whereas aarch64 generated the stack space all the space with a preincrement store `stp x29, x30, [sp, #-size]!` which leaves the frame pointer at the bottom of the frame, below the C local variables (like callback_frame).
So if it works for you, all I can think is that your compiler built the stack frame differently. For the tests above I was using ubuntu 22.04's CC = gcc 11.2.0, and CROSSCC = clang 14.0.6-1~oibaf~j
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #9 from Kevin Puetz PuetzKevinA@JohnDeere.com --- Created attachment 73343 --> https://bugs.winehq.org/attachment.cgi?id=73343 patch adding TRACE(...) of the stack pointer
Here's a patch adding some additional TRACE() calls as you requested, but beware this changes the scenario (same bug, but manifests at a different stack):
WINEDEBUG=trace+seh wineboot now shows
0040:trace:seh:KeUserModeCallback sp=0x21f500, syscall_frame=0x21f540 0040:trace:seh:KiUserCallbackDispatcher sp=000000000011F660 0040:trace:seh:NtUserCallOneParam sp=0x21f500
1. So we can see that syscall_frame is not at the bottom of the stack in KeUserModeCallback 2. KiUserCallbackDispatcher is called back on the user stack, but the func it's invoking is a syscall, so we go back through _wine_syscall_dispatcher and a 3. By the time the syscall machinery gets to win32u!NtUserCallOneParam, we are back on the original (syscall) stack, with exactly the same sp==syscall_frame. NtUserCallOneParam's prologue pushes its callee-save stuff, (same amount as KeUserModeCallback), and we enter the C body with the same sp as KeUserModeCallback's body, having trashed the values it saved with those of .
*with* these traces executing, basically the same thing happens, but earlier - ntdll!__wine_dbg_write is *also* a -syscall, so now the first corruption now happens during the TRACE() call added to KiUserCallbackDispatcher, as the prologue of __wine_dbg_write now does the smashing. But it uses fewer registers and so it only smashes 0x21f530...0x21f53f, rather than the whole thing. its call to __GI___libc_write smashes the rest.
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #10 from Kevin Puetz PuetzKevinA@JohnDeere.com --- Argh, sorry for the typos.
1. So we can see that syscall_frame is not at the bottom of the stack in KeUserModeCallback. 2. KiUserCallbackDispatcher is called back on the user stack, but the func it's invoking is a syscall, so we go back through _wine_syscall_dispatcher. 3. By the time the syscall machinery gets to win32u!NtUserCallOneParam, we are back on the original (syscall) stack, with exactly the same sp==syscall_frame. NtUserCallOneParam's prologue pushes its callee-save stuff, (same amount as KeUserModeCallback), and we enter the C function body with the same sp as KeUserModeCallback body, having trashed its values with our own.
*with* these traces executing, basically the same thing happens, but earlier - ntdll!__wine_dbg_write is *also* a -syscall, so now the first corruption now happens during the TRACE() call added to KiUserCallbackDispatcher, as the prologue of __wine_dbg_write now does the smashing. But it uses fewer registers and so it only smashes 0x21f530...0x21f53f, rather than the whole thing. Its call to __GI___libc_write smashes the rest.
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #11 from Martin Storsjö martin@martin.st --- Thanks for the pointers! As discussed on irc, now I was able to reproduce it - it didn't reproduce when building the ELF code with Clang, and it didn't reproduce with Ubuntu 20.04's GCC 9.4.0, but it did reproduce with Ubuntu 22.04's GCC 11, and with vanilla GCC 9.2 built on Ubuntu 20.04.
I think I understand the issue now. So basically, __wine_syscall_dispatcher assumes that syscall_frame is at the very bottom of the syscall stack.
When setting up a new syscall_frame on the current stack, and calling __wine_syscall_dispatcher_return, we'd need to make sure that the new syscall_frame is at the very exact bottom of the stack.
With local variables in C, there is effectively no such guarantee - the compiler is free to do whatever stack layout it wants to. The attached patch, with the separate "DECLSPEC_NORETURN __attribute__((noinline)) call_user_mode_callback", there's a much greater chance of this still holding up (with a noreturn function, there's little point for the compiler to store anything else on the stack. But the compiler still could. (E.g., I recently looked at how MSVC allocates aligned stack objects, and if e.g. syscall_frame would have a larger alignment than the default 16 bytes, MSVC could leave a gap at the bottom of the stack.)
So one way of guaranteeing the stack layout, is to use an assembly wrapper. Either that's a lot of assembly, or we'd do a little bit of duplicate work with a small and neat assembly wrapper (setting up a user_callback_frame in a C function, then having the small assembly function just memcpy it to the right stack location and call __wine_syscall_dispatcher_return).
There's another small fix that does seem to work for me, but it's not a water tight solution (but it can be made good enough). When __wine_syscall_dispatcher gets its syscall_frame, it then also goes on to call the syscall with the stack pointer set exactly to this value. But we can just as well add a small gap between the previous syscall_frame and the stack pointer we set before handing over to the actual syscall implementation. This would waste a couple bytes of stack, but seems to work for me.
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #12 from Martin Storsjö martin@martin.st --- Created attachment 73348 --> https://bugs.winehq.org/attachment.cgi?id=73348 Patch adding a stack gap in __wine_syscall_dispatcher
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #13 from Kevin Puetz PuetzKevinA@JohnDeere.com ---
The attached patch, with the separate "DECLSPEC_NORETURN __attribute__((noinline)) call_user_mode_callback", there's a much greater chance of this still holding up (with a noreturn function, there's little point for the compiler to store anything else on the stack.
No it's stronger than that. It's noinline that's controlling the compiler, noreturn is just descriptive. In that patch, the setjmp(&jmpbuf) is in KeUserModeCallback. But the callback_frame is in call_user_exception_dispatcher, which, always gets its *own* stack frame (because of noinline). We still smash up the stack of call_user_exception_dispatcher, just like it used to for KeUserModeCallback. But since the longjmp points to KeUserModeCallback, the epilogue of call_user_exception_dispatcher will never get a chance to care.
When setting up a new syscall_frame on the current stack, and calling __wine_syscall_dispatcher_return, we'd need to make sure that the new syscall_frame is at the very exact bottom of the stack.
Yes (given the current __wine_syscall_dispatcher_return). zf and I weren't sure whether that is just convenience, or if there was an actual reason why the stack and syscall_frame needed to be contiguous. If you think it's OK to just add an arbitrary gap, then it would seem more robust for asm code in __wine_syscall_dispatcher_return to simply preserve its own $sp value (which was properly set up by its caller), rather than overwriting it with $sp = syscall_frame. Then it's just going to naturally be the correct stack, rather than an arbitrary 0x100 offset.
(maybe this would mean we need a __wine_syscall_dispatcher_return_callback that differs from __wine_syscall_dispatcher_return, if the usage in handle_syscall_fault and call_init_thunk need to do this).
MSVC could leave a gap at the bottom of the stack
Yeah, and on x86_64 MSVC will *always* leave a gap at the bottom, beacuse that's just part of the ABI:
https://learn.microsoft.com/en-us/cpp/build/stack-usage?view=msvc-170#stack-...
The parameter area is always at the bottom of the stack (even if alloca is used), so that it will always be adjacent to the return address during any function call. It contains at least four entries, but always enough space to hold all the parameters needed by any function that may be called. Note that space is always allocated for the register parameters, even if the parameters themselves are never homed to the stack; a callee is guaranteed that space has been allocated for all its parameters
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #14 from Martin Storsjö martin@martin.st --- (In reply to Kevin Puetz from comment #13)
The attached patch, with the separate "DECLSPEC_NORETURN __attribute__((noinline)) call_user_mode_callback", there's a much greater chance of this still holding up (with a noreturn function, there's little point for the compiler to store anything else on the stack.
No it's stronger than that. It's noinline that's controlling the compiler, noreturn is just descriptive. In that patch, the setjmp(&jmpbuf) is in KeUserModeCallback. But the callback_frame is in call_user_exception_dispatcher, which, always gets its *own* stack frame (because of noinline). We still smash up the stack of call_user_exception_dispatcher, just like it used to for KeUserModeCallback. But since the longjmp points to KeUserModeCallback, the epilogue of call_user_exception_dispatcher will never get a chance to care.
Oh, right, that's true. Indeed, that variant should be fairly safe. It's mildly ugly that we're expecting to maybe clobber this stack frame a bit, but it shouldn't have any practical effect.
When setting up a new syscall_frame on the current stack, and calling __wine_syscall_dispatcher_return, we'd need to make sure that the new syscall_frame is at the very exact bottom of the stack.
Yes (given the current __wine_syscall_dispatcher_return). zf and I weren't sure whether that is just convenience, or if there was an actual reason why the stack and syscall_frame needed to be contiguous. If you think it's OK to just add an arbitrary gap, then it would seem more robust for asm code in __wine_syscall_dispatcher_return to simply preserve its own $sp value (which was properly set up by its caller), rather than overwriting it with $sp = syscall_frame. Then it's just going to naturally be the correct stack, rather than an arbitrary 0x100 offset.
(maybe this would mean we need a __wine_syscall_dispatcher_return_callback that differs from __wine_syscall_dispatcher_return, if the usage in handle_syscall_fault and call_init_thunk need to do this).
Oh, yes, that would work too. I tried making such a patch too, and that seems to work for me. I changed the unused align field in syscall_frame into syscall_stack. I made __wine_syscall_dispatcher_callback set this up, and likewise have signal_init_process set it up. But despite this, __wine_syscall_dispatcher does run into a case where this field is zero, early on in the startup. I fixed this by just using the syscall_frame itself as stack pointer, if it was null, but I wonder where I'm missing an initialization of it.
The patch for doing this is fairly small too, but the end of __wine_syscall_dispatcher_callback which restores the original user mode registers still uses sp pointing at syscall_frame. I can rewrite it not to do that (there's a slight risk of clobbering the stack below sp at that point I believe), but I'd leave that to a second separate patch for clarity.
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #15 from Martin Storsjö martin@martin.st --- Created attachment 73352 --> https://bugs.winehq.org/attachment.cgi?id=73352 Patch storing a separate syscall stack pointer separate from the syscall frame itself
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #16 from Kevin Puetz PuetzKevinA@JohnDeere.com ---
I wonder where I'm missing an initialization of it
I think you're missing it in _wine_syscall_dispatcher itself, when entered in the "normal" case (i.e. output_syscalls in tools/winebuild/import.c, when calling a function which the .spec file declared as `-syscall`). In that case the id is in x8, and the syscall_frame is built up on the stack (via all the code prior to the 3: label that __wine_syscall_dispatcher_return is jumping back to).
Or to put it another way, there should be an `str sp, [x10, #0x120]` somewhere in the vicinity of https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/ntdll/unix/signal_arm... to initialize your new syscall_stack (no longer just alignment padding) along with everything else as it builds up a new syscall_frame. Or something like that (haven't actually tested this)...
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #17 from Kevin Puetz PuetzKevinA@JohnDeere.com ---
there's a slight risk of clobbering the stack below sp at that point I believe
Yeah, I see no indications that wine uses sigaltstack outside of wineserver, so there's at least the chance of an ill-timed signal coming in and smashing things up if sp is *ever* pointed at syscall_frame rather than the actual top-of-stack. If signals use the normal stack, you can't just use sp as scratch register even in asm that will eventually put it back.
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #18 from Martin Storsjö martin@martin.st --- (In reply to Kevin Puetz from comment #16)
I wonder where I'm missing an initialization of it
I think you're missing it in _wine_syscall_dispatcher itself, when entered in the "normal" case (i.e. output_syscalls in tools/winebuild/import.c, when calling a function which the .spec file declared as `-syscall`). In that case the id is in x8, and the syscall_frame is built up on the stack (via all the code prior to the 3: label that __wine_syscall_dispatcher_return is jumping back to).
Or to put it another way, there should be an `str sp, [x10, #0x120]` somewhere in the vicinity of https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/ntdll/unix/ signal_arm64.c#L1304-1319 to initialize your new syscall_stack (no longer just alignment padding) along with everything else as it builds up a new syscall_frame. Or something like that (haven't actually tested this)...
No, that's not it - on entry to __wine_syscall_dispatcher, we have an already allocated syscall_frame somewhere on the kernel stack, where we just fill in the register values from the userspace registers - the point being here that we already need to have syscall_frame set up and syscall_stack set. Whenever we allocate a new syscall_frame on the kernel stack, we'd need to set syscall_stack in it to point to the intended stack boundary.
But I found it; while signal_init_process sets up the syscall stack,
void *kernel_stack = (char *)ntdll_get_thread_data()->kernel_stack + kernel_stack_size; arm64_thread_data()->syscall_frame = (struct syscall_frame *)kernel_stack - 1;
call_init_thunk then later wipes the frame and sets it up:
memset( frame, 0, sizeof(*frame) ); NtSetContextThread( GetCurrentThread(), ctx );
frame->sp = (ULONG64)ctx; frame->pc = (ULONG64)pLdrInitializeThunk; frame->x[0] = (ULONG64)ctx; frame->x[18] = (ULONG64)teb; frame->prev_frame = NULL; frame->restore_flags |= CONTEXT_INTEGER; frame->syscall_table = KeServiceDescriptorTable;
pthread_sigmask( SIG_UNBLOCK, &server_block_set, NULL ); __wine_syscall_dispatcher_return( frame, 0 );
If I set up syscall_stack here, it works as I had expected.
(In reply to Kevin Puetz from comment #17)
there's a slight risk of clobbering the stack below sp at that point I believe
Yeah, I see no indications that wine uses sigaltstack outside of wineserver, so there's at least the chance of an ill-timed signal coming in and smashing things up if sp is *ever* pointed at syscall_frame rather than the actual top-of-stack. If signals use the normal stack, you can't just use sp as scratch register even in asm that will eventually put it back.
Ok, good that we agree on that :-)
I'll attach both of those patches too, for reference.
With that, we've got three different ways of fixing this issue: 1. Separate noinline function that sets up the syscall_frame 2. Add a gap on the stack to compensate for the difference between bottom of stack and syscall_frame 3. Keep track of the bottom of the syscall stack and the syscall frame separately
Two of them are watertight - 2. isn't, but it is dead simple (but needs overestimating how much extra stuff the compiler might add in the stack frame, below syscall_stack).
In the end, I would maybe perhaps still lean towards your patch, i.e. 1. here - it's simple and straightforward and doesn't need extra logic in assembly. The fact that part of the compiler generated stack gets clobbered during later syscalls is the only thing that feels odd with it, but it shouldn't make any practical difference.
https://bugs.winehq.org/show_bug.cgi?id=53682
Martin Storsjö martin@martin.st changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #73352|0 |1 is obsolete| |
--- Comment #19 from Martin Storsjö martin@martin.st --- Created attachment 73359 --> https://bugs.winehq.org/attachment.cgi?id=73359 Patch storing a separate syscall stack pointer separate from the syscall frame itself
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #20 from Martin Storsjö martin@martin.st --- Created attachment 73360 --> https://bugs.winehq.org/attachment.cgi?id=73360 Patch avoiding setting sp to syscall_frame when continuing/returning from the syscall
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #21 from Zeb Figura z.figura12@gmail.com --- If I'm reading this right, Alexandre fixed this with the "build the whole frame in assembly" route in https://source.winehq.org/git/wine.git/commitdiff/4069a8b384c15a90a7af66636ec5650eeb53b391 and previous commits.
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #22 from Martin Storsjö martin@martin.st --- (In reply to Zeb Figura from comment #21)
If I'm reading this right, Alexandre fixed this with the "build the whole frame in assembly" route in https://source.winehq.org/git/wine.git/commitdiff/ 4069a8b384c15a90a7af66636ec5650eeb53b391 and previous commits.
Yes, it seems so. However it seems like something is wrong with this commit - in builds where I didn't run into this issue at all before, I'm now seeing the "user_check_not_lock BUG: holding USER lock" message on startup. However it's less fatal than in the cases with GCC where it failed outright directly on startup; the wineprefix setup does seem succeed somewhat, but some process hits this.
So all is not well yet. I'll see when I have time to dig into it...
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #23 from Martin Storsjö martin@martin.st --- (In reply to Martin Storsjö from comment #22)
(In reply to Zeb Figura from comment #21)
If I'm reading this right, Alexandre fixed this with the "build the whole frame in assembly" route in https://source.winehq.org/git/wine.git/commitdiff/ 4069a8b384c15a90a7af66636ec5650eeb53b391 and previous commits.
Yes, it seems so. However it seems like something is wrong with this commit
- in builds where I didn't run into this issue at all before, I'm now seeing
the "user_check_not_lock BUG: holding USER lock" message on startup. However it's less fatal than in the cases with GCC where it failed outright directly on startup; the wineprefix setup does seem succeed somewhat, but some process hits this.
So all is not well yet. I'll see when I have time to dig into it...
With https://gitlab.winehq.org/wine/wine/-/merge_requests/1318, the regression is fixed, and then it does indeed look like this issue is fixed as well!
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #24 from Martin Storsjö martin@martin.st --- (In reply to Martin Storsjö from comment #23)
(In reply to Martin Storsjö from comment #22)
(In reply to Zeb Figura from comment #21)
If I'm reading this right, Alexandre fixed this with the "build the whole frame in assembly" route in https://source.winehq.org/git/wine.git/commitdiff/ 4069a8b384c15a90a7af66636ec5650eeb53b391 and previous commits.
Yes, it seems so. However it seems like something is wrong with this commit
- in builds where I didn't run into this issue at all before, I'm now seeing
the "user_check_not_lock BUG: holding USER lock" message on startup. However it's less fatal than in the cases with GCC where it failed outright directly on startup; the wineprefix setup does seem succeed somewhat, but some process hits this.
So all is not well yet. I'll see when I have time to dig into it...
With https://gitlab.winehq.org/wine/wine/-/merge_requests/1318, the regression is fixed, and then it does indeed look like this issue is fixed as well!
With this merged now, it does look to me like this is issue is fixed. (I had a setup where I could reproduce the issue well, and it seems to be fixed for me there.) Kevin, can you reconfirm it from your point too?
(The main fix landed in 4069a8b384c15a90a7af66636ec5650eeb53b391 and the follow-up fix in 464d3c86dcb1bd057bde84765e4b2ca1aafaec23.)
https://bugs.winehq.org/show_bug.cgi?id=53682
Fabian Maurer dark.shadow4@web.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dark.shadow4@web.de
https://bugs.winehq.org/show_bug.cgi?id=53682
Kevin Puetz PuetzKevinA@JohnDeere.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution|--- |FIXED
--- Comment #25 from Kevin Puetz PuetzKevinA@JohnDeere.com --- Sorry I missed updating the status here, but yes, fixed.
https://bugs.winehq.org/show_bug.cgi?id=53682
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #26 from Alexandre Julliard julliard@winehq.org --- Closing bugs fixed in 8.11.
https://bugs.winehq.org/show_bug.cgi?id=53682
--- Comment #27 from Kevin Puetz PuetzKevinA@JohnDeere.com --- The change thought to fix this was merged in 7.21, I am very tardy in confirming it but it definitely works for me in 8.0.1.
-
WineHQ Bugzilla