Skip to content

Commit

Permalink
[signals] don't free alternate stack until late
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dankamongmen committed Jan 2, 2025
1 parent a8e7971 commit 287aa4d
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 37 deletions.
22 changes: 13 additions & 9 deletions src/lib/direct.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down
43 changes: 25 additions & 18 deletions src/lib/notcurses.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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){
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)){
Expand All @@ -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);
Expand All @@ -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;
}
Expand Down
26 changes: 18 additions & 8 deletions src/lib/unixsig.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
Expand Down Expand Up @@ -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)){
Expand All @@ -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){
Expand All @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
7 changes: 5 additions & 2 deletions src/lib/unixsig.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@ extern "C" {
#include <signal.h>

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);
Expand Down

0 comments on commit 287aa4d

Please sign in to comment.