[Qemu-devel] [PATCH v2 2/3] qdev: add cleanup logic in device_set_realized() to avoid resource leak

[arei.gonglei]

From: Gonglei <address@hidden>

At present, this function doesn't have partial cleanup implemented,
which will cause resource leak in some scenarios.

Example:

1. Assuming that "dc->realize(dev, &local_err)" execute successful
   and local_err == NULL;
2. Executing device hotplug in hotplug_handler_plug(), but failed
  (It is prone to occur). Then local_err != NULL;
3. error_propagate(errp, local_err) and return. But the resources
 which been allocated in dc->realize() will be leaked.
 Simple backtrace:
  dc->realize()
   |->device_realize
            |->pci_qdev_init()
                |->do_pci_register_device()
                |->etc.

Adding fuller cleanup logic which assure that function can
goto appropriate error label as local_err population is
detected as each relevant point.

Signed-off-by: Gonglei <address@hidden>
---
 hw/core/qdev.c | 58 ++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4a1ac5b..c1a36f0 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -834,12 +834,14 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
             dc->realize(dev, &local_err);
         }
 
-        if (dev->parent_bus && dev->parent_bus->hotplug_handler &&
-            local_err == NULL) {
+        if (local_err != NULL) {
+            goto fail;
+        }
+
+        if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
             hotplug_handler_plug(dev->parent_bus->hotplug_handler,
                                  dev, &local_err);
-        } else if (local_err == NULL &&
-                   object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
+        } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
             HotplugHandler *hotplug_ctrl;
             MachineState *machine = MACHINE(qdev_get_machine());
             MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -852,21 +854,25 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
             }
         }
 
-        if (qdev_get_vmsd(dev) && local_err == NULL) {
+        if (local_err != NULL) {
+            goto post_realize_fail;
+        }
+
+        if (qdev_get_vmsd(dev)) {
             vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
                                            dev->instance_id_alias,
                                            dev->alias_required_for_version);
         }
-        if (local_err == NULL) {
-            QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-                object_property_set_bool(OBJECT(bus), true, "realized",
-                                         &local_err);
-                if (local_err != NULL) {
-                    break;
-                }
+
+        QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+            object_property_set_bool(OBJECT(bus), true, "realized",
+                                     &local_err);
+            if (local_err != NULL) {
+                goto set_bool_fail;
             }
         }
-        if (dev->hotplugged && local_err == NULL) {
+
+        if (dev->hotplugged) {
             device_reset(dev);
         }
         dev->pending_deleted_event = false;
@@ -875,24 +881,36 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
             object_property_set_bool(OBJECT(bus), false, "realized",
                                      &local_err);
             if (local_err != NULL) {
-                break;
+                goto fail;
             }
         }
-        if (qdev_get_vmsd(dev) && local_err == NULL) {
+        if (qdev_get_vmsd(dev)) {
             vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
         }
-        if (dc->unrealize && local_err == NULL) {
+        if (dc->unrealize) {
             dc->unrealize(dev, &local_err);
         }
         dev->pending_deleted_event = true;
-    }
 
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
+        if (local_err != NULL) {
+            goto fail;
+        }
     }
 
     dev->realized = value;
+    return;
+
+set_bool_fail:
+    if (qdev_get_vmsd(dev)) {
+        vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
+    }
+post_realize_fail:
+    if (dc->unrealize) {
+        dc->unrealize(dev, NULL);
+    }
+fail:
+    error_propagate(errp, local_err);
+    return;
 }
 
 static bool device_get_hotpluggable(Object *obj, Error **errp)
-- 
1.7.12.4