| Message ID | 20200406164207.1446817-1-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-5.0] xen-block: Fix uninitialized variable | expand |
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@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>
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);
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(-)