Avoid signal race in virExec

diff --git a/ChangeLog b/ChangeLog
index b2f6ebb00aff8d9b4c6e3f296b97d21d30635fda..3eba2d5e729c00ab9f4bf48d988f6aaa98b676a1 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+Wed Aug 20 09:35:33 BST 2008 Daniel P. Berrange <berrange@redhat.com>
+
+       Avoid signal race in virExec()
+       * src/util.c: Block signals when forking and clear child's
+       signal handlers.
+       * src/remote_protocol.{c,h,x}: Add config.h include file
+       * src/internal.h: define pthread_sigmask interms of sigprocmask
+       for non-pthreads systems
+
 Wed Aug 20 09:28:33 BST 2008 Daniel P. Berrange <berrange@redhat.com>
 
        * src/util.c: Re-arrange virExec() to improve error reporting

diff --git a/qemud/remote_protocol.c b/qemud/remote_protocol.c
index 1a8e68d60ae12eab508bec6291b722fcd689b592..39a20c24cfbe3c55dec6de59b4720208b50ce4f9 100644 (file)
--- a/qemud/remote_protocol.c
+++ b/qemud/remote_protocol.c
@@ -4,6 +4,7 @@
  */
 
 #include "remote_protocol.h"
+#include <config.h>
 #include "internal.h"
 
 bool_t

diff --git a/qemud/remote_protocol.h b/qemud/remote_protocol.h
index 1ad0c547d87e84b9542c20701e2c290f30e4007e..6950f830d2106a1d00cc77ec52eca18f1ad80e4b 100644 (file)
--- a/qemud/remote_protocol.h
+++ b/qemud/remote_protocol.h
@@ -13,6 +13,7 @@
 extern "C" {
 #endif
 
+#include <config.h>
 #include "internal.h"
 #define REMOTE_MESSAGE_MAX 262144
 #define REMOTE_STRING_MAX 65536

diff --git a/qemud/remote_protocol.x b/qemud/remote_protocol.x
index 340203cbaea00ae164dd8bc2d3cf807239076a93..82bd18e7524cfcce2d33687b4366ce8e24a4f77e 100644 (file)
--- a/qemud/remote_protocol.x
+++ b/qemud/remote_protocol.x
@@ -36,6 +36,7 @@
  * 'REMOTE_'.  This makes names quite long.
  */
 
+%#include <config.h>
 %#include "internal.h"
 
 /*----- Data types. -----*/

diff --git a/src/internal.h b/src/internal.h
index 5120ed46fc25c1aae23b335910d2f7bdeed43d3c..8ea2e91a8f110fed83f3e2ca502e58bfde8a277b 100644 (file)
--- a/src/internal.h
+++ b/src/internal.h
@@ -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 --git a/src/util.c b/src/util.c
index a0b02c120ec746adc593b2d20f7b74e04f5ab080..731145303520460cc852dddbbc66cddee59cd411 100644 (file)
--- a/src/util.c
+++ b/src/util.c
@@ -37,6 +37,7 @@
 #include <sys/wait.h>
 #endif
 #include <string.h>
+#include <signal.h>
 #if HAVE_TERMIOS_H
 #include <termios.h>
 #endif
@@ -53,6 +54,10 @@
 #include "memory.h"
 #include "util-lib.c"
 
+#ifndef NSIG
+# define NSIG 32
+#endif
+
 #ifndef MIN
 # define MIN(a, b) ((a) < (b) ? (a) : (b))
 #endif
@@ -110,9 +115,23 @@ static int
 _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,
@@ -172,6 +191,16 @@ _virExec(virConnectPtr conn,
             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;
     }
@@ -186,6 +215,30 @@ _virExec(virConnectPtr conn,
        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]);
     if (pipeerr[0] > 0)