On 10/21/2016 09:01 AM, Markus Armbruster wrote: > Pino Toscano <[email protected]> writes: > >> qmp_output_start_struct() and qmp_output_start_list() create a new >> QObject (QDict, QList) and push it to the stack of the QmpOutputVisitor, >> where it is saved as 'value'. When freeing the iterator in >> qmp_output_free(), these values are never freed properly. >> >> The simple solution is to qobject_decref() them. >> >> Signed-off-by: Pino Toscano <[email protected]> >> --- >> >> Changes in v2: >> - added Signed-off-by >> >> qapi/qmp-output-visitor.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c >> index 9e3b67c..eedf256 100644 >> --- a/qapi/qmp-output-visitor.c >> +++ b/qapi/qmp-output-visitor.c >> @@ -220,6 +220,7 @@ static void qmp_output_free(Visitor *v) >> while (!QSLIST_EMPTY(&qov->stack)) { >> e = QSLIST_FIRST(&qov->stack); >> QSLIST_REMOVE_HEAD(&qov->stack, node); >> + qobject_decref(e->value); >> g_free(e); >> } > > Hmm. The patch looks correct, even though it adds a decref very similar > to the one deleted by commit f24582d "qapi: fix double free in > qmp_output_visitor_cleanup()".
As of that commit, we indeed had a QObject being added to both the stack (qmp_output_push) and to the parent container (qmp_output_add) at the same time, where freeing the parent container is recursive (decref on the root), and therefore we don't have to worry about the stack. > I suspect the bug you fix was introduced > by commit 455ba08 "qmp: Don't abuse stack to track qmp-output root". > Eric? No, I don't see how that changed anything. It moved where the root object was stored, but we still have the scenario where each newly-created QObject is being stored in two places (the stack, and the parent container), and where recursively freeing the parent container should be good enough. > > Should this go into -stable? I'm still not convinced this patch makes sense. I'm now trying to reproduce the problem under valgrind, to see if this is an actual bug here, or if it is a bug in some other part of the code that is not properly cleaning up after a visit. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
