Avoid signal race in virExec

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

qemud/remote_protocol.c

@@ -4,6 +4,7 @@
  */
 
 #include "remote_protocol.h"
+#include <config.h>
 #include "internal.h"
 
 bool_t

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

qemud/remote_protocol.x

@@ -36,6 +36,7 @@
  * 'REMOTE_'.  This makes names quite long.
  */
 
+%#include <config.h>
 %#include "internal.h"
 
 /*----- Data types. -----*/

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 /

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)