[libvirt] PATCH: 2/5: Fix signal handler race condition

[Daniel P. Berrange]

This is the same patch from yesterday to fix the signal handler race
condition. We block signals before doing a fork(), and then in the child
reset all signal handlers before finally unblocking all signals. The 
parent will restore its original signal mask after fork. I've fixed the
incorrect return code check on pthread_sigmask() and iterate from 1
instead of 0. I'll switch to pid_t later, since that'll involve changing
all callers too.

In the internal.h file I also #define pthread_sigmask to sigprocmask
for scenarios where we don't have pthread - as per other usage. This
exposed a bug in remote_protocol.c file where it was not including
the config.h file, hence that change here too

 qemud/remote_protocol.c |    1 
 qemud/remote_protocol.h |    1 
 qemud/remote_protocol.x |    1 
 src/internal.h          |    1 
 src/util.c              |   55 +++++++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 58 insertions(+), 1 deletion(-)

Daniel

diff -r 76da44084eee qemud/remote_protocol.c
--- a/qemud/remote_protocol.c	Tue Aug 12 22:10:07 2008 +0100
+++ b/qemud/remote_protocol.c	Wed Aug 13 10:07:22 2008 +0100
@@ -4,6 +4,7 @@
  */
 
 #include "remote_protocol.h"
+#include <config.h>
 #include "internal.h"
 
 bool_t
diff -r 76da44084eee qemud/remote_protocol.h
--- a/qemud/remote_protocol.h	Tue Aug 12 22:10:07 2008 +0100
+++ b/qemud/remote_protocol.h	Wed Aug 13 10:07:22 2008 +0100
@@ -13,6 +13,7 @@
 extern "C" {
 #endif
 
+#include <config.h>
 #include "internal.h"
 #define REMOTE_MESSAGE_MAX 262144
 #define REMOTE_STRING_MAX 65536
diff -r 76da44084eee qemud/remote_protocol.x
--- a/qemud/remote_protocol.x	Tue Aug 12 22:10:07 2008 +0100
+++ b/qemud/remote_protocol.x	Wed Aug 13 10:07:22 2008 +0100
@@ -36,6 +36,7 @@
  * 'REMOTE_'.  This makes names quite long.
  */
 
+%#include <config.h>
 %#include "internal.h"
 
 /*----- Data types. -----*/
diff -r 76da44084eee src/internal.h
--- a/src/internal.h	Tue Aug 12 22:10:07 2008 +0100
+++ b/src/internal.h	Wed Aug 13 10:07:22 2008 +0100
@@ -22,6 +22,7 @@
 #define pthread_mutex_destroy(lk) /*empty*/
 #define pthread_mutex_lock(lk) /*empty*/
 #define pthread_mutex_unlock(lk) /*empty*/
+#define pthread_sigmask(h, s, o) sigprocmask((h), (s), (o))
 #endif
 
 /* The library itself is allowed to use deprecated functions /
diff -r 76da44084eee src/util.c
--- a/src/util.c	Tue Aug 12 22:10:07 2008 +0100
+++ b/src/util.c	Wed Aug 13 10:07:22 2008 +0100
@@ -37,6 +37,7 @@
 #include <sys/wait.h>
 #endif
 #include <string.h>
+#include <signal.h>
 #include "c-ctype.h"
 
 #ifdef HAVE_PATHS_H
@@ -49,6 +50,10 @@
 #include "util.h"
 #include "memory.h"
 #include "util-lib.c"
+
+#ifndef NSIG
+# define NSIG 32
+#endif
 
 #ifndef MIN
 # define MIN(a, b) ((a) < (b) ? (a) : (b))
@@ -102,9 +107,23 @@
 _virExec(virConnectPtr conn,
          const char *const*argv,
          int *retpid, int infd, int *outfd, int *errfd, int non_block) {
-    int pid, null;
+    int pid, null, i;
     int pipeout[2] = {-1,-1};
     int pipeerr[2] = {-1,-1};
+    sigset_t oldmask, newmask;
+    struct sigaction sig_action;
+
+    /*
+     * Need to block signals now, so that child process can safely
+     * kill off caller's signal handlers without a race.
+     */
+    sigfillset(&newmask);
+    if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) {
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                    _("cannot block signals: %s"),
+                    strerror(errno));
+        return -1;
+    }
 
     if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0) {
         ReportError(conn, VIR_ERR_INTERNAL_ERROR,
@@ -164,6 +183,16 @@
             close(pipeerr[1]);
             *errfd = pipeerr[0];
         }
+
+        /* Restore our original signal mask now child is safely
+           running */
+        if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) != 0) {
+            ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                        _("cannot unblock signals: %s"),
+                        strerror(errno));
+            return -1;
+        }
+
         *retpid = pid;
         return 0;
     }
@@ -177,6 +206,30 @@
        get sent to stderr where they stand a fighting chance
        of being seen / logged */
     virSetErrorFunc(NULL, NULL);
+
+    /* Clear out all signal handlers from parent so nothing
+       unexpected can happen in our child once we unblock
+       signals */
+    sig_action.sa_handler = SIG_DFL;
+    sig_action.sa_flags = 0;
+    sigemptyset(&sig_action.sa_mask);
+
+    for (i = 1 ; i < NSIG ; i++)
+        /* Only possible errors are EFAULT or EINVAL
+           The former wont happen, the latter we
+           expect, so no need to check return value */
+        sigaction(i, &sig_action, NULL);
+
+    /* Unmask all signals in child, since we've no idea
+       what the caller's done with their signal mask
+       and don't want to propagate that to children */
+    sigemptyset(&newmask);
+    if (pthread_sigmask(SIG_SETMASK, &newmask, NULL) != 0) {
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                    _("cannot unblock signals: %s"),
+                    strerror(errno));
+        return -1;
+    }
 
     if (pipeout[0] > 0)
         close(pipeout[0]);


-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|