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

@@ -66,7 +66,8 @@ static int active_list=0;
 typedef std::vector<event_t *> 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;
 /**
@@ -247,7 +248,10 @@ void event_add_handler( const event_t *event )
 		signal_handle( e->param1.signal, 1 );
 	}
     
+    // 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<event_t *> *out )
@@ -322,6 +328,28 @@ int event_get( event_t *criterion, std::vector<event_t *> *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) )
 	{
-		/*
+        event_fire_signal(event->param1.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;
 	}
 	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 )

event.h

@@ -122,6 +122,12 @@ void event_remove( event_t *event );
 */
 int event_get( event_t *criterion, std::vector<event_t *> *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<event_t *> *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
 */

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'~' )

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
 */

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

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?
+            /* In an interactive session, tell the principal parser to skip all blocks we're executing so control-C returns control to the user. */
-			block_t *c = NULL;//parser.current_block;
 			if( p && found_proc )
 			{
-				while( c )
+                parser_t::skip_all_blocks();
-				{
-					c->skip=1;
-					c=c->outer;
-				}		
 			}			
 		}
 	}
@@ -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 )
 {
 	

proc.h

@@ -433,7 +433,9 @@ typedef std::list<job_t *> 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;

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 )
+        parser_t::skip_all_blocks();
-		{
-			c->type=FAKE;
-			c->skip=1;
-			c=c->outer;
-		}
 	}
 	
 	interrupted = 1;

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_is_signal_observed(signal))
-	if( event_get( &e, 0 ) )
+    {
-	{
+		event_fire_signal(signal);
-		event_fire( &e );
 	}
 }
 
@@ -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_is_signal_observed(SIGHUP))
-	if( event_get( &e, 0 ) )
 	{
-		default_handler( sig, 0, 0 );	
+		default_handler(sig, 0, 0);
 	}
 	else
 	{
-		reader_exit( 1, 1 );
+		reader_exit(1, 1);
 	}	
-	
 }
 
 /**