[libvirt] [PATCH] qemu: avoid memory leaks
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. * src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks. --- src/qemu/qemu_command.c | 23 --- 1 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6a2e2ae..c638420 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) { +VIR_FREE(chr); goto error; +} *monConfig = chr; } @@ -6545,10 +6552,11 @@ 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]; +disk = def->disks[i]; if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { first_rbd_disk = disk; +disk = NULL;
Re: [libvirt] [PATCH] qemu: avoid memory leaks
On 08/02/2011 04:10 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. * src/qemu/qemu_command.c (qemuParseCommandLine): Plug leaks. --- src/qemu/qemu_command.c | 23 --- 1 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6a2e2ae..c638420 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) { +VIR_FREE(chr); Shouldn't this be virDomainChrDefFree(chr)? goto error; +} *monConfig = chr; } @@ -6545,10 +6552,11 @@ 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]; +disk = def->disks[i]; if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&&
Re: [libvirt] [PATCH] qemu: avoid memory leaks
On 08/02/2011 02:48 PM, Laine Stump wrote: On 08/02/2011 04:10 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. - if (qemuParseCommandLineChr(chr, val)< 0) + if (qemuParseCommandLineChr(chr, val)< 0) { + VIR_FREE(chr); Shouldn't this be virDomainChrDefFree(chr)? virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i< def->ndisks ; i++) { - virDomainDiskDefPtr disk = def->disks[i]; + disk = def->disks[i]; if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&& disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { first_rbd_disk = disk; + disk = NULL; break; } If you never hit the if condition, disk will be left pointing to one of the disks in def, and will be freed during cleanup. I think you want to set disk = NULL after this look is finished as well, don't you? (Either that, or just use a different variable). Both good points. I'm squashing this in to create v2. diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index c638420..194f3ff 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -6522,7 +6522,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; if (qemuParseCommandLineChr(chr, val) < 0) { -VIR_FREE(chr); +virDomainChrSourceDefFree(chr); goto error; } @@ -6552,11 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, char *hosts, *port, *saveptr = NULL, *token; virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i < def->ndisks ; i++) { -disk = def->disks[i]; -if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && -disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { -first_rbd_disk = disk; -disk = NULL; +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; } } -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: avoid memory leaks
On 08/02/2011 05:46 PM, Eric Blake wrote: On 08/02/2011 02:48 PM, Laine Stump wrote: On 08/02/2011 04:10 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. - if (qemuParseCommandLineChr(chr, val)< 0) + if (qemuParseCommandLineChr(chr, val)< 0) { + VIR_FREE(chr); Shouldn't this be virDomainChrDefFree(chr)? virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i< def->ndisks ; i++) { - virDomainDiskDefPtr disk = def->disks[i]; + disk = def->disks[i]; if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&& disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { first_rbd_disk = disk; + disk = NULL; break; } If you never hit the if condition, disk will be left pointing to one of the disks in def, and will be freed during cleanup. I think you want to set disk = NULL after this look is finished as well, don't you? (Either that, or just use a different variable). Both good points. I'm squashing this in to create v2. diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index c638420..194f3ff 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -6522,7 +6522,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, goto no_memory; if (qemuParseCommandLineChr(chr, val) < 0) { -VIR_FREE(chr); +virDomainChrSourceDefFree(chr); goto error; } @@ -6552,11 +6552,9 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, char *hosts, *port, *saveptr = NULL, *token; virDomainDiskDefPtr first_rbd_disk = NULL; for (i = 0 ; i < def->ndisks ; i++) { -disk = def->disks[i]; -if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && -disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { -first_rbd_disk = disk; -disk = NULL; +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; } } ACK with those changes. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Avoid memory leaks on qemuParseRBDString
From: Alex Jia Detected by valgrind. Leak introduced in commit 5745dc1. * src/qemu/qemu_command.c: fix memory leak on failure and successful path. * How to reproduce? % valgrind -v --leak-check=full ./qemuargv2xmltest * Actual result: ==2196== 80 bytes in 1 blocks are definitely lost in loss record 3 of 4 ==2196==at 0x4A05FDE: malloc (vg_replace_malloc.c:236) ==2196==by 0x39CF07F6E1: strdup (in /lib64/libc-2.12.so) ==2196==by 0x419823: qemuParseRBDString (qemu_command.c:1657) ==2196==by 0x4221ED: qemuParseCommandLine (qemu_command.c:5934) ==2196==by 0x422AFB: qemuParseCommandLineString (qemu_command.c:7561) ==2196==by 0x416864: testCompareXMLToArgvHelper (qemuargv2xmltest.c:48) ==2196==by 0x417DB1: virtTestRun (testutils.c:141) ==2196==by 0x415CAF: mymain (qemuargv2xmltest.c:175) ==2196==by 0x4174A7: virtTestMain (testutils.c:696) ==2196==by 0x39CF01ECDC: (below main) (in /lib64/libc-2.12.so) ==2196== ==2196== LEAK SUMMARY: ==2196==definitely lost: 80 bytes in 1 blocks Signed-off-by: Alex Jia --- src/qemu/qemu_command.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ea1b763..69bf868 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1709,9 +1709,11 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk) p = next; } +VIR_FREE(options); return 0; no_memory: +VIR_FREE(options); virReportOOMError(); return -1; } -- 1.7.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Avoid memory leaks on qemuParseRBDString
On 2012年01月06日 14:36, a...@redhat.com wrote: From: Alex Jia Detected by valgrind. Leak introduced in commit 5745dc1. * src/qemu/qemu_command.c: fix memory leak on failure and successful path. * How to reproduce? % valgrind -v --leak-check=full ./qemuargv2xmltest * Actual result: ==2196== 80 bytes in 1 blocks are definitely lost in loss record 3 of 4 ==2196==at 0x4A05FDE: malloc (vg_replace_malloc.c:236) ==2196==by 0x39CF07F6E1: strdup (in /lib64/libc-2.12.so) ==2196==by 0x419823: qemuParseRBDString (qemu_command.c:1657) ==2196==by 0x4221ED: qemuParseCommandLine (qemu_command.c:5934) ==2196==by 0x422AFB: qemuParseCommandLineString (qemu_command.c:7561) ==2196==by 0x416864: testCompareXMLToArgvHelper (qemuargv2xmltest.c:48) ==2196==by 0x417DB1: virtTestRun (testutils.c:141) ==2196==by 0x415CAF: mymain (qemuargv2xmltest.c:175) ==2196==by 0x4174A7: virtTestMain (testutils.c:696) ==2196==by 0x39CF01ECDC: (below main) (in /lib64/libc-2.12.so) ==2196== ==2196== LEAK SUMMARY: ==2196==definitely lost: 80 bytes in 1 blocks Signed-off-by: Alex Jia --- src/qemu/qemu_command.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ea1b763..69bf868 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1709,9 +1709,11 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk) p = next; } +VIR_FREE(options); return 0; no_memory: +VIR_FREE(options); virReportOOMError(); return -1; } Trivial, ACK and Pushed. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list