Re: [libvirt] [PATCHv2 02/16] 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.

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a3bce4a..ee706f9 100644 --- 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 11dd7b0..f8ee8b1 100644 --- 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;