diff --git a/ChangeLog b/ChangeLog index b2f6ebb00a..3eba2d5e72 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +Wed Aug 20 09:35:33 BST 2008 Daniel P. Berrange + + 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 * src/util.c: Re-arrange virExec() to improve error reporting diff --git a/qemud/remote_protocol.c b/qemud/remote_protocol.c index 1a8e68d60a..39a20c24cf 100644 --- a/qemud/remote_protocol.c +++ b/qemud/remote_protocol.c @@ -4,6 +4,7 @@ */ #include "remote_protocol.h" +#include #include "internal.h" bool_t diff --git a/qemud/remote_protocol.h b/qemud/remote_protocol.h index 1ad0c547d8..6950f830d2 100644 --- a/qemud/remote_protocol.h +++ b/qemud/remote_protocol.h @@ -13,6 +13,7 @@ extern "C" { #endif +#include #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 340203cbae..82bd18e752 100644 --- a/qemud/remote_protocol.x +++ b/qemud/remote_protocol.x @@ -36,6 +36,7 @@ * 'REMOTE_'. This makes names quite long. */ +%#include %#include "internal.h" /*----- Data types. -----*/ diff --git a/src/internal.h b/src/internal.h index 5120ed46fc..8ea2e91a8f 100644 --- 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 a0b02c120e..7311453035 100644 --- a/src/util.c +++ b/src/util.c @@ -37,6 +37,7 @@ #include #endif #include +#include #if HAVE_TERMIOS_H #include #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)