INN commit: trunk (6 files)

INN Commit rra at isc.org
Sat Jun 20 20:51:22 UTC 2015


    Date: Saturday, June 20, 2015 @ 13:51:22
  Author: iulius
Revision: 9905

innd: mask signals when not in select

This excludes the possibility of a signal handler accessing data that
the main code is mutating.

Rationale:
When a child (say, nnrpd) terminates, innd gets SIGCHLD and
immediately invokes PROCreap().  This looks for the process in
PROCtable.  However, if another child process has just been started then
PROCwatch() will be part way through executing realloc() to expand
PROCtable at the point the signal arrives.  So PROCreap() may see an old
and possibly corrupted version of the table.

Thanks to Richard Kettlewell for the patch.

Modified:
  trunk/include/inn/libinn.h
  trunk/innd/chan.c
  trunk/innd/innd.c
  trunk/innd/util.c
  trunk/lib/network-innbind.c
  trunk/lib/xsignal.c

-----------------------+
 include/inn/libinn.h  |    4 +
 innd/chan.c           |    5 ++
 innd/innd.c           |    2 
 innd/util.c           |    6 ++
 lib/network-innbind.c |    2 
 lib/xsignal.c         |  105 ++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 123 insertions(+), 1 deletion(-)

Modified: include/inn/libinn.h
===================================================================
--- include/inn/libinn.h	2015-06-20 17:23:51 UTC (rev 9904)
+++ include/inn/libinn.h	2015-06-20 20:51:22 UTC (rev 9905)
@@ -82,6 +82,10 @@
 extern int      setfdlimit(unsigned int limit);
 extern void     (*xsignal(int signum, void (*sigfunc)(int)))(int);
 extern void     (*xsignal_norestart(int signum, void (*sigfunc)(int)))(int);
+extern void     xsignal_mask(void);
+extern void     xsignal_unmask(void);
+extern void     xsignal_enable_masking(void);
+extern void     xsignal_forked(void);
 
 
 /* Headers. */

Modified: innd/chan.c
===================================================================
--- innd/chan.c	2015-06-20 17:23:51 UTC (rev 9904)
+++ innd/chan.c	2015-06-20 20:51:22 UTC (rev 9905)
@@ -1173,8 +1173,13 @@
                 tv.tv_sec = innconf->timer;
             }
         }
+
+        /* Mask signals when not in select to prevent a signal handler
+           from accessing data that the main code is mutating. */
         TMRstart(TMR_IDLE);
+        xsignal_unmask();
         count = select(channels.max_fd + 1, &rdfds, &wrfds, NULL, &tv);
+        xsignal_mask();
         TMRstop(TMR_IDLE);
 
         if (count < 0) {

Modified: innd/innd.c
===================================================================
--- innd/innd.c	2015-06-20 17:23:51 UTC (rev 9904)
+++ innd/innd.c	2015-06-20 20:51:22 UTC (rev 9905)
@@ -387,6 +387,8 @@
        root, switch to running as the news user. */
     ensure_news_user_grp(true, true);
 
+    xsignal_enable_masking();
+
     /* Handle malloc debugging. */
 #if	defined(_DEBUG_MALLOC_INC)
     m.i = M_HANDLE_ABORT;

Modified: innd/util.c
===================================================================
--- innd/util.c	2015-06-20 17:23:51 UTC (rev 9904)
+++ innd/util.c	2015-06-20 20:51:22 UTC (rev 9905)
@@ -212,7 +212,11 @@
     if (i > 0)
         return i;
 
-    /* Child -- do any I/O redirection. */
+    /* Child. */
+    /* Restore signal disposition and mask. */
+    xsignal_forked();
+
+    /* Do any I/O redirection. */
     if (fd0 != 0) {
         if (dup2(fd0, 0) < 0) {
             syslog(L_FATAL, NODUP2, LogName, fd0, 0, av[0]);

Modified: lib/network-innbind.c
===================================================================
--- lib/network-innbind.c	2015-06-20 17:23:51 UTC (rev 9904)
+++ lib/network-innbind.c	2015-06-20 20:51:22 UTC (rev 9905)
@@ -166,6 +166,8 @@
         syswarn("cannot fork innbind for %s, port %hu", address, port);
         return INVALID_SOCKET;
     } else if (child == 0) {
+        /* Restore signal disposition and mask */
+        xsignal_forked();
         message_fatal_cleanup = network_child_fatal;
         socket_close(1);
         if (dup2(pipefds[1], 1) < 0)

Modified: lib/xsignal.c
===================================================================
--- lib/xsignal.c	2015-06-20 17:23:51 UTC (rev 9904)
+++ lib/xsignal.c	2015-06-20 20:51:22 UTC (rev 9905)
@@ -12,12 +12,36 @@
 
 #include "config.h"
 #include "inn/libinn.h"
+#include <errno.h>
 #include <signal.h>
 
 typedef void (*sig_handler_type)(int);
 
 #ifdef HAVE_SIGACTION
 
+static bool signal_masking = false;
+static int signal_max;
+static sigset_t signals_masked, signals_unmasked;
+
+/* If signal masking is enabled, add/remove signum from
+ * signals_masked, and update the current mask to match.
+ */
+static void
+set_signal_handled(int signum, sig_handler_type sigfunc)
+{
+    if (signal_masking) {
+        if (signum > signal_max) {
+            signal_max = signum;
+        }
+        if (sigfunc != SIG_IGN && sigfunc != SIG_DFL) {
+            sigaddset(&signals_masked, signum);
+        } else {
+            sigdelset(&signals_masked, signum);
+        }
+        xsignal_mask();
+    }
+}
+
 sig_handler_type
 xsignal(int signum, sig_handler_type sigfunc)
 {
@@ -35,6 +59,7 @@
 
     if (sigaction(signum, &act, &oact) < 0)
         return SIG_ERR;
+    set_signal_handled(signum, sigfunc);
     return oact.sa_handler;
 }
 
@@ -55,9 +80,66 @@
 
     if (sigaction(signum, &act, &oact) < 0)
         return SIG_ERR;
+    set_signal_handled(signum, sigfunc);
     return oact.sa_handler;
 }
 
+/* Mask (block) all handled signals. */
+void
+xsignal_mask(void)
+{
+    int save_errno = errno;
+    sigprocmask(SIG_SETMASK, &signals_masked, NULL);
+    errno = save_errno;
+}
+
+/* Unmask (unblock) all handled signals. */
+void
+xsignal_unmask(void)
+{
+    int save_errno = errno;
+    sigprocmask(SIG_SETMASK, &signals_unmasked, NULL);
+    errno = save_errno;
+}
+
+/* Enable signal masking.
+ *
+ * The purpose of this is to ensure that signal handlers only run when
+ * nothing else is going on, i.e. so they never access any data
+ * structure while it is in an inconsistent state.
+ */
+void
+xsignal_enable_masking(void)
+{
+    sigprocmask(SIG_SETMASK, NULL, &signals_masked);
+    sigprocmask(SIG_SETMASK, NULL, &signals_unmasked);
+    signal_masking = true;
+}
+
+/* Clean up signal mask for a forked process. */
+void
+xsignal_forked(void)
+{
+    if (signal_masking) {
+        int n;
+
+        /* Remove handlers for signals we handled.  The reason for this is that
+         * the child process could in principle receive one of these signals
+         * after the mask is released.  If this happens, we don't want the
+         * handler to be run (even, or perhaps especially, inside the child) -
+         * chaos would ensue.
+         */
+        for (n = 0; n < signal_max; ++n) {
+            if (sigismember(&signals_masked, n)
+                && !sigismember(&signals_unmasked, n)) {
+                signal(n, SIG_DFL);
+            }
+        }
+        /* Now it's OK to unblock signals we handled. */
+        xsignal_unmask();
+    }
+}
+
 #else /* !HAVE_SIGACTION */
 
 sig_handler_type
@@ -72,4 +154,27 @@
     return signal(signum, sigfunc);
 }
 
+/* Obsolete systems just have to put up with unsafe signal behaviour.
+ * Sorry.
+ */
+void
+xsignal_mask(void)
+{
+}
+
+void
+xsignal_unmask(void)
+{
+}
+
+void
+xsignal_enable_masking(void)
+{
+}
+
+void
+xsignal_forked(void)
+{
+}
+
 #endif /* !HAVE_SIGACTION */



More information about the inn-committers mailing list