util: eliminate device object leaks related to virDomain*Remove*()

There are several functions in domain_conf.c that remove a device
object from the domain's list of that object type, but don't free the
object or return it to the caller to free. In many cases this isn't a
problem because the caller already had a pointer to the object and
frees it afterward, but in several cases the removed object was just
left floating around with no references to it.

In particular, the function qemuDomainDetachDeviceConfig() calls
functions to locate and remove net (virDomainNetRemoveByMac), disk
(virDomainDiskRemoveByName()), and lease (virDomainLeaseRemove())
devices, but neither it nor its caller qemuDomainModifyDeviceConfig()
ever obtain a pointer to the device being removed, much less free it.

This patch modifies the following "remove" functions to return a
pointer to the device object being removed from the domain device
arrays, to give the caller the option of freeing the device object
using that pointer if needed. In places where the object was
previously leaked, it is now freed:

  virDomainDiskRemove
  virDomainDiskRemoveByName
  virDomainNetRemove
  virDomainNetRemoveByMac
  virDomainHostdevRemove
  virDomainLeaseRemove
  virDomainLeaseRemoveAt

The functions that had been leaking:

  libxlDomainDetachConfig - leaked a virDomainDiskDef
  qemuDomainDetachDeviceConfig - could leak a virDomainDiskDef,
                            a virDomainNetDef, or a
                            virDomainLeaseDef
  qemuDomainDetachLease   - leaked a virDomainLeaseDef

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a5c7945d3fdd0a87d10af634f8fd0a4918ff4f12..333b10eab39d8593b5ca8c223503a6a311f2fb89 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6856,9 +6856,11 @@ virDomainHostdevInsert(virDomainDefPtr def, virDomainHostdevDefPtr hostdev)
     return 0;
 }
 
-void
+virDomainHostdevDefPtr
 virDomainHostdevRemove(virDomainDefPtr def, size_t i)
 {
+    virDomainHostdevDefPtr hostdev = def->hostdevs[i];
+
     if (def->nhostdevs > 1) {
         memmove(def->hostdevs + i,
                 def->hostdevs + i + 1,
@@ -6872,6 +6874,7 @@ virDomainHostdevRemove(virDomainDefPtr def, size_t i)
         VIR_FREE(def->hostdevs);
         def->nhostdevs = 0;
     }
+    return hostdev;
 }
 
 /* Find an entry in hostdevs that matches the source spec in
@@ -7035,8 +7038,11 @@ void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
 }
 
 
-void virDomainDiskRemove(virDomainDefPtr def, size_t i)
+virDomainDiskDefPtr
+virDomainDiskRemove(virDomainDefPtr def, size_t i)
 {
+    virDomainDiskDefPtr disk = disk = def->disks[i];
+
     if (def->ndisks > 1) {
         memmove(def->disks + i,
                 def->disks + i + 1,
@@ -7050,15 +7056,16 @@ void virDomainDiskRemove(virDomainDefPtr def, size_t i)
         VIR_FREE(def->disks);
         def->ndisks = 0;
     }
+    return disk;
 }
 
-int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name)
+virDomainDiskDefPtr
+virDomainDiskRemoveByName(virDomainDefPtr def, const char *name)
 {
     int i = virDomainDiskIndexByName(def, name, false);
     if (i < 0)
-        return -1;
-    virDomainDiskRemove(def, i);
-    return 0;
+        return NULL;
+    return virDomainDiskRemove(def, i);
 }
 
 int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net)
@@ -7084,7 +7091,8 @@ int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char *mac)
     return -1;
 }
 
-void virDomainNetRemove(virDomainDefPtr def, size_t i)
+virDomainNetDefPtr
+virDomainNetRemove(virDomainDefPtr def, size_t i)
 {
     virDomainNetDefPtr net = def->nets[i];
 
@@ -7115,16 +7123,17 @@ void virDomainNetRemove(virDomainDefPtr def, size_t i)
         VIR_FREE(def->nets);
         def->nnets = 0;
     }
+    return net;
 }
 
-int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac)
+virDomainNetDefPtr
+virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac)
 {
     int i = virDomainNetIndexByMac(def, mac);
 
     if (i < 0)
-        return -1;
-    virDomainNetRemove(def, i);
-    return 0;
+        return NULL;
+    return virDomainNetRemove(def, i);
 }
 
 
@@ -7233,8 +7242,12 @@ void virDomainLeaseInsertPreAlloced(virDomainDefPtr def,
 }
 
 
-void virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i)
+virDomainLeaseDefPtr
+virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i)
 {
+
+    virDomainLeaseDefPtr lease = def->leases[i];
+
     if (def->nleases > 1) {
         memmove(def->leases + i,
                 def->leases + i + 1,
@@ -7245,17 +7258,18 @@ void virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i)
         VIR_FREE(def->leases);
         def->nleases = 0;
     }
+    return lease;
 }
 
 
-int virDomainLeaseRemove(virDomainDefPtr def,
-                         virDomainLeaseDefPtr lease)
+virDomainLeaseDefPtr
+virDomainLeaseRemove(virDomainDefPtr def,
+                     virDomainLeaseDefPtr lease)
 {
     int i = virDomainLeaseIndex(def, lease);
     if (i < 0)
-        return -1;
-    virDomainLeaseRemoveAt(def, i);
-    return 0;
+        return NULL;
+    return virDomainLeaseRemoveAt(def, i);
 }
 
 

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a2cf78979c6ed40bb08167e3a68fc3f67aa705d3..0f40f1d72159ee4f71678c65b68c803d987f0378 100644 (file)
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1901,16 +1901,21 @@ void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
                                    virDomainDiskDefPtr disk);
 int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def);
 
-void virDomainDiskRemove(virDomainDefPtr def, size_t i);
-int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
+virDomainDiskDefPtr
+virDomainDiskRemove(virDomainDefPtr def, size_t i);
+virDomainDiskDefPtr
+virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
 
 int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char *mac);
 int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net);
-void virDomainNetRemove(virDomainDefPtr def, size_t i);
-int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac);
+virDomainNetDefPtr
+virDomainNetRemove(virDomainDefPtr def, size_t i);
+virDomainNetDefPtr
+virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac);
 
 int virDomainHostdevInsert(virDomainDefPtr def, virDomainHostdevDefPtr hostdev);
-void virDomainHostdevRemove(virDomainDefPtr def, size_t i);
+virDomainHostdevDefPtr
+virDomainHostdevRemove(virDomainDefPtr def, size_t i);
 int virDomainHostdevFind(virDomainDefPtr def, virDomainHostdevDefPtr match,
                          virDomainHostdevDefPtr *found);
 
@@ -1955,9 +1960,11 @@ int virDomainLeaseInsert(virDomainDefPtr def,
 int virDomainLeaseInsertPreAlloc(virDomainDefPtr def);
 void virDomainLeaseInsertPreAlloced(virDomainDefPtr def,
                                     virDomainLeaseDefPtr lease);
-void virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i);
-int virDomainLeaseRemove(virDomainDefPtr def,
-                         virDomainLeaseDefPtr lease);
+virDomainLeaseDefPtr
+virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i);
+virDomainLeaseDefPtr
+virDomainLeaseRemove(virDomainDefPtr def,
+                     virDomainLeaseDefPtr lease);
 
 int virDomainSaveXML(const char *configDir,
                      virDomainDefPtr def,

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index a7bb75100a96106121e7faebd48222da485d6998..177b21898162da0a1df9e558162a673f3d02caae 100644 (file)
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3089,17 +3089,18 @@ libxlDomainDetachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
 static int
 libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
 {
-    virDomainDiskDefPtr disk;
+    virDomainDiskDefPtr disk, detach;
     int ret = -1;
 
     switch (dev->type) {
         case VIR_DOMAIN_DEVICE_DISK:
             disk = dev->data.disk;
-            if (virDomainDiskRemoveByName(vmdef, disk->dst)) {
+            if (!(detach = virDomainDiskRemoveByName(vmdef, disk->dst))) {
                 libxlError(VIR_ERR_INVALID_ARG,
                             _("no target device %s"), disk->dst);
                 break;
             }
+            virDomainDiskDefFree(detach);
             ret = 0;
             break;
         default:

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d2c45446c3bb873a174018b243c2f82ed737dde9..e533267e7bec0617f34d2d1bcae27ee82265f68e 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5436,23 +5436,24 @@ static int
 qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
                              virDomainDeviceDefPtr dev)
 {
-    virDomainDiskDefPtr disk;
-    virDomainNetDefPtr net;
-    virDomainLeaseDefPtr lease;
+    virDomainDiskDefPtr disk, det_disk;
+    virDomainNetDefPtr net, det_net;
+    virDomainLeaseDefPtr lease, det_lease;
 
     switch (dev->type) {
     case VIR_DOMAIN_DEVICE_DISK:
         disk = dev->data.disk;
-        if (virDomainDiskRemoveByName(vmdef, disk->dst)) {
+        if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) {
             qemuReportError(VIR_ERR_INVALID_ARG,
                             _("no target device %s"), disk->dst);
             return -1;
         }
+        virDomainDiskDefFree(det_disk);
         break;
 
     case VIR_DOMAIN_DEVICE_NET:
         net = dev->data.net;
-        if (virDomainNetRemoveByMac(vmdef, net->mac)) {
+        if (!(det_net = virDomainNetRemoveByMac(vmdef, net->mac))) {
             char macbuf[VIR_MAC_STRING_BUFLEN];
 
             virMacAddrFormat(net->mac, macbuf);
@@ -5460,16 +5461,18 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
                             _("no nic of mac %s"), macbuf);
             return -1;
         }
+        virDomainNetDefFree(det_net);
         break;
 
     case VIR_DOMAIN_DEVICE_LEASE:
         lease = dev->data.lease;
-        if (virDomainLeaseRemove(vmdef, lease) < 0) {
+        if (!(det_lease = virDomainLeaseRemove(vmdef, lease))) {
             qemuReportError(VIR_ERR_INVALID_ARG,
                             _("Lease %s in lockspace %s does not exist"),
                             lease->key, NULLSTR(lease->lockspace));
             return -1;
         }
+        virDomainLeaseDefFree(det_lease);
         break;
 
     default:

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 22d0231a925e9752b123b24b96fe768410496a95..1e563545b6f91a10283c0eedf90792803fa6d19a 100644 (file)
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2286,6 +2286,7 @@ int qemuDomainDetachLease(struct qemud_driver *driver,
                           virDomainObjPtr vm,
                           virDomainLeaseDefPtr lease)
 {
+    virDomainLeaseDefPtr det_lease;
     int i;
 
     if ((i = virDomainLeaseIndex(vm->def, lease)) < 0) {
@@ -2298,6 +2299,7 @@ int qemuDomainDetachLease(struct qemud_driver *driver,
     if (virDomainLockLeaseDetach(driver->lockManager, vm, lease) < 0)
         return -1;
 
-    virDomainLeaseRemoveAt(vm->def, i);
+    det_lease = virDomainLeaseRemoveAt(vm->def, i);
+    virDomainLeaseDefFree(det_lease);
     return 0;
 }