[libvirt] [PATCH] command: avoid double close bugs

[Wen Congyang]

At 05/30/2012 09:20 AM, Eric Blake Wrote:
> KAMEZAWA Hiroyuki reported a nasty double-free bug when virCommand
> is used to convert a string into input to a child command.  The
> problem is that the poll() loop of virCommandProcessIO would close()
> the write end of the pipe in order to let the child see EOF, then
> the caller virCommandRun() would also close the same fd number, with
> the second close possibly nuking an fd opened by some other thread
> in the meantime.  This in turn can have all sorts of bad effects.
> 
> This is based on his first attempt at a patch, at
> https://bugzilla.redhat.com/show_bug.cgi?id=823716

close fd more twice is the cause of this bug. But there are some
other codes that have the same problem. I am searching all such
codes recent days.

> 
> * src/util/command.c (_virCommand): Drop inpipe member.
> (virCommandProcessIO): Add argument, to avoid closing caller's fd
> without informing caller.
> (virCommandRun, virCommandNewArgs): Adjust clients.
> ---
>  src/util/command.c |   18 ++++++------------
>  1 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/src/util/command.c b/src/util/command.c
> index eaa9f16..bf219e4 100644
> --- a/src/util/command.c
> +++ b/src/util/command.c
> @@ -83,7 +83,6 @@ struct _virCommand {
>      char **errbuf;
> 
>      int infd;
> -    int inpipe;
>      int outfd;
>      int errfd;
>      int *outfdptr;
> @@ -788,7 +787,6 @@ virCommandNewArgs(const char *const*args)
>      cmd->handshakeNotify[1] = -1;
> 
>      cmd->infd = cmd->outfd = cmd->errfd = -1;
> -    cmd->inpipe = -1;
>      cmd->pid = -1;
> 
>      virCommandAddArgSet(cmd, args);
> @@ -1676,7 +1674,7 @@ virCommandTranslateStatus(int status)
>   * Manage input and output to the child process.
>   */
>  static int
> -virCommandProcessIO(virCommandPtr cmd)
> +virCommandProcessIO(virCommandPtr cmd, int *inpipe)
>  {
>      int infd = -1, outfd = -1, errfd = -1;
>      size_t inlen = 0, outlen = 0, errlen = 0;
> @@ -1687,7 +1685,7 @@ virCommandProcessIO(virCommandPtr cmd)
>       * via pipe */
>      if (cmd->inbuf) {
>          inlen = strlen(cmd->inbuf);
> -        infd = cmd->inpipe;
> +        infd = *inpipe;
>      }
> 
>      /* With out/err buffer, the outfd/errfd have been filled with an
> @@ -1807,12 +1805,9 @@ virCommandProcessIO(virCommandPtr cmd)
>                      }
>                  } else {
>                      inoff += done;
> -                    if (inoff == inlen) {
> -                        int tmpfd ATTRIBUTE_UNUSED;
> -                        tmpfd = infd;
> -                        if (VIR_CLOSE(infd) < 0)
> -                            VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
> -                    }
> +                    if (inoff == inlen && VIR_CLOSE(*inpipe) < 0)
> +                        VIR_DEBUG("ignoring failed close on fd %d", infd);
> +                    infd = -1;

if inoff != inlen, we should not set infd to -1.

>                  }
>              }
>          }
> @@ -1938,7 +1933,6 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
>              return -1;
>          }
>          cmd->infd = infd[0];
> -        cmd->inpipe = infd[1];
>      }
> 
>      /* If caller requested the same string for stdout and stderr, then
> @@ -1981,7 +1975,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
>      }
> 
>      if (string_io)
> -        ret = virCommandProcessIO(cmd);
> +        ret = virCommandProcessIO(cmd, &infd[1]);
> 
>      if (virCommandWait(cmd, exitstatus) < 0)
>          ret = -1;