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

[Markus Armbruster]

"Chenqun (kuhn)" <address@hidden> writes:

>>-----Original Message-----
>>From: Qemu-devel [mailto:qemu-devel-
>>bounces+kuhn.chenqun=address@hidden] On Behalf Of Markus
>>Armbruster
>>Sent: Tuesday, March 17, 2020 5:23 PM
>>To: address@hidden
>>Cc: Kevin Wolf <address@hidden>; address@hidden;
>>address@hidden; address@hidden
>>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 <address@hidden>
>>Signed-off-by: Markus Armbruster <address@hidden>
>>---
> 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.