Re: [Qemu-devel] [PATCH] io: some fixes to handling of /dev/null when running commands

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/01/2016 14:05, Daniel P. Berrange wrote:
> The /dev/null file handle was leaked in a couple of places.
> There is also the possibility that both readfd and writefd
> point to the same /dev/null file handle, so care must be
> taken not to close the same file handle twice.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  io/channel-command.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/io/channel-command.c b/io/channel-command.c
> index a220fe8..a9c67aa 100644
> --- a/io/channel-command.c
> +++ b/io/channel-command.c
> @@ -66,7 +66,7 @@ qio_channel_command_new_spawn(const char *const argv[],
>  
>      if (stdinnull || stdoutnull) {
>          devnull = open("/dev/null", O_RDWR);
> -        if (!devnull) {
> +        if (devnull < 0) {
>              error_setg_errno(errp, errno,
>                               "Unable to open /dev/null");
>              goto error;
> @@ -98,6 +98,9 @@ qio_channel_command_new_spawn(const char *const argv[],
>              close(stdoutfd[0]);
>              close(stdoutfd[1]);
>          }
> +        if (devnull != -1) {
> +            close(devnull);
> +        }
>  
>          execv(argv[0], (char * const *)argv);
>          _exit(1);
> @@ -117,6 +120,9 @@ qio_channel_command_new_spawn(const char *const argv[],
>      return ioc;
>  
>   error:
> +    if (devnull != -1) {
> +        close(devnull);
> +    }
>      if (stdinfd[0] != -1) {
>          close(stdinfd[0]);
>      }
> @@ -202,12 +208,12 @@ static void qio_channel_command_finalize(Object *obj)
>      QIOChannelCommand *ioc = QIO_CHANNEL_COMMAND(obj);
>      if (ioc->readfd != -1) {
>          close(ioc->readfd);
> -        ioc->readfd = -1;
>      }
> -    if (ioc->writefd != -1) {
> +    if (ioc->writefd != -1 &&
> +        ioc->writefd != ioc->readfd) {
>          close(ioc->writefd);
> -        ioc->writefd = -1;
>      }
> +    ioc->writefd = ioc->readfd = -1;
>      if (ioc->pid > 0) {
>  #ifndef WIN32
>          qio_channel_command_abort(ioc, NULL);
> @@ -299,12 +305,16 @@ static int qio_channel_command_close(QIOChannel *ioc,
>      /* We close FDs before killing, because that
>       * gives a better chance of clean shutdown
>       */
> -    if (close(cioc->writefd) < 0) {
> +    if (cioc->readfd != -1 &&
> +        close(cioc->readfd) < 0) {
>          rv = -1;
>      }
> -    if (close(cioc->readfd) < 0) {
> +    if (cioc->writefd != -1 &&
> +        cioc->writefd != cioc->readfd &&
> +        close(cioc->writefd) < 0) {
>          rv = -1;
>      }
> +    cioc->writefd = cioc->readfd = -1;
>  #ifndef WIN32
>      if (qio_channel_command_abort(cioc, errp) < 0) {
>          return -1;
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>