qom/qom-qmp-cmds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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
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
>-----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 >
"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.
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 :|
© 2016 - 2024 Red Hat, Inc.