From 69446be1ee063771f2e0b498a50f9b0787ea70c7 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 4 Jun 2012 14:20:01 -0700 Subject: [PATCH] Signal handling cleanup and improved safety Fixes issue where you couldn't control-C out of a loop (https://github.com/ridiculousfish/fishfish/issues/13) Also stops doing memory allocation in the signal handler (oops) https://github.com/ridiculousfish/fishfish/issues/27 --- event.cpp | 69 ++++++++++++++++++++++++++++++++++++++++-------------- event.h | 9 +++++++ expand.cpp | 8 ------- parser.cpp | 21 +++++++++++++++++ parser.h | 5 ++++ proc.cpp | 17 +++++--------- proc.h | 4 +++- reader.cpp | 11 ++------- signal.cpp | 17 ++++++-------- 9 files changed, 104 insertions(+), 57 deletions(-) diff --git a/event.cpp b/event.cpp index 33b32daa9..368f2d6c0 100644 --- a/event.cpp +++ b/event.cpp @@ -66,7 +66,8 @@ static int active_list=0; typedef std::vector event_list_t; /** - List of event handlers + List of event handlers. + Note this is inspected by our signal handler, so we must block signals around manipulating it. */ static event_list_t events; /** @@ -246,8 +247,11 @@ void event_add_handler( const event_t *event ) { signal_handle( e->param1.signal, 1 ); } - - events.push_back(e); + + // Block around updating the events vector + signal_block(); + events.push_back(e); + signal_unblock(); } void event_remove( event_t *criterion ) @@ -296,7 +300,9 @@ void event_remove( event_t *criterion ) new_list.push_back(n); } } + signal_block(); events.swap(new_list); + signal_unblock(); } int event_get( event_t *criterion, std::vector *out ) @@ -322,6 +328,28 @@ int event_get( event_t *criterion, std::vector *out ) return found; } +bool event_is_signal_observed(int sig) +{ + /* We are in a signal handler! Don't allocate memory, etc. + This does what event_match does, except it doesn't require passing in an event_t. + */ + size_t i, max = events.size(); + for (i=0; i < max; i++) + { + const event_t *event = events[i]; + if (event->type == EVENT_ANY) + { + return true; + } + else if (event->type == EVENT_SIGNAL) + { + if( event->param1.signal == EVENT_ANY_SIGNAL || event->param1.signal == sig) + return true; + } + } + return false; +} + /** Free all events in the kill list */ @@ -519,26 +547,32 @@ static void event_fire_delayed() } } +void event_fire_signal(int signal) +{ + /* + This means we are in a signal handler. We must be very + careful not do do anything that could cause a memory + allocation or something else that might be bad when in a + signal handler. + */ + if( sig_list[active_list].count < SIG_UNHANDLED_MAX ) + sig_list[active_list].signal[sig_list[active_list].count++]=signal; + else + sig_list[active_list].overflow=1; +} + void event_fire( event_t *event ) { - is_event++; if( event && (event->type == EVENT_SIGNAL) ) { - /* - This means we are in a signal handler. We must be very - careful not do do anything that could cause a memory - allocation or something else that might be bad when in a - signal handler. - */ - if( sig_list[active_list].count < SIG_UNHANDLED_MAX ) - sig_list[active_list].signal[sig_list[active_list].count++]=event->param1.signal; - else - sig_list[active_list].overflow=1; + event_fire_signal(event->param1.signal); } else { + is_event++; + /* Fire events triggered by signals */ @@ -555,9 +589,8 @@ void event_fire( event_t *event ) event_fire_internal( event ); } } - + is_event--; } - is_event--; } @@ -569,10 +602,10 @@ void event_destroy() { for_each(events.begin(), events.end(), event_free); - events.resize(0); + events.clear(); for_each(killme.begin(), killme.end(), event_free); - killme.resize(0); + killme.clear(); } void event_free( event_t *e ) diff --git a/event.h b/event.h index 402b3594b..c176a50ab 100644 --- a/event.h +++ b/event.h @@ -122,6 +122,12 @@ void event_remove( event_t *event ); */ int event_get( event_t *criterion, std::vector *out ); +/** + Returns whether an event listener is registered for the given signal. + This is safe to call from a signal handler. +*/ +bool event_is_signal_observed(int signal); + /** Fire the specified event. The function_name field of the event must be set to 0. If the event is of type EVENT_SIGNAL, no the event is @@ -140,6 +146,9 @@ int event_get( event_t *criterion, std::vector *out ); */ void event_fire( event_t *event ); +/** Like event_fire, but takes a signal directly. */ +void event_fire_signal(int signal); + /** Initialize the event-handling library */ diff --git a/expand.cpp b/expand.cpp index 9e19cec0e..28e2c6902 100644 --- a/expand.cpp +++ b/expand.cpp @@ -1341,14 +1341,6 @@ static void expand_tilde_internal( wcstring &input ) } } -static wchar_t * expand_tilde_internal_compat( wchar_t *in ) -{ - wcstring tmp = in; - expand_tilde_internal(tmp); - free(in); - return wcsdup(tmp.c_str()); -} - void expand_tilde( wcstring &input) { if( ! input.empty() && input.at(0) == L'~' ) diff --git a/parser.cpp b/parser.cpp index 1d91e5b38..f93dbf3a6 100644 --- a/parser.cpp +++ b/parser.cpp @@ -371,14 +371,35 @@ parser_t::parser_t(enum parser_type_t type, bool errors) : } +/* A pointer to the principal parser (which is a static local) */ +static parser_t *s_principal_parser = NULL; + parser_t &parser_t::principal_parser(void) { ASSERT_IS_NOT_FORKED_CHILD(); ASSERT_IS_MAIN_THREAD(); static parser_t parser(PARSER_TYPE_GENERAL, true); + if (! s_principal_parser) + { + s_principal_parser = &parser; + } return parser; } +void parser_t::skip_all_blocks(void) +{ + /* Tell all blocks to skip */ + if (s_principal_parser) + { + block_t *c = s_principal_parser->current_block; + while( c ) + { + c->skip = true; + c = c->outer; + } + } +} + /** Return the current number of block nestings */ diff --git a/parser.h b/parser.h index ca97fc733..8ff8b3060 100644 --- a/parser.h +++ b/parser.h @@ -332,6 +332,11 @@ class parser_t { /** Get the "principal" parser, whatever that is */ static parser_t &principal_parser(); + /** Indicates that execution of all blocks in the principal parser should stop. + This is called from signal handlers! + */ + static void skip_all_blocks(); + /** Create a parser of the given type */ parser_t(enum parser_type_t type, bool show_errors); diff --git a/proc.cpp b/proc.cpp index e084f1e44..4908a1d75 100644 --- a/proc.cpp +++ b/proc.cpp @@ -399,11 +399,10 @@ static void mark_process_status( const job_t *j, */ static void handle_child_status( pid_t pid, int status ) { - int found_proc = 0; + bool found_proc = false; const job_t *j=0; process_t *p=0; // char mess[MESS_SIZE]; - found_proc = 0; /* snprintf( mess, MESS_SIZE, @@ -413,7 +412,7 @@ static void handle_child_status( pid_t pid, int status ) */ job_iterator_t jobs; - while ((j = jobs.next())) + while (! found_proc && (j = jobs.next())) { process_t *prev=0; for( p=j->first_process; p; p=p->next ) @@ -442,7 +441,7 @@ static void handle_child_status( pid_t pid, int status ) kill(prev->pid,SIGPIPE); } } - found_proc = 1; + found_proc = true; break; } prev = p; @@ -466,15 +465,10 @@ static void handle_child_status( pid_t pid, int status ) } else { - //PCA INSTANCED_PARSER what is this? - block_t *c = NULL;//parser.current_block; + /* In an interactive session, tell the principal parser to skip all blocks we're executing so control-C returns control to the user. */ if( p && found_proc ) { - while( c ) - { - c->skip=1; - c=c->outer; - } + parser_t::skip_all_blocks(); } } } @@ -499,6 +493,7 @@ static void handle_child_status( pid_t pid, int status ) } +/* This is called from a signal handler */ void job_handle_signal ( int signal, siginfo_t *info, void *con ) { diff --git a/proc.h b/proc.h index ae8a85434..a52a19950 100644 --- a/proc.h +++ b/proc.h @@ -433,7 +433,9 @@ typedef std::list job_list_t; bool job_list_is_empty(void); -/** A class to aid iteration over jobs list */ +/** A class to aid iteration over jobs list. + Note this is used from a signal handler, so it must be careful to not allocate memory. +*/ class job_iterator_t { job_list_t * const job_list; job_list_t::iterator current, end; diff --git a/reader.cpp b/reader.cpp index bc6b51239..ca3ac3159 100644 --- a/reader.cpp +++ b/reader.cpp @@ -493,19 +493,12 @@ static void reader_kill( size_t begin_idx, int length, int mode, int newv ) } +/* This is called from a signal handler! */ void reader_handle_int( int sig ) { - //PCA INSTANCED_PARSER what is this? - block_t *c = NULL;//current_block; - if( !is_interactive_read ) { - while( c ) - { - c->type=FAKE; - c->skip=1; - c=c->outer; - } + parser_t::skip_all_blocks(); } interrupted = 1; diff --git a/signal.cpp b/signal.cpp index 83a4b13eb..dec6187e3 100644 --- a/signal.cpp +++ b/signal.cpp @@ -427,10 +427,9 @@ const wchar_t *signal_get_desc( int sig ) */ static void default_handler(int signal, siginfo_t *info, void *context) { - event_t e = event_t::signal_event(signal); - if( event_get( &e, 0 ) ) - { - event_fire( &e ); + if (event_is_signal_observed(signal)) + { + event_fire_signal(signal); } } @@ -449,16 +448,14 @@ static void handle_winch( int sig, siginfo_t *info, void *context ) */ static void handle_hup( int sig, siginfo_t *info, void *context ) { - event_t e = event_t::signal_event(SIGHUP); - if( event_get( &e, 0 ) ) + if (event_is_signal_observed(SIGHUP)) { - default_handler( sig, 0, 0 ); + default_handler(sig, 0, 0); } else { - reader_exit( 1, 1 ); - } - + reader_exit(1, 1); + } } /**