command: avoid leaking fds across fork

Since libvirt is multi-threaded, we should use FD_CLOEXEC as much
as possible in the parent, and only relax fds to inherited after
forking, to avoid leaking an fd created in one thread to a fork
run in another thread.  This gets us closer to that ideal, by
making virCommand automatically clear FD_CLOEXEC on fds intended
for the child, as well as avoiding a window of time with non-cloexec
pipes created for capturing output.

* src/util/command.c (virExecWithHook): Use CLOEXEC in parent.  In
child, guarantee that all fds to pass to child are inheritable.
(getDevNull): Use CLOEXEC.
(prepareStdFd): New helper function.
(virCommandRun, virCommandRequireHandshake): Use pipe2.
* src/qemu/qemu_command.c (qemuBuildCommandLine): Simplify caller.

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a3bce4ac58885f3513e27d002ae3b41c98de9990..ee706f9a869ac49ef933982b16e221eed4f15a04 100644 (file)
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4575,14 +4575,6 @@ qemuBuildCommandLine(virConnectPtr conn,
         } else if (STREQ(migrateFrom, "stdio")) {
             if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) {
                 virCommandAddArgFormat(cmd, "fd:%d", migrateFd);
-                /* migrateFd might be cloexec, but qemu must inherit
-                 * it if vmop indicates qemu will be executed */
-                if (vmop != VIR_VM_OP_NO_OP &&
-                    virSetInherit(migrateFd, true) < 0) {
-                    qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                                    _("Failed to clear cloexec flag"));
-                    goto error;
-                }
                 virCommandPreserveFD(cmd, migrateFd);
             } else if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) {
                 virCommandAddArg(cmd, "exec:cat");
@@ -4612,14 +4604,6 @@ qemuBuildCommandLine(virConnectPtr conn,
                 goto error;
             }
             virCommandAddArg(cmd, migrateFrom);
-            /* migrateFd might be cloexec, but qemu must inherit
-             * it if vmop indicates qemu will be executed */
-            if (vmop != VIR_VM_OP_NO_OP &&
-                virSetInherit(migrateFd, true) < 0) {
-                qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                                _("Failed to clear cloexec flag"));
-                goto error;
-            }
             virCommandPreserveFD(cmd, migrateFd);
         } else if (STRPREFIX(migrateFrom, "unix")) {
             if (!qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX)) {

diff --git a/src/util/command.c b/src/util/command.c
index 11dd7b0a6e88be1f1288b049e637a0a6fd9c9545..f8ee8b12f2b7900c88ea0bca7c9e67426dd314d7 100644 (file)
--- a/src/util/command.c
+++ b/src/util/command.c
@@ -259,7 +259,7 @@ cleanup:
 static int
 getDevNull(int *null)
 {
-    if (*null == -1 && (*null = open("/dev/null", O_RDWR)) < 0) {
+    if (*null == -1 && (*null = open("/dev/null", O_RDWR|O_CLOEXEC)) < 0) {
         virReportSystemError(errno,
                              _("cannot open %s"),
                              "/dev/null");
@@ -268,6 +268,18 @@ getDevNull(int *null)
     return 0;
 }
 
+/* Ensure that STD is an inheritable copy of FD.  Return 0 on success,
+ * -1 on failure.  */
+static int
+prepareStdFd(int fd, int std)
+{
+    if (fd == std)
+        return virSetInherit(fd, true);
+    if (dup2(fd, std) != std)
+        return -1;
+    return 0;
+}
+
 /*
  * @argv argv to exec
  * @envp optional environment to use for exec
@@ -327,7 +339,7 @@ virExecWithHook(const char *const*argv,
 
     if (outfd != NULL) {
         if (*outfd == -1) {
-            if (pipe(pipeout) < 0) {
+            if (pipe2(pipeout, O_CLOEXEC) < 0) {
                 virReportSystemError(errno,
                                      "%s", _("cannot create pipe"));
                 goto cleanup;
@@ -340,12 +352,6 @@ virExecWithHook(const char *const*argv,
                 goto cleanup;
             }
 
-            if (virSetCloseExec(pipeout[0]) == -1) {
-                virReportSystemError(errno,
-                                     "%s", _("Failed to set close-on-exec file descriptor flag"));
-                goto cleanup;
-            }
-
             childout = pipeout[1];
         } else {
             childout = *outfd;
@@ -358,7 +364,7 @@ virExecWithHook(const char *const*argv,
 
     if (errfd != NULL) {
         if (*errfd == -1) {
-            if (pipe(pipeerr) < 0) {
+            if (pipe2(pipeerr, O_CLOEXEC) < 0) {
                 virReportSystemError(errno,
                                      "%s", _("Failed to create pipe"));
                 goto cleanup;
@@ -371,12 +377,6 @@ virExecWithHook(const char *const*argv,
                 goto cleanup;
             }
 
-            if (virSetCloseExec(pipeerr[0]) == -1) {
-                virReportSystemError(errno,
-                                     "%s", _("Failed to set close-on-exec file descriptor flag"));
-                goto cleanup;
-            }
-
             childerr = pipeerr[1];
         } else {
             childerr = *errfd;
@@ -426,28 +426,29 @@ virExecWithHook(const char *const*argv,
     }
 
     openmax = sysconf(_SC_OPEN_MAX);
-    for (i = 3; i < openmax; i++)
-        if (i != infd &&
-            i != childout &&
-            i != childerr &&
-            (!keepfd || i >= FD_SETSIZE || !FD_ISSET(i, keepfd))) {
+    for (i = 3; i < openmax; i++) {
+        if (i == infd || i == childout || i == childerr)
+            continue;
+        if (!keepfd || i >= FD_SETSIZE || !FD_ISSET(i, keepfd)) {
             tmpfd = i;
             VIR_FORCE_CLOSE(tmpfd);
+        } else if (virSetInherit(i, true) < 0) {
+            virReportSystemError(errno, _("failed to preserve fd %d"), i);
+            goto fork_error;
         }
+    }
 
-    if (dup2(infd, STDIN_FILENO) < 0) {
+    if (prepareStdFd(infd, STDIN_FILENO) < 0) {
         virReportSystemError(errno,
                              "%s", _("failed to setup stdin file handle"));
         goto fork_error;
     }
-    if (childout > 0 &&
-        dup2(childout, STDOUT_FILENO) < 0) {
+    if (childout > 0 && prepareStdFd(childout, STDOUT_FILENO) < 0) {
         virReportSystemError(errno,
                              "%s", _("failed to setup stdout file handle"));
         goto fork_error;
     }
-    if (childerr > 0 &&
-        dup2(childerr, STDERR_FILENO) < 0) {
+    if (childerr > 0 && prepareStdFd(childerr, STDERR_FILENO) < 0) {
         virReportSystemError(errno,
                              "%s", _("failed to setup stderr file handle"));
         goto fork_error;
@@ -1805,7 +1806,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
     /* If we have an input buffer, we need
      * a pipe to feed the data to the child */
     if (cmd->inbuf) {
-        if (pipe(infd) < 0) {
+        if (pipe2(infd, O_CLOEXEC) < 0) {
             virReportSystemError(errno, "%s",
                                  _("unable to open pipe"));
             cmd->has_error = -1;
@@ -2295,11 +2296,11 @@ void virCommandRequireHandshake(virCommandPtr cmd)
         return;
     }
 
-    if (pipe(cmd->handshakeWait) < 0) {
+    if (pipe2(cmd->handshakeWait, O_CLOEXEC) < 0) {
         cmd->has_error = errno;
         return;
     }
-    if (pipe(cmd->handshakeNotify) < 0) {
+    if (pipe2(cmd->handshakeNotify, O_CLOEXEC) < 0) {
         VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
         VIR_FORCE_CLOSE(cmd->handshakeWait[1]);
         cmd->has_error = errno;