Re: [Qemu-devel] [PATCH 4/8] qdev: fix object reference leak in case device.realize() fails

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2598 bytes --]

On Thu, Jul 21, 2016 at 05:54:35PM +0200, Igor Mammedov wrote:
> If device doesn't have parent assined before its realize
> is called, device_set_realized() will implicitly set parent
> to '/machine/unattached'.
> 
> However device_set_realized() may fail after that point at
> several other points leaving not realized object dangling
> in '/machine/unattached' and as result caller of
> 
>   obj = object_new()
>     obj->ref == 1
>   object_property_set_bool(obj,..., true, "realized",...)
>     obj->ref == 2
>   if (fail)
>       object_unref(obj);
>       obj->ref == 1
> 
> will get object leak instead of expected object destruction.
> 
> Fix it by making device_set_realized() to cleanup after itself
> in case of failure.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/core/qdev.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6680089..ee4a083 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -885,6 +885,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>      HotplugHandler *hotplug_ctrl;
>      BusState *bus;
>      Error *local_err = NULL;
> +    bool unattached_parent = false;
> +    static int unattached_count;
>  
>      if (dev->hotplugged && !dc->hotpluggable) {
>          error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> @@ -893,12 +895,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>  
>      if (value && !dev->realized) {
>          if (!obj->parent) {
> -            static int unattached_count;
>              gchar *name = g_strdup_printf("device[%d]", unattached_count++);
>  
>              object_property_add_child(container_get(qdev_get_machine(),
>                                                      "/unattached"),
>                                        name, obj, &error_abort);
> +            unattached_parent = true;
>              g_free(name);
>          }
>  
> @@ -987,6 +989,10 @@ post_realize_fail:
>  
>  fail:
>      error_propagate(errp, local_err);
> +    if (unattached_parent) {
> +        object_unparent(OBJECT(dev));
> +        unattached_count--;
> +    }
>  }
>  
>  static bool device_get_hotpluggable(Object *obj, Error **errp)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]