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@xxxxxxxxxx>
> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list