[libvirt] [PATCH] qemuMonitorTextGetMemoryStats: plug a leak on an error path

[Daniel P. Berrange]

On Wed, Jan 20, 2010 at 06:48:31PM +0100, Jim Meyering wrote:
> Coverity complained about a leak via this return -1
> in qemu_monitor_text.c:
> 
>   int qemuMonitorTextGetMemoryStats(qemuMonitorPtr mon,
>                                     virDomainMemoryStatPtr stats,
>                                     unsigned int nr_stats)
>   {
>       char *reply = NULL;
>       int ret = 0;
>       char *offset;
> 
>       if (qemuMonitorCommand(mon, "info balloon", &reply) < 0) {
>           qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED,
>                            "%s", _("could not query memory balloon statistics"));
>           return -1;
>       }
> 
> That can happen because
> qemuMonitorCommand calls
> qemuMonitorCommandWithFd which calls
> qemuMonitorCommandWithHandler, which does this:
> 
> 
> 218    ret = qemuMonitorSend(mon, &msg);
> ...
> 228  	    if (msg.rxBuffer) {
> 229  	        *reply = msg.rxBuffer;
> 230  	    } else {
> 231  	        *reply = strdup("");
> 232  	        if (!*reply) {
> 233  	            virReportOOMError(NULL);
> 234  	            return -1;
> 235  	        }
> 236  	    }
> 237
> 238  	    if (ret < 0)
> 239  	        virReportSystemError(NULL, msg.lastErrno,
> 240  	                             _("cannot send monitor command '%s'"), cmd);
> 241
> 242  	    return ret;
> 243  	}
> 
> That function breaks contract by failing to free *reply when it
> returns a negative value.  Here's the fix:
> 
> >From 3b44df075f9d4330ec27d59eddaa0a32c20d7ac1 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering at redhat.com>
> Date: Wed, 20 Jan 2010 18:24:47 +0100
> Subject: [PATCH] qemuMonitorTextGetMemoryStats: plug a leak on an error path
> 
> * src/qemu/qemu_monitor_text.c (qemuMonitorCommandWithHandler):
> Always free *reply, upon failure.
> ---
>  src/qemu/qemu_monitor_text.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index c3848b5..d921c7e 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -1,7 +1,7 @@
>  /*
>   * qemu_monitor_text.c: interaction with QEMU monitor console
>   *
> - * Copyright (C) 2006-2009 Red Hat, Inc.
> + * Copyright (C) 2006-2010 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -235,9 +235,11 @@ qemuMonitorCommandWithHandler(qemuMonitorPtr mon,
>          }
>      }
> 
> -    if (ret < 0)
> +    if (ret < 0) {
>          virReportSystemError(NULL, msg.lastErrno,
>                               _("cannot send monitor command '%s'"), cmd);
> +        VIR_FREE(*reply);
> +    }
> 
>      return ret;
>  }

ACK, that is good - I definitely intended that  *reply was NULL upon
error return.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|