Re: [PATCH] qemu: avoid memory leaks

[Laine Stump <laine@xxxxxxxxx>]

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&&
>                       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).


>               }
> @@ -6676,6 +6684,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
>   no_memory:
>       virReportOOMError();
>   error:
> +    VIR_FREE(disk);
>       VIR_FREE(cmd);
>       virDomainDefFree(def);
>       VIR_FREE(nics);

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list