[libvirt] [PATCH RESEND] command: avoid fd leak on failure

[Daniel Veillard]

On Wed, Oct 12, 2011 at 05:59:40PM -0600, Eric Blake wrote:
> virCommandTransferFD promises that the fd is no longer owned by
> the caller.  Normally, we want the fd to remain open until the
> child runs, but in error situations, we must close it earlier.
> 
> * src/util/command.c (virCommandTransferFD): Close fd now if we
> can't track it to close later.
> (virCommandKeepFD): Adjust helper to make this easier.
> ---
> 
> This leak can only happen on OOM or other extreme error conditions,
> but ought to be plugged.  When I originally posted this:
> https://www.redhat.com/archives/libvir-list/2011-July/msg00674.html
> DV was worried that callers might abuse things and use fd
> even after this function closed it; but I proved to myself in
> writing a (non-working) v2 that all callers were already safe,
> and that this v1 was indeed a smaller solution.

  Okidoc then :-)

>  src/util/command.c |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/src/util/command.c b/src/util/command.c
> index 699ba2d..c3ce361 100644
> --- a/src/util/command.c
> +++ b/src/util/command.c
> @@ -730,23 +730,26 @@ virCommandNewArgList(const char *binary, ...)
>   * closing it.  FD must not be one of the three standard streams.  If
>   * transfer is true, then fd will be closed in the parent after a call
>   * to Run/RunAsync/Free, otherwise caller is still responsible for fd.
> + * Returns true if a transferring caller should close FD now, and
> + * false if the transfer is successfully recorded.
>   */
> -static void
> +static bool
>  virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer)
>  {
>      if (!cmd)
> -        return;
> +        return fd > STDERR_FILENO;
> 
>      if (fd <= STDERR_FILENO || FD_SETSIZE <= fd) {
>          if (!cmd->has_error)
>              cmd->has_error = -1;
>          VIR_DEBUG("cannot preserve %d", fd);
> -        return;
> +        return fd > STDERR_FILENO;
>      }
> 
>      FD_SET(fd, &cmd->preserve);
>      if (transfer)
>          FD_SET(fd, &cmd->transfer);
> +    return false;
>  }
> 
>  /**
> @@ -761,7 +764,7 @@ virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer)
>  void
>  virCommandPreserveFD(virCommandPtr cmd, int fd)
>  {
> -    return virCommandKeepFD(cmd, fd, false);
> +    virCommandKeepFD(cmd, fd, false);
>  }
> 
>  /**
> @@ -771,12 +774,14 @@ virCommandPreserveFD(virCommandPtr cmd, int fd)
>   *
>   * Transfer the specified file descriptor
>   * to the child, instead of closing it on exec.
> - * Close the fd in the parent during Run/RunAsync/Free.
> + * The parent should no longer use fd, and the parent's copy will
> + * be automatically closed no later than during Run/RunAsync/Free.
>   */
>  void
>  virCommandTransferFD(virCommandPtr cmd, int fd)
>  {
> -    return virCommandKeepFD(cmd, fd, true);
> +    if (virCommandKeepFD(cmd, fd, true))
> +        VIR_FORCE_CLOSE(fd);
>  }

  ACK,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/