| Message ID | 1476787052-27333-1-git-send-email-ptoscano@redhat.com |
|---|---|
| State | New |
| Headers | show |
Pino Toscano <ptoscano@redhat.com> 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 <ptoscano@redhat.com> > --- > > 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()". I suspect the bug you fix was introduced by commit 455ba08 "qmp: Don't abuse stack to track qmp-output root". Eric? Should this go into -stable?
On 10/21/2016 09:01 AM, Markus Armbruster wrote: > Pino Toscano <ptoscano@redhat.com> 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 <ptoscano@redhat.com> >> --- >> >> 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.
On 10/21/2016 10:39 AM, Eric Blake wrote: > On 10/21/2016 09:01 AM, Markus Armbruster wrote: >> Pino Toscano <ptoscano@redhat.com> 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 <ptoscano@redhat.com> >> >> 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()". > In fact, applying this patch regresses to the very state that f24582d tried to prevent. However, I'm unable to see a difference in valgrind on tests/test-qmp-output-visitor either with or without this patch, which sadly means our testsuite is not actually testing this scenario. >> Should this go into -stable? > > I'm still not convinced this patch makes sense. NACK. As mentioned in the v1 thread, the leak that Pino was seeing is fixed by http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg04023.html I don't think we don't want this patch.
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); }
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 <ptoscano@redhat.com> --- Changes in v2: - added Signed-off-by qapi/qmp-output-visitor.c | 1 + 1 file changed, 1 insertion(+)