[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 02/16] command: avoid leaking fds across fork



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]