[v1] Memory leak fixes

[PATCH for-5.0 3/3] object-add: don't create return value if failed

Posted by Marc-André Lureau 5 years, 10 months ago

If object-add failed, no need to create a return value that may later
be leaked.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qom/qom-qmp-cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 435193b036..6bd137ccbf 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -287,8 +287,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
     visit_free(v);
     if (obj) {
         object_unref(obj);
+        *ret_data = QOBJECT(qdict_new());
     }
-    *ret_data = QOBJECT(qdict_new());
 }
 
 void qmp_object_del(const char *id, Error **errp)
-- 
2.26.0.rc2.42.g98cedd0233

Re: [PATCH for-5.0 3/3] object-add: don't create return value if failed

Posted by Philippe Mathieu-Daudé 5 years, 10 months ago

On 3/25/20 7:47 PM, Marc-André Lureau wrote:
> If object-add failed, no need to create a return value that may later
> be leaked.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   qom/qom-qmp-cmds.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 435193b036..6bd137ccbf 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -287,8 +287,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
>       visit_free(v);
>       if (obj) {
>           object_unref(obj);
> +        *ret_data = QOBJECT(qdict_new());
>       }
> -    *ret_data = QOBJECT(qdict_new());
>   }
>   
>   void qmp_object_del(const char *id, Error **errp)
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH for-5.0 3/3] object-add: don't create return value if failed

Posted by Paolo Bonzini 5 years, 10 months ago

On 25/03/20 19:47, Marc-André Lureau wrote:
> If object-add failed, no need to create a return value that may later
> be leaked.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qom/qom-qmp-cmds.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 435193b036..6bd137ccbf 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -287,8 +287,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
>      visit_free(v);
>      if (obj) {
>          object_unref(obj);
> +        *ret_data = QOBJECT(qdict_new());
>      }
> -    *ret_data = QOBJECT(qdict_new());
>  }
>  
>  void qmp_object_del(const char *id, Error **errp)
> 

It can be slightly simplified:

------------------- 8< ----------------------
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: [PATCH] object-add: don't create return value if failed
    
No need to return an empty value from object-add (it would also leak
if the command failed).  While at it, remove the "if" around object_unref
since object_unref handles NULL arguments just fine.
    
Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 435193b036..e47ebe8ed1 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -285,10 +285,7 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
     v = qobject_input_visitor_new(QOBJECT(qdict));
     obj = user_creatable_add_type(type, id, qdict, v, errp);
     visit_free(v);
-    if (obj) {
-        object_unref(obj);
-    }
-    *ret_data = QOBJECT(qdict_new());
+    object_unref(obj);
 }
 
 void qmp_object_del(const char *id, Error **errp)


I queued this patch and your other two.  Thanks,

Paolo

Re: [PATCH for-5.0 3/3] object-add: don't create return value if failed

Posted by Markus Armbruster 5 years, 10 months ago

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 25/03/20 19:47, Marc-André Lureau wrote:
>> If object-add failed, no need to create a return value that may later
>> be leaked.
>> 
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  qom/qom-qmp-cmds.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
>> index 435193b036..6bd137ccbf 100644
>> --- a/qom/qom-qmp-cmds.c
>> +++ b/qom/qom-qmp-cmds.c
>> @@ -287,8 +287,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
>>      visit_free(v);
>>      if (obj) {
>>          object_unref(obj);
>> +        *ret_data = QOBJECT(qdict_new());
>>      }
>> -    *ret_data = QOBJECT(qdict_new());
>>  }
>>  
>>  void qmp_object_del(const char *id, Error **errp)
>> 
>
> It can be slightly simplified:
>
> ------------------- 8< ----------------------
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH] object-add: don't create return value if failed
>     
> No need to return an empty value from object-add (it would also leak
> if the command failed).  While at it, remove the "if" around object_unref
> since object_unref handles NULL arguments just fine.
>     
> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 435193b036..e47ebe8ed1 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -285,10 +285,7 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
>      v = qobject_input_visitor_new(QOBJECT(qdict));
>      obj = user_creatable_add_type(type, id, qdict, v, errp);
>      visit_free(v);
> -    if (obj) {
> -        object_unref(obj);
> -    }
> -    *ret_data = QOBJECT(qdict_new());
> +    object_unref(obj);
>  }
>  
>  void qmp_object_del(const char *id, Error **errp)

Yes, that's better.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

> I queued this patch and your other two.  Thanks,
>
> Paolo