[libvirt] [PATCHv2 9/3] qemu: avoid memory leaks
Laine Stump
laine at laine.org
Tue Aug 2 22:32:10 UTC 2011
On 08/02/2011 05:46 PM, Eric Blake wrote:
> Quite a few leaks detected by coverity. For chr, the leaks were
> close enough to the allocations to plug in place; for disk, the
> leaks were separated from the allocation by enough other lines with
> intermediate failure cases that I refactored the cleanup instead.
Sorry, two more problems I didn't notice before:
1) instead of VIR_FREE(disk) down at the bottom, you should be calling
virDomainDiskDefFree()
2) There are some places in that qemuParseCommandLine where
virDomainDiskDefFree() is called right before goto no_memory (or goto
error), but since virDomainDiskDefFree() doesn't/can't set disk to NULL,
you'll end up with a double free.
> * src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks.
> ---
>
> v2: address review comments from v1
>
> src/qemu/qemu_command.c | 27 +++++++++++++++++----------
> 1 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6a2e2ae..194f3ff 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5938,6 +5938,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> int video = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
> int nvirtiodisk = 0;
> qemuDomainCmdlineDefPtr cmd = NULL;
> + virDomainDiskDefPtr disk = NULL;
>
> if (pidfile)
> *pidfile = NULL;
> @@ -6124,7 +6125,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> STRPREFIX(arg, "-fd") ||
> STREQ(arg, "-cdrom")) {
> WANT_VALUE();
> - virDomainDiskDefPtr disk;
> if (VIR_ALLOC(disk)< 0)
> goto no_memory;
>
> @@ -6239,6 +6239,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> goto no_memory;
> }
> def->disks[def->ndisks++] = disk;
> + disk = NULL;
> } else if (STREQ(arg, "-no-acpi")) {
> def->features&= ~(1<< VIR_DOMAIN_FEATURE_ACPI);
> } else if (STREQ(arg, "-no-reboot")) {
> @@ -6307,8 +6308,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> if (!(chr = virDomainChrDefNew()))
> goto error;
>
> - if (qemuParseCommandLineChr(&chr->source, val)< 0)
> + if (qemuParseCommandLineChr(&chr->source, val)< 0) {
> + virDomainChrDefFree(chr);
> goto error;
> + }
> if (VIR_REALLOC_N(def->serials, def->nserials+1)< 0) {
> virDomainChrDefFree(chr);
> goto no_memory;
> @@ -6325,8 +6328,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> if (!(chr = virDomainChrDefNew()))
> goto error;
>
> - if (qemuParseCommandLineChr(&chr->source, val)< 0)
> + if (qemuParseCommandLineChr(&chr->source, val)< 0) {
> + virDomainChrDefFree(chr);
> goto error;
> + }
> if (VIR_REALLOC_N(def->parallels, def->nparallels+1)< 0) {
> virDomainChrDefFree(chr);
> goto no_memory;
> @@ -6353,7 +6358,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> }
> def->inputs[def->ninputs++] = input;
> } else if (STRPREFIX(val, "disk:")) {
> - virDomainDiskDefPtr disk;
> if (VIR_ALLOC(disk)< 0)
> goto no_memory;
> disk->src = strdup(val + strlen("disk:"));
> @@ -6376,6 +6380,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> goto no_memory;
> }
> def->disks[def->ndisks++] = disk;
> + disk = NULL;
> } else {
> virDomainHostdevDefPtr hostdev;
> if (!(hostdev = qemuParseCommandLineUSB(val)))
> @@ -6399,7 +6404,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> def->nets[def->nnets++] = net;
> }
> } else if (STREQ(arg, "-drive")) {
> - virDomainDiskDefPtr disk;
> WANT_VALUE();
> if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk)))
> goto error;
> @@ -6408,6 +6412,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> goto no_memory;
> }
> def->disks[def->ndisks++] = disk;
> + disk = NULL;
>
> if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
> nvirtiodisk++;
> @@ -6516,8 +6521,10 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> if (VIR_ALLOC(chr)< 0)
> goto no_memory;
>
> - if (qemuParseCommandLineChr(chr, val)< 0)
> + if (qemuParseCommandLineChr(chr, val)< 0) {
> + virDomainChrSourceDefFree(chr);
> goto error;
> + }
>
> *monConfig = chr;
> }
> @@ -6545,10 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> char *hosts, *port, *saveptr = NULL, *token;
> virDomainDiskDefPtr first_rbd_disk = NULL;
> for (i = 0 ; i< def->ndisks ; i++) {
> - virDomainDiskDefPtr disk = def->disks[i];
> - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&&
> - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
> - first_rbd_disk = disk;
> + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_NETWORK&&
> + def->disks[i]->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) {
> + first_rbd_disk = def->disks[i];
> break;
> }
> }
> @@ -6676,6 +6682,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> no_memory:
> virReportOOMError();
> error:
> + VIR_FREE(disk);
> VIR_FREE(cmd);
> virDomainDefFree(def);
> VIR_FREE(nics);
More information about the libvir-list
mailing list