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. * src/qemu/qemu_command.c (qemuBuildCommandLine): Simplify caller. --- src/qemu/qemu_command.c | 16 -------------- src/util/command.c | 51 ++++++++++++++++++++++++----------------------- 2 files changed, 26 insertions(+), 41 deletions(-) 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 83d4e88..e4cf389 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -257,7 +257,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"); @@ -266,6 +266,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 @@ -325,7 +337,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; @@ -338,12 +350,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; @@ -356,7 +362,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; @@ -369,12 +375,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; @@ -424,28 +424,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; -- 1.7.4.4