On Tue, Jul 19, 2011 at 10:20:25PM -0600, Eric Blake wrote:
> 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.
NB while we can leak FDs across fork(), we'll never leak FDs into
a child process, since we always close all FDs explicitly before
doing any exec(). So this is a nice cleanup, but doesn't fix any
actual bugs in libvirt code, unless there is some external library
we link to that does unsafe fork/exec.
> 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;
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list