diff mbox

[v2,05/16] osdep: add qemu_fork() wrapper for safely handling signals

Message ID 1444648509-29179-6-git-send-email-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé Oct. 12, 2015, 11:14 a.m. UTC
When using regular fork() the child process of course inherits
all the parents' signal handlers. If the child then proceeds
to close() any open file descriptors, it may break some of those
registered signal handlers. The child generally does not want to
ever run any of the signal handlers tha parent may have installed
in the short time before it exec's. The parent may also have blocked
various signals which the child process will want enabled.

This introduces a wrapper qemu_fork() that takes care to sanitize
signal handling across fork. Before forking it blocks all signals
in the parent thread. After fork returns, the parent unblocks the
signals and carries on as usual. The child, however, resets all the
signal handlers back to their defaults before it unblocks signals.
The child process can now exec the binary in a "clean" signal
environment.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qemu/osdep.h | 16 ++++++++++++
 util/oslib-posix.c   | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 util/oslib-win32.c   |  9 +++++++
 3 files changed, 96 insertions(+)

Comments

Eric Blake Oct. 19, 2015, 10:24 p.m. UTC | #1
On 10/12/2015 05:14 AM, Daniel P. Berrange wrote:
> When using regular fork() the child process of course inherits
> all the parents' signal handlers. If the child then proceeds
> to close() any open file descriptors, it may break some of those
> registered signal handlers. The child generally does not want to
> ever run any of the signal handlers tha parent may have installed

s/tha/that the/

> in the short time before it exec's. The parent may also have blocked
> various signals which the child process will want enabled.
> 
> This introduces a wrapper qemu_fork() that takes care to sanitize
> signal handling across fork. Before forking it blocks all signals
> in the parent thread. After fork returns, the parent unblocks the
> signals and carries on as usual. The child, however, resets all the
> signal handlers back to their defaults before it unblocks signals.
> The child process can now exec the binary in a "clean" signal
> environment.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/qemu/osdep.h | 16 ++++++++++++
>  util/oslib-posix.c   | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  util/oslib-win32.c   |  9 +++++++
>  3 files changed, 96 insertions(+)

Looks very similar to libvirt's fork() wrapper :)


> +        for (i = 1; i < NSIG; i++) {
> +            /* Only possible errors are EFAULT or EINVAL The former
> +             * won't happen, the latter we expect, so no need to check
> +             * return value */
> +            (void)sigaction(i, &sig_action, NULL);
> +        }

Libvirt gets to rely on gnulib for a guaranteed definition of NSIG.  But
POSIX doesn't document it, and I'm not sure whether it is present on all
platforms qemu targets.  I guess we'll find out soon enough.

Reviewed-by: Eric Blake <eblake@redhat.com>
diff mbox

Patch

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index ef21efb..b568424 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -69,6 +69,8 @@ 
 #include "sysemu/os-posix.h"
 #endif
 
+#include "qapi/error.h"
+
 #if defined(CONFIG_SOLARIS) && CONFIG_SOLARIS_VERSION < 10
 /* [u]int_fast*_t not in <sys/int_types.h> */
 typedef unsigned char           uint_fast8_t;
@@ -286,4 +288,18 @@  void os_mem_prealloc(int fd, char *area, size_t sz);
 
 int qemu_read_password(char *buf, int buf_size);
 
+/**
+ * qemu_fork:
+ *
+ * A version of fork that avoids signal handler race
+ * conditions that can lead to child process getting
+ * signals that are otherwise only expected by the
+ * parent. It also resets all signal handlers to the
+ * default settings.
+ *
+ * Returns 0 to child process, pid number to parent
+ * or -1 on failure.
+ */
+pid_t qemu_fork(Error **errp);
+
 #endif
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index a0fcdc2..4024918 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -490,3 +490,74 @@  int qemu_read_password(char *buf, int buf_size)
     printf("\n");
     return ret;
 }
+
+
+pid_t qemu_fork(Error **errp)
+{
+    sigset_t oldmask, newmask;
+    struct sigaction sig_action;
+    int saved_errno;
+    pid_t pid;
+
+    /*
+     * 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) {
+        error_setg_errno(errp, errno,
+                         "cannot block signals");
+        return -1;
+    }
+
+    pid = fork();
+    saved_errno = errno;
+
+    if (pid < 0) {
+        /* attempt to restore signal mask, but ignore failure, to
+         * avoid obscuring the fork failure */
+        (void)pthread_sigmask(SIG_SETMASK, &oldmask, NULL);
+        error_setg_errno(errp, saved_errno,
+                         "cannot fork child process");
+        errno = saved_errno;
+        return -1;
+    } else if (pid) {
+        /* parent process */
+
+        /* Restore our original signal mask now that the child is
+         * safely running. Only documented failures are EFAULT (not
+         * possible, since we are using just-grabbed mask) or EINVAL
+         * (not possible, since we are using correct arguments).  */
+        (void)pthread_sigmask(SIG_SETMASK, &oldmask, NULL);
+    } else {
+        /* child process */
+        size_t i;
+
+        /* 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
+             * won't happen, the latter we expect, so no need to check
+             * return value */
+            (void)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) {
+            Error *local_err = NULL;
+            error_setg_errno(&local_err, errno,
+                             "cannot unblock signals");
+            error_report_err(local_err);
+            _exit(1);
+        }
+    }
+    return pid;
+}
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 08f5a9c..09f9e98 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -496,3 +496,12 @@  int qemu_read_password(char *buf, int buf_size)
     buf[i] = '\0';
     return 0;
 }
+
+
+pid_t qemu_fork(Error **errp)
+{
+    errno = ENOSYS;
+    error_setg_errno(errp, errno,
+                     "cannot fork child process");
+    return -1;
+}