1:32 p.m.
"Daniel P. Berrange" <berrange(a)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);