4 p.m.
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