| Message ID | 1476782267-2602-1-git-send-email-ptoscano@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/18/2016 04:17 AM, Pino Toscano wrote: > 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. Do any of the tests (perhaps run under valgrind) show this leak? If not, maybe we should enhance their coverage. > > The simple solution is to qobject_decref() them. > --- > qapi/qmp-output-visitor.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Eric Blake <eblake@redhat.com> > > 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); > } > >
On 10/18/2016 06:13 AM, Eric Blake wrote: > On 10/18/2016 04:17 AM, Pino Toscano wrote: >> 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. > > Do any of the tests (perhaps run under valgrind) show this leak? If not, > maybe we should enhance their coverage. > >> >> The simple solution is to qobject_decref() them. >> --- >> qapi/qmp-output-visitor.c | 1 + >> 1 file changed, 1 insertion(+) > > Reviewed-by: Eric Blake <eblake@redhat.com> except this should be done against v2, where you had S-o-B :) > >> >> 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); >> } >> >> >
On Tuesday, 18 October 2016 06:13:30 CEST Eric Blake wrote: > On 10/18/2016 04:17 AM, Pino Toscano wrote: > > 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. > > Do any of the tests (perhaps run under valgrind) show this leak? If not, > maybe we should enhance their coverage. Running a simple `qemu-img info file.qcow2` under valgrind was enough for me to show the leak. In this case, another simple fix is needed to fully fix the leak: http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg04023.html (Yes, I just saw your ACK on this, Eric, just leaving it here for reference.) > > > > The simple solution is to qobject_decref() them. > > --- > > qapi/qmp-output-visitor.c | 1 + > > 1 file changed, 1 insertion(+) > > Reviewed-by: Eric Blake <eblake@redhat.com> > > > > > 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); > > } > > > > > >
On 10/18/2016 06:22 AM, Pino Toscano wrote: > On Tuesday, 18 October 2016 06:13:30 CEST Eric Blake wrote: >> On 10/18/2016 04:17 AM, Pino Toscano wrote: >>> 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. >> >> Do any of the tests (perhaps run under valgrind) show this leak? If not, >> maybe we should enhance their coverage. > > Running a simple `qemu-img info file.qcow2` under valgrind was enough > for me to show the leak. I'm still not reproducing it. :( > > In this case, another simple fix is needed to fully fix the leak: > http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg04023.html In fact, isn't that fix alone enough to fix the leak? The more I think about this patch (and the thread on v2), the more I think it is too prone to double-freeing things. >>> +++ 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); >>> } >>> >>> >> >> > >
On Friday, 21 October 2016 16:20:30 CEST Eric Blake wrote: > On 10/18/2016 06:22 AM, Pino Toscano wrote: > > On Tuesday, 18 October 2016 06:13:30 CEST Eric Blake wrote: > >> On 10/18/2016 04:17 AM, Pino Toscano wrote: > >>> 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. > >> > >> Do any of the tests (perhaps run under valgrind) show this leak? If not, > >> maybe we should enhance their coverage. > > > > Running a simple `qemu-img info file.qcow2` under valgrind was enough > > for me to show the leak. > > I'm still not reproducing it. :( Funny, now I cannot either, not even by resetting master back to the day when I did the patches. And I'm pretty sure that it was an issue, since doing only this patch did not fully fix the leak: valgrind runs were fine, so a full run of the libguestfs test suite with the rebuild qemu as hypervisor. > > In this case, another simple fix is needed to fully fix the leak: > > http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg04023.html > > In fact, isn't that fix alone enough to fix the leak? The more I think > about this patch (and the thread on v2), the more I think it is too > prone to double-freeing things. I agree on the "this is not needed part", so let's just drop this patch. Thanks,
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); }