[PATCH v4 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.) -- v4: ntdll: Print an error if we receive a signal not on the signal stack. 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 | 2 ++ dlls/ntdll/unix/thread.c | 27 +++++++++++++++++++++++++++ dlls/ntdll/unix/unix_private.h | 1 + 6 files changed, 77 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/unix/signal_arm.c b/dlls/ntdll/unix/signal_arm.c index 681c2e3c0c1..f82d8e4d182 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 3855afc34f4..16fb912a448 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 */ @@ -1265,6 +1276,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; @@ -1408,6 +1421,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 )) @@ -1435,6 +1450,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); } @@ -1448,6 +1465,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; @@ -1471,10 +1490,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 a567aa31b2a..a57734dcdf4 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 07ffb269d19..2aee4b0a37e 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 ucontext_t *init_handler( void *sigcontext ) { + check_signal_stack(); + #ifdef __linux__ if (fs32_sel) { 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 d7d1d0d2ac0..5d89551c622 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -192,6 +192,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=130382 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?)
Jinoh Kang (@iamahuman) commented about dlls/ntdll/unix/thread.c:
+/*********************************************************************** + * 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" ); `fprintf` is not async-signal-safe.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2126#note_26517
Jinoh Kang (@iamahuman) commented about dlls/ntdll/unix/thread.c:
+ */ +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 */ How about using `abort()`?
``` Signal Standard Action Comment ------------------------------------------------------------------------ SIGABRT P1990 Core Abort signal from abort(3) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2126#note_26518
On Thu Mar 9 12:55:34 2023 +0000, Jinoh Kang wrote:
`fprintf` is not async-signal-safe. For example, if the signal occured during a malloc() call or I/O housekeeping, this could lead to an infinite signal trigger loop.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2126#note_26519
On Thu Mar 9 12:59:31 2023 +0000, Jinoh Kang wrote:
For example, if the signal occured during a malloc() call or I/O housekeeping, this could lead to an infinite signal trigger loop. No, it's not (although it's unlikely to matter in practice, and in fact we already do potentially use fprintf in signal handlers... although there are only a couple of places this is possible and they probably should be fixed too.)
Anyway, it's easy to just use write(2, ...) instead, so I'll change it accordingly, thanks. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2126#note_26562
How about using `abort()`?
I guess that also works, and is mildly more appropriate than SEGV. Not sure why I didn't notice it in the first place. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2126#note_26586
participants (4)
-
Jinoh Kang (＠iamahuman) -
Marvin -
Zebediah Figura
-
Zebediah Figura (＠zfigura)