Patchwork qom: In function object_set_link_property(), first call object_ref(), then object_unref().

login
register
mail settings
Submitter Alexander Barabash
Date Feb. 22, 2012, 5:22 p.m.
Message ID <1329931346-28207-1-git-send-email-alexander_barabash@mentor.com>
Download mbox | patch
Permalink /patch/142512/
State New
Headers show

Comments

Alexander Barabash - Feb. 22, 2012, 5:22 p.m.
From: Alexander Barabash <alexander_barabash@mentor.com>

In the old implementation, if the new value of the property links
to the same object, as the old value, that object is first unref-ed,
and then ref-ed. This leads to unintended deinitialization of that object.

In the new implementation, this is fixed.

Signed-off-by: Alexander Barabash <alexander_barabash@mentor.com>
---
 qom/object.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)
Paolo Bonzini - Feb. 22, 2012, 5:24 p.m.
On 02/22/2012 06:22 PM, alexander_barabash@mentor.com wrote:
> From: Alexander Barabash <alexander_barabash@mentor.com>
> 
> In the old implementation, if the new value of the property links
> to the same object, as the old value, that object is first unref-ed,
> and then ref-ed. This leads to unintended deinitialization of that object.
> 
> In the new implementation, this is fixed.
> 
> Signed-off-by: Alexander Barabash <alexander_barabash@mentor.com>
> ---
>  qom/object.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 941c291..e6591e1 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -892,6 +892,7 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>                                       const char *name, Error **errp)
>  {
>      Object **child = opaque;
> +    Object *old_target;
>      bool ambiguous = false;
>      const char *type;
>      char *path;
> @@ -901,10 +902,8 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>  
>      visit_type_str(v, &path, name, errp);
>  
> -    if (*child) {
> -        object_unref(*child);
> -        *child = NULL;
> -    }
> +    old_target = *child;
> +    *child = NULL;
>  
>      if (strcmp(path, "") != 0) {
>          Object *target;
> @@ -930,6 +929,10 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>      }
>  
>      g_free(path);
> +
> +    if (old_target != NULL) {
> +        object_unref(old_target);
> +    }
>  }
>  
>  void object_property_add_link(Object *obj, const char *name,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Alexander Barabash - Feb. 22, 2012, 5:25 p.m.
On 02/22/2012 07:24 PM, Paolo Bonzini wrote:
> On 02/22/2012 06:22 PM, alexander_barabash@mentor.com wrote:
>> From: Alexander Barabash<alexander_barabash@mentor.com>
>>
>> In the old implementation, if the new value of the property links
>> to the same object, as the old value, that object is first unref-ed,
>> and then ref-ed. This leads to unintended deinitialization of that object.
>>
>> In the new implementation, this is fixed.
>>
>> Signed-off-by: Alexander Barabash<alexander_barabash@mentor.com>
>> ---
>>   qom/object.c |   11 +++++++----
>>   1 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 941c291..e6591e1 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -892,6 +892,7 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>>                                        const char *name, Error **errp)
>>   {
>>       Object **child = opaque;
>> +    Object *old_target;
>>       bool ambiguous = false;
>>       const char *type;
>>       char *path;
>> @@ -901,10 +902,8 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>>
>>       visit_type_str(v,&path, name, errp);
>>
>> -    if (*child) {
>> -        object_unref(*child);
>> -        *child = NULL;
>> -    }
>> +    old_target = *child;
>> +    *child = NULL;
>>
>>       if (strcmp(path, "") != 0) {
>>           Object *target;
>> @@ -930,6 +929,10 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>>       }
>>
>>       g_free(path);
>> +
>> +    if (old_target != NULL) {
>> +        object_unref(old_target);
>> +    }
>>   }
>>
>>   void object_property_add_link(Object *obj, const char *name,
> Reviewed-by: Paolo Bonzini<pbonzini@redhat.com>
Sorry, I did not figure out yet how to add the "v2" to the subject line. ))
Alex
Eric Blake - Feb. 22, 2012, 5:52 p.m.
On 02/22/2012 10:25 AM, Alexander Barabash wrote:

> Sorry, I did not figure out yet how to add the "v2" to the subject line. ))

git send-email --subject-prefix=PATCHv2

or

git send-email --annotate, then in the editor that pops up, add v2 in
the appropriate place

there's probably other possibilities as well.
Anthony Liguori - Feb. 24, 2012, 3:32 p.m.
On 02/22/2012 11:22 AM, alexander_barabash@mentor.com wrote:
> From: Alexander Barabash<alexander_barabash@mentor.com>
>
> In the old implementation, if the new value of the property links
> to the same object, as the old value, that object is first unref-ed,
> and then ref-ed. This leads to unintended deinitialization of that object.
>
> In the new implementation, this is fixed.
>
> Signed-off-by: Alexander Barabash<alexander_barabash@mentor.com>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>   qom/object.c |   11 +++++++----
>   1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 941c291..e6591e1 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -892,6 +892,7 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>                                        const char *name, Error **errp)
>   {
>       Object **child = opaque;
> +    Object *old_target;
>       bool ambiguous = false;
>       const char *type;
>       char *path;
> @@ -901,10 +902,8 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>
>       visit_type_str(v,&path, name, errp);
>
> -    if (*child) {
> -        object_unref(*child);
> -        *child = NULL;
> -    }
> +    old_target = *child;
> +    *child = NULL;
>
>       if (strcmp(path, "") != 0) {
>           Object *target;
> @@ -930,6 +929,10 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>       }
>
>       g_free(path);
> +
> +    if (old_target != NULL) {
> +        object_unref(old_target);
> +    }
>   }
>
>   void object_property_add_link(Object *obj, const char *name,

Patch

diff --git a/qom/object.c b/qom/object.c
index 941c291..e6591e1 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -892,6 +892,7 @@  static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
                                      const char *name, Error **errp)
 {
     Object **child = opaque;
+    Object *old_target;
     bool ambiguous = false;
     const char *type;
     char *path;
@@ -901,10 +902,8 @@  static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
 
     visit_type_str(v, &path, name, errp);
 
-    if (*child) {
-        object_unref(*child);
-        *child = NULL;
-    }
+    old_target = *child;
+    *child = NULL;
 
     if (strcmp(path, "") != 0) {
         Object *target;
@@ -930,6 +929,10 @@  static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
     }
 
     g_free(path);
+
+    if (old_target != NULL) {
+        object_unref(old_target);
+    }
 }
 
 void object_property_add_link(Object *obj, const char *name,