"Daniel P. Berrange" <berrange@redhat.com> wrote:
The LXC patches identified a race condition between fork/exec'ing child processes and signal handlers.
Looks fine modulo a few details:
diff -r 1dbfb08d365d src/util.c ... @@ -104,9 +109,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;
Use pid_t as the type for "pid". Hmm... it'd be good to do the same to preexisting *retpid, (above) too.
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) {
This should test "!= 0", since the function is specified to return "nonzero" upon failure. Two more uses below.
+ ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("cannot block signals: %s"), + strerror(errno)); + return -1; + }
if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0) { ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, @@ -122,6 +141,34 @@ goto cleanup; }
+ if (outfd) { + if(non_block &&
I know this is moved code, but you might want to take the opportunity to add a space between the "if" and the following open parenthesis.
+ virSetNonBlock(pipeout[0]) == -1) { + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to set non-blocking file descriptor flag")); + goto cleanup; + } + + if(virSetCloseExec(pipeout[0]) == -1) { + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to set close-on-exec file descriptor flag")); + goto cleanup; + } + } + if (errfd) { + if(non_block && + virSetNonBlock(pipeerr[0]) == -1) { + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to set non-blocking file descriptor flag")); + goto cleanup; + } + if(virSetCloseExec(pipeerr[0]) == -1) { + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to set close-on-exec file descriptor flag")); + goto cleanup; + } + } + if ((pid = fork()) < 0) { ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("cannot fork child process: %s"), strerror(errno)); @@ -132,33 +179,48 @@ close(null); if (outfd) { close(pipeout[1]); - if(non_block) - if(virSetNonBlock(pipeout[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set non-blocking file descriptor flag")); - - if(virSetCloseExec(pipeout[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set close-on-exec file descriptor flag")); *outfd = pipeout[0]; } if (errfd) { close(pipeerr[1]); - if(non_block) - if(virSetNonBlock(pipeerr[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set non-blocking file descriptor flag")); - - if(virSetCloseExec(pipeerr[0]) == -1) - ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set close-on-exec file descriptor flag")); *errfd = pipeerr[0]; } *retpid = pid; + + /* Restore our original signal mask now child is safely + running */ + if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) < 0) { + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("cannot unblock signals: %s"), + strerror(errno)); + return -1; + } + return 0; }
/* child */ + + /* Clear out all signal handlers from parent so nothing + unexpected can happen in our child here */ + sig_action.sa_handler = SIG_DFL; + sig_action.sa_flags = 0; + sigemptyset(&sig_action.sa_mask); + + for (i = 0 ; i < NSIG ; i++)
I'd start at 1, since afaik, sigaction(0, ... serves no purpose.
+ /* 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 */ + if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) < 0) { + ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("cannot unblock signals: %s"), + strerror(errno)); + return -1; + }
if (pipeout[0] > 0 && close(pipeout[0]) < 0) _exit(1);