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

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;
+        return NULL;
-    virDomainDiskRemove(def, i);
+    return virDomainDiskRemove(def, i);
-    return 0;
 }
 
 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;
+        return NULL;
-    virDomainNetRemove(def, i);
+    return virDomainNetRemove(def, i);
-    return 0;
 }
 
 
@@ -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
-                         virDomainLeaseDefPtr lease)
+virDomainLeaseRemove(virDomainDefPtr def,
+                     virDomainLeaseDefPtr lease)
 {
     int i = virDomainLeaseIndex(def, lease);
     if (i < 0)
-        return -1;
+        return NULL;
-    virDomainLeaseRemoveAt(def, i);
+    return virDomainLeaseRemoveAt(def, i);
-    return 0;
 }
 
 

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);
+virDomainDiskDefPtr
-int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
+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);
+virDomainNetDefPtr
-int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac);
+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);
+virDomainLeaseDefPtr
-int virDomainLeaseRemove(virDomainDefPtr def,
+virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i);
-                         virDomainLeaseDefPtr lease);
+virDomainLeaseDefPtr
+virDomainLeaseRemove(virDomainDefPtr def,
+                     virDomainLeaseDefPtr lease);
 
 int virDomainSaveXML(const char *configDir,
                      virDomainDefPtr def,

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:

src/qemu/qemu_driver.c

@@ -5436,23 +5436,24 @@ static int
 qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
                              virDomainDeviceDefPtr dev)
 {
-    virDomainDiskDefPtr disk;
+    virDomainDiskDefPtr disk, det_disk;
-    virDomainNetDefPtr net;
+    virDomainNetDefPtr net, det_net;
-    virDomainLeaseDefPtr lease;
+    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:

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;
 }