7:27 a.m.
The LXC patches identified a race condition between fork/exec'ing child
processes and signal handlers.
The process using libvirt can have setup arbitrary signal handlers. In
the libvirtd case we have one attached to SIGCHILD, and the handler writes
to a pipe which is then processeed in the main loop. When you fork() signal
handlers are preserved in the child process, but you may well not want the
signal handlers to be run in the children - for example in LXC we closed
the FD associated the pipe after fork(), and that FD is now asociated
with a socket we use to talk to the container. So if the original signal
handler is run bad stuff will happen in the child.
Now signal handlers will eventually get reset when you exec(), but this
leaves open a race condition window between the fork & exec() when we
cannot assume it is safe to run the signal handlers from the parent
So, this changes the virExec() function so that before fork()ing it
blocks all signals. After fork() the parent process can restore its
original signal mask. The child process meanwhile calls sigaction()
to reset all signal handlers to SIG_DFL, and then unblocks all signals.
NB, the child does not restore the parents sig-mask - libvirt can be
called from any app, and we don't want to inherit whatever mask the
app may have setup.
On Linux there is a convenient NSIG constant defined in signal.h
telling us how many signals there are. If this isn't defined I
simply set it to 32 which is enough for UNIX pre-dating the
addition of real-time signals to POSIX. If we find convenient
or more appropriate values for other non-Linux OS we can update
this as needed.
NB I call pthread_sigmask() instead of sigprocmask() when doing
the fork/exec, because we cannot assume that the application
using libvirt is single-threaded & thus we only want to block
signals in the thread doing the fork/exec.
Daniel
diff -r 1dbfb08d365d src/util.c
--- a/src/util.c Thu Aug 07 16:55:11 2008 +0100
+++ b/src/util.c Thu Aug 07 16:55:53 2008 +0100
@@ -37,6 +37,7 @@
#include <sys/wait.h>
#endif
#include <string.h>
+#include <signal.h>
#include "c-ctype.h"
#ifdef HAVE_PATHS_H
@@ -49,6 +50,10 @@
#include "util.h"
#include "memory.h"
#include "util-lib.c"
+
+#ifndef NSIG
+# define NSIG 32
+#endif
#ifndef MIN
# define MIN(a, b) ((a) < (b) ? (a) : (b))
@@ -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;
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, 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 &&
+ 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++)
+ /* 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);
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|