Changeset 18fd5685

Timestamp:

Jul 23, 2011, 9:25:16 PM (13 years ago)

Author:

David Benjamin <davidben@mit.edu>

Branches:

release-1.8

Children:

96e3221

Parents:

aaf8a77

git-author:

David Benjamin <davidben@mit.edu> (07/20/11 02:03:42)

git-committer:

David Benjamin <davidben@mit.edu> (07/23/11 21:25:16)

Message:

Assign all watched signals to a dummy no-op disposition

Otherwise some (namely SIGWINCH) may have SIG_DFL = SIG_IGN. This means
that the signal is ignored if the signal thread is in the middle of
processing another signal (a race condition), and on Solaris it's not
delivered at all.

Also more consistently consider errors in setup here fatal. And rename
CHECK_RESULT to OR_DIE.

Reported-by: Benjamin Kaduk <kaduk@mit.edu>
Reviewed-by: Adam Glasgall <adam@crossproduct.net>

Files:

• owl.c (modified) (2 diffs)
• signal.c (modified) (3 diffs)

owl.c

@@ -389 +389 @@
 }
 
-#define CHECK_RESULT(s, syscall) \
+#define OR_DIE(s, syscall)       \
   G_STMT_START {                 \
-    if ((syscall) != 0) {        \
+    if ((syscall) == -1) {       \
       perror((s));               \
       exit(1);                   \
@@ -402 +402 @@
   sigset_t sigset;
   int ret, i;
-  const int signals[] = { SIGABRT, SIGBUS, SIGCHLD, SIGFPE, SIGHUP, SIGILL,
-                          SIGINT, SIGQUIT, SIGSEGV, SIGTERM, SIGWINCH };
+  const int reset_signals[] = { SIGABRT, SIGBUS, SIGCHLD, SIGFPE, SIGILL,
+                                SIGQUIT, SIGSEGV, };
+  /* Don't bother resetting watched ones because owl_signal_init will. */
+  const int watch_signals[] = { SIGWINCH, SIGTERM, SIGHUP, SIGINT, };
 
   /* Sanitize our signals; the mask and dispositions from our parent
    * aren't really useful. Signal list taken from equivalent code in
    * Chromium. */
-  CHECK_RESULT("sigemptyset", sigemptyset(&sigset));
+  OR_DIE("sigemptyset", sigemptyset(&sigset));
   if ((ret = pthread_sigmask(SIG_SETMASK, &sigset, NULL)) != 0) {
     errno = ret;
     perror("pthread_sigmask");
-  }
-  for (i = 0; i < G_N_ELEMENTS(signals); i++) {
-    CHECK_RESULT("sigaction", sigaction(signals[i], &sig_default, NULL));
+    exit(1);
+  }
+  for (i = 0; i < G_N_ELEMENTS(reset_signals); i++) {
+    OR_DIE("sigaction", sigaction(reset_signals[i], &sig_default, NULL));
   }
 
   /* Turn off SIGPIPE; we check the return value of write. */
-  CHECK_RESULT("sigaction", sigaction(SIGPIPE, &sig_ignore, NULL));
+  OR_DIE("sigaction", sigaction(SIGPIPE, &sig_ignore, NULL));
 
   /* Register some signals with the signal thread. */
-  CHECK_RESULT("sigaddset", sigaddset(&sigset, SIGWINCH));
-  CHECK_RESULT("sigaddset", sigaddset(&sigset, SIGTERM));
-  CHECK_RESULT("sigaddset", sigaddset(&sigset, SIGHUP));
-  CHECK_RESULT("sigaddset", sigaddset(&sigset, SIGINT));
-  owl_signal_init(&sigset, sig_handler, NULL);
+  owl_signal_init(watch_signals, G_N_ELEMENTS(watch_signals),
+                  sig_handler, NULL);
 }
 

signal.c

@@ -1 +1 @@
 #include <errno.h>
+#include <glib.h>
 #include <pthread.h>
 #include <signal.h>
@@ -13 +14 @@
 static void *signal_thread_func(void *data);
 
-/* Initializes the signal thread to listen for 'set' on a dedicated
+static void dummy_handler(int signum)
+{
+  /* Do nothing. This should never get called. It'd be nice to report the error
+   * or something, but you can't have nice things in a signal handler. */
+}
+
+#define OR_DIE(s, syscall)       \
+  G_STMT_START {                 \
+    if ((syscall) == -1) {       \
+      perror((s));               \
+      exit(1);                   \
+    }                            \
+  } G_STMT_END
+
+/* Initializes the signal thread to listen for 'signals' on a dedicated
  * thread. 'callback' is called *on the signal thread* when a signal
  * is received.
@@ -19 +34 @@
  * This function /must/ be called before any other threads are
  * created. (Otherwise the signals will not get blocked correctly.) */
-void owl_signal_init(const sigset_t *set, void (*callback)(const siginfo_t*, void*), void *data) {
+void owl_signal_init(const int *signals, int num_signals, void (*callback)(const siginfo_t*, void*), void *data) {
+  struct sigaction sig_dummy = { .sa_handler = dummy_handler };
   int ret;
+  int i;
 
-  signal_set = *set;
   signal_cb = callback;
   signal_cbdata = data;
+
+  /* Stuff the signals into our sigset_t. Also assign all of them to a dummy
+   * handler. Otherwise, if their default is SIG_IGN, they will get dropped if
+   * delivered while processing. On Solaris, they will not get delivered at
+   * all. */
+  OR_DIE("sigemptyset", sigemptyset(&signal_set));
+  for (i = 0; i < num_signals; i++) {
+    OR_DIE("sigaddset", sigaddset(&signal_set, signals[i]));
+    OR_DIE("sigaction", sigaction(signals[i], &sig_dummy, NULL));
+  }
+
   /* Block these signals in all threads, so we can get them. */
-  if ((ret = pthread_sigmask(SIG_BLOCK, set, NULL)) != 0) {
+  if ((ret = pthread_sigmask(SIG_BLOCK, &signal_set, NULL)) != 0) {
     errno = ret;
     perror("pthread_sigmask");
+    exit(1);
   }
   /* Spawn a dedicated thread to sigwait. */