Re: [PATCH] qemu: avoid memory leaks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]