Since 7f5d9b206d1e ("object-add: don't create return value if
failed"), qmp_object_add() don't write any value in 'ret_data', thus
has random data. Then qobject_unref() fails and abort().

Fix by initialising 'ret_data' properly.

Fixes: 5f07c4d60d09 ("qapi: Flatten object-add")
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/block/xen-block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 07bb32e22b51..99cb4c67cb09 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);
 
-- 
Anthony PERARD

On 4/6/20 6:42 PM, Anthony PERARD wrote:
> Since 7f5d9b206d1e ("object-add: don't create return value if
> failed"), qmp_object_add() don't write any value in 'ret_data', thus
> has random data. Then qobject_unref() fails and abort().
> 
> Fix by initialising 'ret_data' properly.

Or move qobject_unref() after the error check?

-- >8 --
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 07bb32e22b..f3f1cbef65 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -869,7 +869,6 @@ static XenBlockIOThread 
*xen_block_iothread_create(const char *id,
      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);
@@ -878,6 +877,7 @@ static XenBlockIOThread 
*xen_block_iothread_create(const char *id,
          g_free(iothread);
          return NULL;
      }
+    qobject_unref(ret_data);

      return iothread;
  }
---

> 
> Fixes: 5f07c4d60d09 ("qapi: Flatten object-add")
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>   hw/block/xen-block.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 07bb32e22b51..99cb4c67cb09 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);
>   
> 

On Mon, Apr 06, 2020 at 06:50:41PM +0200, Philippe Mathieu-Daudé wrote:
> On 4/6/20 6:42 PM, Anthony PERARD wrote:
> > Since 7f5d9b206d1e ("object-add: don't create return value if
> > failed"), qmp_object_add() don't write any value in 'ret_data', thus
> > has random data. Then qobject_unref() fails and abort().
> > 
> > Fix by initialising 'ret_data' properly.
> 
> Or move qobject_unref() after the error check?
> 
> -- >8 --
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 07bb32e22b..f3f1cbef65 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -869,7 +869,6 @@ static XenBlockIOThread *xen_block_iothread_create(const
> char *id,
>      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);
> @@ -878,6 +877,7 @@ static XenBlockIOThread *xen_block_iothread_create(const
> char *id,
>          g_free(iothread);
>          return NULL;
>      }
> +    qobject_unref(ret_data);

That won't help, qmp_object_add() doesn't change the value of ret_data
at all. The other users of qmp_object_add() passes an initialised
'ret_data', so we should do the same I think.

Thanks,

-- 
Anthony PERARD

Anthony PERARD <anthony.perard@citrix.com> writes:

> On Mon, Apr 06, 2020 at 06:50:41PM +0200, Philippe Mathieu-Daudé wrote:
>> On 4/6/20 6:42 PM, Anthony PERARD wrote:
>> > Since 7f5d9b206d1e ("object-add: don't create return value if
>> > failed"), qmp_object_add() don't write any value in 'ret_data', thus
>> > has random data. Then qobject_unref() fails and abort().
>> > 
>> > Fix by initialising 'ret_data' properly.
>> 
>> Or move qobject_unref() after the error check?
>> 
>> -- >8 --
>> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
>> index 07bb32e22b..f3f1cbef65 100644
>> --- a/hw/block/xen-block.c
>> +++ b/hw/block/xen-block.c
>> @@ -869,7 +869,6 @@ static XenBlockIOThread *xen_block_iothread_create(const
>> char *id,
>>      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);
>> @@ -878,6 +877,7 @@ static XenBlockIOThread *xen_block_iothread_create(const
>> char *id,
>>          g_free(iothread);
>>          return NULL;
>>      }
>> +    qobject_unref(ret_data);
>
> That won't help, qmp_object_add() doesn't change the value of ret_data
> at all. The other users of qmp_object_add() passes an initialised
> 'ret_data', so we should do the same I think.

Since the QMP core does it, other callers should do it, too.

For QAPI commands that don't return anything, the marshaller should not
use @ret_data, let alone store through it.  qmp_object_add() complies.
Thus, assert(!ret_data) would do.  qobject_unref(ret_data) is also
correct.

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