From 287aa4dd587fc1ae56eb505c781321ff5aadddee Mon Sep 17 00:00:00 2001 From: nick black Date: Thu, 2 Jan 2025 08:47:08 -0500 Subject: [PATCH] [signals] don't free alternate stack until late musl uses any registered signal stack for its thread cancellation logic. since alternate stacks are a per-thread deal, but signal handlers are per-process, threads other than the main thread (having not disabled their (shared) alternate signal stack) could get whalloped by musl's "SIGCANCEL". ought close #2828. --- src/lib/direct.c | 22 +++++++++++++--------- src/lib/notcurses.c | 43 +++++++++++++++++++++++++------------------ src/lib/unixsig.c | 26 ++++++++++++++++++-------- src/lib/unixsig.h | 7 +++++-- 4 files changed, 61 insertions(+), 37 deletions(-) diff --git a/src/lib/direct.c b/src/lib/direct.c index 1667acd245..60cbed33de 100644 --- a/src/lib/direct.c +++ b/src/lib/direct.c @@ -832,9 +832,9 @@ int ncdirect_printf_aligned(ncdirect* n, int y, ncalign_e align, const char* fmt } static int -ncdirect_stop_minimal(void* vnc){ +ncdirect_stop_minimal(void* vnc, void** altstack){ ncdirect* nc = vnc; - int ret = drop_signals(nc); + int ret = drop_signals(nc, altstack); fbuf f = {0}; if(fbuf_init_small(&f) == 0){ ret |= reset_term_attributes(&nc->tcache, &f); @@ -939,26 +939,30 @@ ncdirect* ncdirect_core_init(const char* termtype, FILE* outfp, uint64_t flags){ ncdirect_set_styles(ret, 0); return ret; -err: - if(ret->tcache.ttyfd >= 0){ - (void)tcsetattr(ret->tcache.ttyfd, TCSANOW, ret->tcache.tpreserved); +err:{ + void* altstack; + if(ret->tcache.ttyfd >= 0){ + (void)tcsetattr(ret->tcache.ttyfd, TCSANOW, ret->tcache.tpreserved); + } + drop_signals(ret, &altstack); + pthread_mutex_destroy(&ret->stats.lock); + free(ret); } - drop_signals(ret); - pthread_mutex_destroy(&ret->stats.lock); - free(ret); return NULL; } int ncdirect_stop(ncdirect* nc){ int ret = 0; if(nc){ - ret |= ncdirect_stop_minimal(nc); + void* altstack; + ret |= ncdirect_stop_minimal(nc, &altstack); free_terminfo_cache(&nc->tcache); if(nc->tcache.ttyfd >= 0){ ret |= close(nc->tcache.ttyfd); } pthread_mutex_destroy(&nc->stats.lock); free(nc); + free(altstack); } return ret; } diff --git a/src/lib/notcurses.c b/src/lib/notcurses.c index 55a053ce3f..0579bc8f03 100644 --- a/src/lib/notcurses.c +++ b/src/lib/notcurses.c @@ -118,10 +118,10 @@ int reset_term_palette(const tinfo* ti, fbuf* f, unsigned touchedpalette){ // to tear down and account for internal structures. note that we do lots of // shit here that is unsafe within a signal handler =[ FIXME. static int -notcurses_stop_minimal(void* vnc){ +notcurses_stop_minimal(void* vnc, void** altstack){ notcurses* nc = vnc; int ret = 0; - ret |= drop_signals(nc); + ret |= drop_signals(nc, altstack); // collect output into the memstream buffer, and then dump it directly using // blocking_write(), to avoid problems with unreliable fflush(). fbuf* f = &nc->rstate.f; @@ -1285,11 +1285,13 @@ notcurses* notcurses_core_init(const notcurses_options* opts, FILE* outfp){ ret->margin_l, ret->margin_t, ret->margin_r, ret->margin_b, ret->flags & NCOPTION_DRAIN_INPUT)){ + void* altstack; fbuf_free(&ret->rstate.f); pthread_mutex_destroy(&ret->pilelock); pthread_mutex_destroy(&ret->stats.lock); - drop_signals(ret); + drop_signals(ret, &altstack); free(ret); + free(altstack); return NULL; } if(ret->tcache.maxpaletteread > -1){ @@ -1385,20 +1387,22 @@ notcurses* notcurses_core_init(const notcurses_options* opts, FILE* outfp){ } return ret; -err: - logpanic("alas, you will not be going to space today."); - notcurses_stop_minimal(ret); - fbuf_free(&ret->rstate.f); - if(ret->tcache.ttyfd >= 0 && ret->tcache.tpreserved){ - (void)tcsetattr(ret->tcache.ttyfd, TCSAFLUSH, ret->tcache.tpreserved); - free(ret->tcache.tpreserved); - } - drop_signals(ret); - del_curterm(cur_term); - pthread_mutex_destroy(&ret->stats.lock); - pthread_mutex_destroy(&ret->pilelock); - free(ret); - return NULL; +err:{ + void* altstack; + logpanic("alas, you will not be going to space today."); + notcurses_stop_minimal(ret, &altstack); + fbuf_free(&ret->rstate.f); + if(ret->tcache.ttyfd >= 0 && ret->tcache.tpreserved){ + (void)tcsetattr(ret->tcache.ttyfd, TCSAFLUSH, ret->tcache.tpreserved); + free(ret->tcache.tpreserved); + } + del_curterm(cur_term); + pthread_mutex_destroy(&ret->stats.lock); + pthread_mutex_destroy(&ret->pilelock); + free(ret); + free(altstack); + return NULL; + } } // updates *pile to point at (*pile)->next, frees all but standard pile/plane @@ -1444,7 +1448,8 @@ int notcurses_stop(notcurses* nc){ //notcurses_debug(nc, stderr); int ret = 0; if(nc){ - ret |= notcurses_stop_minimal(nc); + void* altstack; + ret |= notcurses_stop_minimal(nc, &altstack); // if we were not using the alternate screen, our cursor's wherever we last // wrote. move it to the furthest place to which it advanced. if(!get_escape(&nc->tcache, ESCAPE_SMCUP)){ @@ -1463,6 +1468,7 @@ int notcurses_stop(notcurses* nc){ } egcpool_dump(&nc->pool); free(nc->lastframe); + // perhaps surprisingly, this stops the input thread free_terminfo_cache(&nc->tcache); // get any current stats loaded into stash_stats notcurses_stats_reset(nc, NULL); @@ -1476,6 +1482,7 @@ int notcurses_stop(notcurses* nc){ ret |= pthread_mutex_destroy(&nc->pilelock); fbuf_free(&nc->rstate.f); free(nc); + free(altstack); } return ret; } diff --git a/src/lib/unixsig.c b/src/lib/unixsig.c index 52c726536f..b61f945533 100644 --- a/src/lib/unixsig.c +++ b/src/lib/unixsig.c @@ -17,8 +17,9 @@ int unblock_signals(const sigset_t* old_blocked_signals){ return 0; } -int drop_signals(void* nc){ +int drop_signals(void* nc, void** altstack){ void* expected = nc; + *altstack = NULL; if(!atomic_compare_exchange_strong(&signal_nc, &expected, NULL)){ return -1; } @@ -29,7 +30,7 @@ int drop_signals(void* nc){ // inhibited), and ensures that only one notcurses/ncdirect context is active // at any given time. int setup_signals(void* vnc, bool no_quit_sigs, bool no_winch_sigs, - int(*handler)(void*)){ + int(*handler)(void*, void**)){ (void)no_quit_sigs; (void)no_winch_sigs; (void)handler; @@ -72,7 +73,7 @@ static struct sigaction old_term; // Prepared in setup_signals(), and never changes across our lifetime. static sigset_t wblock_signals; -static int(*fatal_callback)(void*); // fatal handler callback +static int(*fatal_callback)(void*, void**); // fatal handler callback int block_signals(sigset_t* old_blocked_signals){ if(pthread_sigmask(SIG_BLOCK, &wblock_signals, old_blocked_signals)){ @@ -88,9 +89,18 @@ int unblock_signals(const sigset_t* old_blocked_signals){ return 0; } -int drop_signals(void* nc){ +// we don't want to run our signal handlers (especially those for fatal +// signals) once library shutdown has started. we need to leave any +// alternate signal stack around, however, because musl rather dubiously +// interprets POSIX's sigaltstack() definition to imply our alternate stack +// a wildfire suitable for dousing with the hosting libc's adolescent seed. +// see https://github.com/dankamongmen/notcurses/issues/2828. our child +// threads still have the alternate stack configured, so we must leave it +// up. callers ought thus free *altstack at the very end of things. +int drop_signals(void* nc, void** altstack){ int ret = -1; void* expected = nc; + *altstack = NULL; pthread_mutex_lock(&lock); if(atomic_compare_exchange_strong(&signal_nc, &expected, nc)){ if(handling_winch){ @@ -116,9 +126,9 @@ int drop_signals(void* nc){ fprintf(stderr, "couldn't remove alternate signal stack (%s)", strerror(errno)); } } - free(alt_signal_stack.ss_sp); - alt_signal_stack.ss_sp = NULL; } + *altstack = alt_signal_stack.ss_sp; + alt_signal_stack.ss_sp = NULL; ret = !atomic_compare_exchange_strong(&signal_nc, &expected, NULL); } pthread_mutex_unlock(&lock); @@ -145,7 +155,7 @@ static void fatal_handler(int signo, siginfo_t* siginfo, void* v){ notcurses* nc = atomic_load(&signal_nc); if(nc){ - fatal_callback(nc); + fatal_callback(nc, NULL); // fuck the alt stack save yourselves switch(signo){ case SIGTERM: invoke_old(&old_term, signo, siginfo, v); break; case SIGSEGV: invoke_old(&old_segv, signo, siginfo, v); break; @@ -176,7 +186,7 @@ void setup_alt_sig_stack(void){ // inhibited), and ensures that only one notcurses/ncdirect context is active // at any given time. int setup_signals(void* vnc, bool no_quit_sigs, bool no_winch_sigs, - int(*handler)(void*)){ + int(*handler)(void*, void**)){ notcurses* nc = vnc; void* expected = NULL; struct sigaction sa; diff --git a/src/lib/unixsig.h b/src/lib/unixsig.h index e3d491a890..9ff96c6983 100644 --- a/src/lib/unixsig.h +++ b/src/lib/unixsig.h @@ -8,8 +8,11 @@ extern "C" { #include int setup_signals(void* nc, bool no_quit_sigs, bool no_winch_sig, - int(*handler)(void*)); -int drop_signals(void* nc); + int(*handler)(void*, void**)); + +// call at the beginning of shutdown (we don't want to run fatal signal +// handlers during shutdown!). altstack is written to be freed late. +int drop_signals(void* nc, void** altstack); // block a few signals for the duration of a write to the terminal. int block_signals(sigset_t* old_blocked_signals);