[PATCH 0/1] MR2126: ntdll: Print an error if we receive a signal not on the signal stack.
Without this patch the process will silently die. I managed to trigger this while trying to trace Child of Light with Renderdoc, the latter of which crashed in its own "TargetControlServerThread". This, confusingly, manifested in the game restarting itself without the Ubisoft overlay; apparently the game or one of its launchers was capable of recognizing when the process had died and restarting it, but would not try to inject the overlay a second time. I have not investigated the cause of the crash; it is not unlikely that it resulted from the overlay injection (despite the fact that that should only directly affect PE code.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2126
From: Zebediah Figura <zfigura(a)codeweavers.com> Without this patch the process will silently die. I managed to trigger this while trying to trace Child of Light with Renderdoc, the latter of which crashed in its own "TargetControlServerThread". This, confusingly, manifested in the game restarting itself without the Ubisoft overlay; apparently the game or one of its launchers was capable of recognizing when the process had died and restarting it, but would not try to inject the overlay a second time. I have not investigated the cause of the crash; it is not unlikely that it resulted from the overlay injection (despite the fact that that should only directly affect PE code.) --- dlls/ntdll/unix/signal_arm.c | 19 +++++++++++++++++++ dlls/ntdll/unix/signal_arm64.c | 25 ++++++++++++++++++++++++- dlls/ntdll/unix/signal_i386.c | 5 ++++- dlls/ntdll/unix/signal_x86_64.c | 4 ++++ dlls/ntdll/unix/thread.c | 27 +++++++++++++++++++++++++++ dlls/ntdll/unix/unix_private.h | 1 + 6 files changed, 79 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/unix/signal_arm.c b/dlls/ntdll/unix/signal_arm.c index 8180bc52b46..d3d3d0b92e6 100644 --- a/dlls/ntdll/unix/signal_arm.c +++ b/dlls/ntdll/unix/signal_arm.c @@ -750,6 +750,15 @@ NTSTATUS unwind_builtin_dll( void *args ) } +/*********************************************************************** + * init_handler + */ +static inline void init_handler( const ucontext_t *sigcontext ) +{ + check_signal_stack(); +} + + /*********************************************************************** * get_trap_code * @@ -833,6 +842,8 @@ static void save_context( CONTEXT *context, const ucontext_t *sigcontext ) C(0); C(1); C(2); C(3); C(4); C(5); C(6); C(7); C(8); C(9); C(10); #undef C + init_handler( sigcontext ); + context->ContextFlags = CONTEXT_FULL; context->Sp = SP_sig(sigcontext); /* Stack pointer */ context->Lr = LR_sig(sigcontext); /* Link register */ @@ -1302,6 +1313,8 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) EXCEPTION_RECORD rec = { 0 }; ucontext_t *context = sigcontext; + init_handler( sigcontext ); + switch (get_trap_code(signal, context)) { case TRAP_ARM_PRIVINFLT: /* Invalid opcode exception */ @@ -1445,6 +1458,8 @@ static void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { HANDLE handle; + init_handler( sigcontext ); + if (!p__wine_ctrl_routine) return; if (!NtCreateThreadEx( &handle, THREAD_ALL_ACCESS, NULL, NtCurrentProcess(), p__wine_ctrl_routine, 0 /* CTRL_C_EVENT */, 0, 0, 0, 0, NULL )) @@ -1472,6 +1487,8 @@ static void abrt_handler( int signal, siginfo_t *siginfo, void *sigcontext ) */ static void quit_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { + init_handler( sigcontext ); + abort_thread(0); } @@ -1485,6 +1502,8 @@ static void usr1_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { CONTEXT context; + init_handler( sigcontext ); + if (is_inside_syscall( sigcontext )) { context.ContextFlags = CONTEXT_FULL; diff --git a/dlls/ntdll/unix/signal_arm64.c b/dlls/ntdll/unix/signal_arm64.c index bd9818c808a..5d12f75fac5 100644 --- a/dlls/ntdll/unix/signal_arm64.c +++ b/dlls/ntdll/unix/signal_arm64.c @@ -479,6 +479,15 @@ NTSTATUS unwind_builtin_dll( void *args ) } +/*********************************************************************** + * init_handler + */ +static inline void init_handler( const ucontext_t *sigcontext ) +{ + check_signal_stack(); +} + + /*********************************************************************** * save_fpu * @@ -534,6 +543,8 @@ static void save_context( CONTEXT *context, const ucontext_t *sigcontext ) { DWORD i; + init_handler( sigcontext ); + context->ContextFlags = CONTEXT_FULL; context->u.s.Fp = FP_sig(sigcontext); /* Frame pointer */ context->u.s.Lr = LR_sig(sigcontext); /* Link register */ @@ -1060,6 +1071,8 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) EXCEPTION_RECORD rec = { 0 }; ucontext_t *context = sigcontext; + init_handler( sigcontext ); + rec.NumberParameters = 2; rec.ExceptionInformation[0] = (get_fault_esr( context ) & 0x40) != 0; rec.ExceptionInformation[1] = (ULONG_PTR)siginfo->si_addr; @@ -1203,6 +1216,8 @@ static void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { HANDLE handle; + init_handler( sigcontext ); + if (!p__wine_ctrl_routine) return; if (!NtCreateThreadEx( &handle, THREAD_ALL_ACCESS, NULL, NtCurrentProcess(), p__wine_ctrl_routine, 0 /* CTRL_C_EVENT */, 0, 0, 0, 0, NULL )) @@ -1230,6 +1245,8 @@ static void abrt_handler( int signal, siginfo_t *siginfo, void *sigcontext ) */ static void quit_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { + init_handler( sigcontext ); + abort_thread(0); } @@ -1243,6 +1260,8 @@ static void usr1_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { CONTEXT context; + init_handler( sigcontext ); + if (is_inside_syscall( sigcontext )) { context.ContextFlags = CONTEXT_FULL; @@ -1266,10 +1285,14 @@ static void usr1_handler( int signal, siginfo_t *siginfo, void *sigcontext ) */ static void usr2_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { - struct syscall_frame *frame = arm64_thread_data()->syscall_frame; + struct syscall_frame *frame; ucontext_t *context = sigcontext; DWORD i; + init_handler( sigcontext ); + + frame = arm64_thread_data()->syscall_frame; + if (!is_inside_syscall( sigcontext )) return; FP_sig(context) = frame->fp; diff --git a/dlls/ntdll/unix/signal_i386.c b/dlls/ntdll/unix/signal_i386.c index 1cea685d995..3c0647bc831 100644 --- a/dlls/ntdll/unix/signal_i386.c +++ b/dlls/ntdll/unix/signal_i386.c @@ -670,7 +670,10 @@ __ASM_GLOBAL_FUNC( clear_alignment_flag, */ static inline void *init_handler( const ucontext_t *sigcontext ) { - TEB *teb = get_current_teb(); + TEB *teb; + + check_signal_stack(); + teb = get_current_teb(); clear_alignment_flag(); diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 54babc7d964..c065844bf93 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -814,6 +814,8 @@ static inline void set_sigcontext( const CONTEXT *context, ucontext_t *sigcontex */ static inline void init_handler( const ucontext_t *sigcontext ) { + check_signal_stack(); + #ifdef __linux__ if (fs32_sel) { @@ -2130,6 +2132,8 @@ static void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { HANDLE handle; + init_handler( sigcontext ); + if (!p__wine_ctrl_routine) return; if (!NtCreateThreadEx( &handle, THREAD_ALL_ACCESS, NULL, NtCurrentProcess(), p__wine_ctrl_routine, 0 /* CTRL_C_EVENT */, 0, 0, 0, 0, NULL )) diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c index d56962e1721..31582e0b799 100644 --- a/dlls/ntdll/unix/thread.c +++ b/dlls/ntdll/unix/thread.c @@ -1530,6 +1530,33 @@ NTSTATUS WINAPI NtRaiseException( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL } +/*********************************************************************** + * check_signal_stack + */ +void check_signal_stack(void) +{ + stack_t ss; + + sigaltstack( NULL, &ss ); + if (!(ss.ss_flags & SS_ONSTACK)) + { + struct sigaction sa; + + /* if this happened we probably got a signal from a non-wine thread. + * regardless of the reason, we can't get access to the TEB (or there + * isn't one), so print a message and bail */ + fprintf( stderr, "wine: received signal not on the signal stack; terminating\n" ); + + /* exit with SIGSEGV so that we dump core */ + sa.sa_flags = SA_RESETHAND | SA_NODEFER; + sigemptyset( &sa.sa_mask ); + sa.sa_handler = NULL; + sigaction( SIGSEGV, &sa, NULL ); + kill( getpid(), SIGSEGV ); + } +} + + /********************************************************************** * NtCurrentTeb (NTDLL.@) */ diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index 8821cd78491..74d4a50e332 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -193,6 +193,7 @@ extern NTSTATUS get_thread_context( HANDLE handle, void *context, BOOL *self, US extern unsigned int alloc_object_attributes( const OBJECT_ATTRIBUTES *attr, struct object_attributes **ret, data_size_t *ret_len ) DECLSPEC_HIDDEN; extern NTSTATUS system_time_precise( void *args ) DECLSPEC_HIDDEN; +extern void check_signal_stack(void) DECLSPEC_HIDDEN; extern void *anon_mmap_fixed( void *start, size_t size, int prot, int flags ) DECLSPEC_HIDDEN; extern void *anon_mmap_alloc( size_t size, int prot ) DECLSPEC_HIDDEN; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2126
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=129165 Your paranoid android. === debian11 (32 bit report) === Report validation errors: ntdll:exception has no test summary line (early exit of the main process?) === debian11 (32 bit zh:CN report) === Report validation errors: ntdll:exception has no test summary line (early exit of the main process?)
This is helpful on the Mac also, if a non-Wine thread (usually the process main thread) segfaulted it would then segfault again in the handler and end up hung in an infinite loop. With this patch I get a normal Mac crash report and dialog. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2126#note_23384
Should we really crash on any unexpected signal? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2126#note_23407
Should we really crash on any unexpected signal?
All of the signals we catch are signals for which the default action specified by POSIX is to terminate. In the case of TRAP, SEGV, ILL, BUS we have no choice. That might be true for FPE too but I can't remember; but either way, either the program was expecting an exception and won't get it, or it was going to crash anyway. In the case of ABRT, QUIT, INT not terminating seems like the wrong thing to do (and we can't handle them the normal way because we don't have a TEB and can't contact the server). We could potentially ignore USR1 and USR2, but that doesn't seem great for stability. Whatever was sending them had a reason to do so, and isn't going to function correctly. Perhaps more saliently, we were already crashing on unexpected signals, simply by virtue of trying to access the TEB and then dereferencing it. This patch just makes it more obvious. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2126#note_23470
participants (5)
-
Brendan Shanks (＠bshanks) -
Marvin -
Rémi Bernon
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)