[PATCH] qom-qmp-cmds: Fix another memory leak in qmp_object_add()

[PATCH] qom-qmp-cmds: Fix another memory leak in qmp_object_add()

[Markus Armbruster]

When user_creatable_add_type() fails, qmp_object_add() returns both
its error and the usual empty QDict success value.  The QMP core
handles the error, and ignores the success value, leaking it.  Exposed
by qmp-cmd-test case /x86_64/qmp/object-add-without-props, and duly
reported both by ASan and valgrind.

To plug the leak, set the success value only on success.

Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e
Cc: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Markus Armbruster <armbru@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.21.1

Re: [PATCH] qom-qmp-cmds: Fix another memory leak in qmp_object_add()

[Marc-André Lureau]

On Tue, Mar 17, 2020 at 10:23 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> When user_creatable_add_type() fails, qmp_object_add() returns both
> its error and the usual empty QDict success value.  The QMP core
> handles the error, and ignores the success value, leaking it.  Exposed
> by qmp-cmd-test case /x86_64/qmp/object-add-without-props, and duly
> reported both by ASan and valgrind.
>
> To plug the leak, set the success value only on success.
>
> Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e
> Cc: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-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.21.1
>
>


-- 
Marc-André Lureau

Re: [PATCH] qom-qmp-cmds: Fix another memory leak in qmp_object_add()

[Daniel P. Berrangé]

On Tue, Mar 17, 2020 at 10:22:41AM +0100, Markus Armbruster wrote:
> When user_creatable_add_type() fails, qmp_object_add() returns both
> its error and the usual empty QDict success value.  The QMP core
> handles the error, and ignores the success value, leaking it.  Exposed
> by qmp-cmd-test case /x86_64/qmp/object-add-without-props, and duly
> reported both by ASan and valgrind.
> 
> To plug the leak, set the success value only on success.
> 
> Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e
> Cc: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qom/qom-qmp-cmds.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> 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.21.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

RE: [PATCH] qom-qmp-cmds: Fix another memory leak in qmp_object_add()

[Chenqun (kuhn)]

>-----Original Message-----
>From: Qemu-devel [mailto:qemu-devel-
>bounces+kuhn.chenqun=huawei.com@nongnu.org] On Behalf Of Markus
>Armbruster
>Sent: Tuesday, March 17, 2020 5:23 PM
>To: qemu-devel@nongnu.org
>Cc: Kevin Wolf <kwolf@redhat.com>; pbonzini@redhat.com;
>berrange@redhat.com; ehabkost@redhat.com
>Subject: [PATCH] qom-qmp-cmds: Fix another memory leak in
>qmp_object_add()
>
>When user_creatable_add_type() fails, qmp_object_add() returns both its
>error and the usual empty QDict success value.  The QMP core handles the
>error, and ignores the success value, leaking it.  Exposed by qmp-cmd-test
>case /x86_64/qmp/object-add-without-props, and duly reported both by
>ASan and valgrind.
>
>To plug the leak, set the success value only on success.
>
>Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e
>Cc: Kevin Wolf <kwolf@redhat.com>
>Signed-off-by: Markus Armbruster <armbru@redhat.com>
>---
Hi,  Markus

        Looks like the same patch that has been reported already here:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg03928.html

Maybe we should initialize ret_data in xen-block to avoid a possible uninitialized error ？

Thanks.
> 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.21.1
>

Re: [PATCH] qom-qmp-cmds: Fix another memory leak in qmp_object_add()

[Markus Armbruster]

"Chenqun (kuhn)" <kuhn.chenqun@huawei.com> writes:

>>-----Original Message-----
>>From: Qemu-devel [mailto:qemu-devel-
>>bounces+kuhn.chenqun=huawei.com@nongnu.org] On Behalf Of Markus
>>Armbruster
>>Sent: Tuesday, March 17, 2020 5:23 PM
>>To: qemu-devel@nongnu.org
>>Cc: Kevin Wolf <kwolf@redhat.com>; pbonzini@redhat.com;
>>berrange@redhat.com; ehabkost@redhat.com
>>Subject: [PATCH] qom-qmp-cmds: Fix another memory leak in
>>qmp_object_add()
>>
>>When user_creatable_add_type() fails, qmp_object_add() returns both its
>>error and the usual empty QDict success value.  The QMP core handles the
>>error, and ignores the success value, leaking it.  Exposed by qmp-cmd-test
>>case /x86_64/qmp/object-add-without-props, and duly reported both by
>>ASan and valgrind.
>>
>>To plug the leak, set the success value only on success.
>>
>>Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e
>>Cc: Kevin Wolf <kwolf@redhat.com>
>>Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>---
> Hi,  Markus
>
>         Looks like the same patch that has been reported already here:
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg03928.html

That patch has an additional hunk:

  diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
  index 3885464513..041866b846 100644
  --- a/hw/block/xen-block.c
  +++ b/hw/block/xen-block.c
  @@ -860,7 +860,7 @@ static XenBlockIOThread *xen_block_iothread_create(const 
  char *id,
       XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
       Error *local_err = NULL;
       QDict *opts;
  -    QObject *ret_data;
  +    QObject *ret_data = NULL;

       iothread->id = g_strdup(id);

       opts = qdict_new();
       qdict_put_str(opts, "qom-type", TYPE_IOTHREAD);
       qdict_put_str(opts, "id", id);
       qmp_object_add(opts, &ret_data, &local_err);
       qobject_unref(opts);
       qobject_unref(ret_data);

       if (local_err) {
           error_propagate(errp, local_err);

           g_free(iothread->id);
           g_free(iothread);
           return NULL;
       }

       return iothread;

> Maybe we should initialize ret_data in xen-block to avoid a possible uninitialized error ？

Yes, because xen_block_iothread_create() passes @ret_data to
qobject_unref() even after qmp_object_add() failed.

In short, Pan Nengyuan's patch is more complete.