fatal-signal: Run signal hooks outside of actual signal handlers.

Rather than running signal hooks directly from the actual signal
handler, simply record the fact that the signal occured and run
the hook next time around the poll loop.  This allows significantly
more freedom as to what can actually be done in the signal hooks.

1 files changed, 57 insertions, 70 deletions

diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
index 81805214..64b045d3 100644
--- a/lib/fatal-signal.c
+++ b/lib/fatal-signal.c
@@ -20,10 +20,13 @@
 #include <signal.h>
 #include <stdbool.h>
 #include <stdio.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include "poll-loop.h"
 #include "shash.h"
+#include "socket-util.h"
 #include "util.h"
 
 #define THIS_MODULE VLM_fatal_signal
@@ -45,53 +48,32 @@ struct hook {
 static struct hook hooks[MAX_HOOKS];
 static size_t n_hooks;
 
-/* Number of nesting signal blockers. */
-static int block_level = 0;
-
-/* Signal mask saved by outermost signal blocker. */
-static sigset_t saved_signal_mask;
+static int signal_fds[2];
+static volatile sig_atomic_t stored_sig_nr = SIG_ATOMIC_MAX;
 
 /* Disabled by fatal_signal_fork()? */
 static bool disabled;
 
-static void call_sigprocmask(int how, sigset_t* new_set, sigset_t* old_set);
+static void fatal_signal_init(void);
 static void atexit_handler(void);
 static void call_hooks(int sig_nr);
 
-/* Registers 'hook' to be called when a process termination signal is raised.
- * If 'run_at_exit' is true, 'hook' is also called during normal process
- * termination, e.g. when exit() is called or when main() returns.
- *
- * 'func' will be invoked from an asynchronous signal handler, so it must be
- * written appropriately.  For example, it must not call most C library
- * functions, including malloc() or free(). */
-void
-fatal_signal_add_hook(void (*func)(void *aux), void *aux, bool run_at_exit)
-{
-    fatal_signal_block();
-    assert(n_hooks < MAX_HOOKS);
-    hooks[n_hooks].func = func;
-    hooks[n_hooks].aux = aux;
-    hooks[n_hooks].run_at_exit = run_at_exit;
-    n_hooks++;
-    fatal_signal_unblock();
-}
-
-/* Blocks program termination signals until fatal_signal_unblock() is called.
- * May be called multiple times with nesting; if so, fatal_signal_unblock()
- * must be called the same number of times to unblock signals.
- *
- * This is needed while adjusting a data structure that will be accessed by a
- * fatal signal hook, so that the hook is not invoked while the data structure
- * is in an inconsistent state. */
-void
-fatal_signal_block(void)
+static void
+fatal_signal_init(void)
 {
     static bool inited = false;
+
     if (!inited) {
         size_t i;
 
         inited = true;
+
+        if (pipe(signal_fds)) {
+            ovs_fatal(errno, "could not create pipe");
+        }
+        set_nonblocking(signal_fds[0]);
+        set_nonblocking(signal_fds[1]);
+
         sigemptyset(&fatal_signal_set);
         for (i = 0; i < ARRAY_SIZE(fatal_signals); i++) {
             int sig_nr = fatal_signals[i];
@@ -108,23 +90,24 @@ fatal_signal_block(void)
         }
         atexit(atexit_handler);
     }
-
-    if (++block_level == 1) {
-        call_sigprocmask(SIG_BLOCK, &fatal_signal_set, &saved_signal_mask);
-    }
 }
 
-/* Unblocks program termination signals blocked by fatal_signal_block() is
- * called.  If multiple calls to fatal_signal_block() are nested,
- * fatal_signal_unblock() must be called the same number of times to unblock
- * signals. */
+/* Registers 'hook' to be called when a process termination signal is raised.
+ * If 'run_at_exit' is true, 'hook' is also called during normal process
+ * termination, e.g. when exit() is called or when main() returns.
+ *
+ * The hook is not called immediately from the signal handler but rather the
+ * next time the poll loop iterates, so it is freed from the usual restrictions
+ * on signal handler functions. */
 void
-fatal_signal_unblock(void)
+fatal_signal_add_hook(void (*func)(void *aux), void *aux, bool run_at_exit)
 {
-    assert(block_level > 0);
-    if (--block_level == 0) {
-        call_sigprocmask(SIG_SETMASK, &saved_signal_mask, NULL);
-    }
+    fatal_signal_init();
+    assert(n_hooks < MAX_HOOKS);
+    hooks[n_hooks].func = func;
+    hooks[n_hooks].aux = aux;
+    hooks[n_hooks].run_at_exit = run_at_exit;
+    n_hooks++;
 }
 
 /* Handles fatal signal number 'sig_nr'.
@@ -139,12 +122,29 @@ fatal_signal_unblock(void)
 void
 fatal_signal_handler(int sig_nr)
 {
-    call_hooks(sig_nr);
+    ignore(write(signal_fds[1], "", 1));
+    stored_sig_nr = sig_nr;
+}
+
+void
+fatal_signal_run(void)
+{
+    int sig_nr = stored_sig_nr;
 
-    /* Re-raise the signal with the default handling so that the program
-     * termination status reflects that we were killed by this signal */
-    signal(sig_nr, SIG_DFL);
-    raise(sig_nr);
+    if (sig_nr != SIG_ATOMIC_MAX) {
+        call_hooks(sig_nr);
+
+        /* Re-raise the signal with the default handling so that the program
+         * termination status reflects that we were killed by this signal */
+        signal(sig_nr, SIG_DFL);
+        raise(sig_nr);
+    }
+}
+
+void
+fatal_signal_wait(void)
+{
+    poll_fd_wait(signal_fds[0], POLLIN);
 }
 
 static void
@@ -189,11 +189,9 @@ fatal_signal_add_file_to_unlink(const char *file)
         fatal_signal_add_hook(unlink_files, NULL, true);
     }
 
-    fatal_signal_block();
     if (!shash_find(&files, file)) {
         shash_add(&files, file, NULL);
     }
-    fatal_signal_unblock();
 }
 
 /* Unregisters 'file' from being unlinked when the program terminates via
@@ -203,12 +201,10 @@ fatal_signal_remove_file_to_unlink(const char *file)
 {
     struct shash_node *node;
 
-    fatal_signal_block();
     node = shash_find(&files, file);
     if (node) {
         shash_delete(&files, node);
     }
-    fatal_signal_unblock();
 }
 
 /* Like fatal_signal_remove_file_to_unlink(), but also unlinks 'file'.
@@ -232,12 +228,6 @@ unlink_files(void *aux UNUSED)
     do_unlink_files(); 
 }
 
-/* This is a fatal_signal_add_hook() callback (via unlink_files()).  It will be
- * invoked from an asynchronous signal handler, so it cannot call most C
- * library functions (unlink() is an explicit exception, see
- * http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html).
- * That includes free(), so it doesn't try to free the 'files' data
- * structure. */
 static void
 do_unlink_files(void)
 {
@@ -265,13 +255,10 @@ fatal_signal_fork(void)
             signal(sig_nr, SIG_IGN);
         }
     }
-}
-
-static void
-call_sigprocmask(int how, sigset_t* new_set, sigset_t* old_set)
-{
-    int error = sigprocmask(how, new_set, old_set);
-    if (error) {
-        fprintf(stderr, "sigprocmask: %s\n", strerror(errno));
+
+    /* Raise any signals that we have already received with the default
+     * handler. */
+    if (stored_sig_nr != SIG_ATOMIC_MAX) {
+        raise(stored_sig_nr);
     }
 }