* [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor @ 2016-10-18 10:37 Pino Toscano 2016-10-21 14:01 ` Markus Armbruster 0 siblings, 1 reply; 6+ messages in thread From: Pino Toscano @ 2016-10-18 10:37 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, mdroth, ptoscano 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); } -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor 2016-10-18 10:37 [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor Pino Toscano @ 2016-10-21 14:01 ` Markus Armbruster 2016-10-21 15:39 ` Eric Blake 0 siblings, 1 reply; 6+ messages in thread From: Markus Armbruster @ 2016-10-21 14:01 UTC (permalink / raw) To: Pino Toscano; +Cc: qemu-devel, mdroth 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor 2016-10-21 14:01 ` Markus Armbruster @ 2016-10-21 15:39 ` Eric Blake 2016-10-21 21:32 ` Eric Blake 0 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2016-10-21 15:39 UTC (permalink / raw) To: Markus Armbruster, Pino Toscano; +Cc: qemu-devel, mdroth [-- Attachment #1: Type: text/plain, Size: 2372 bytes --] 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor 2016-10-21 15:39 ` Eric Blake @ 2016-10-21 21:32 ` Eric Blake 2016-10-21 22:10 ` Eric Blake 0 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2016-10-21 21:32 UTC (permalink / raw) To: Markus Armbruster, Pino Toscano; +Cc: qemu-devel, mdroth [-- Attachment #1: Type: text/plain, Size: 1416 bytes --] 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor 2016-10-21 21:32 ` Eric Blake @ 2016-10-21 22:10 ` Eric Blake 2016-10-24 6:56 ` Markus Armbruster 0 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2016-10-21 22:10 UTC (permalink / raw) To: Markus Armbruster, Pino Toscano; +Cc: qemu-devel, mdroth [-- Attachment #1: Type: text/plain, Size: 4735 bytes --] On 10/21/2016 04:32 PM, Eric Blake wrote: > 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? > > 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. And below is my hack to test-qmp-output-visitor, which shows (at least under valgrind) that we don't want the patch. I should probably turn that into a formal patch submission; but should I piggyback the existing test or add a new one? With the patch omitted, valgrind is happy; with the patch applied, I see: /visitor/output/struct-errors: ==8458== Invalid read of size 8 ==8458== at 0x175DC2: qobject_decref (qobject.h:81) ==8458== by 0x1767A5: qentry_destroy (qdict.c:413) ==8458== by 0x17690B: qdict_destroy_obj (qdict.c:451) ==8458== by 0x1784AB: qobject_destroy (qobject.c:29) ==8458== by 0x171927: qobject_decref (qobject.h:83) ==8458== by 0x1721B4: qmp_output_free (qmp-output-visitor.c:223) ==8458== by 0x16ED8E: visit_free (qapi-visit-core.c:34) ==8458== by 0x130F39: visitor_output_teardown (test-qmp-output-visitor.c:44) ==8458== by 0x130FE1: visitor_reset (test-qmp-output-visitor.c:59) ==8458== by 0x1326EF: test_visitor_out_struct_errors (test-qmp-output-visitor.c:283) ==8458== by 0x50A8940: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2) ==8458== by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2) ==8458== Address 0x8259db8 is 8 bytes inside a block of size 4,120 free'd ==8458== at 0x4C2CD5A: free (vg_replace_malloc.c:530) ==8458== by 0x5088F2D: g_free (in /usr/lib64/libglib-2.0.so.0.4800.2) ==8458== by 0x176937: qdict_destroy_obj (qdict.c:456) ==8458== by 0x1784AB: qobject_destroy (qobject.c:29) ==8458== by 0x171927: qobject_decref (qobject.h:83) ==8458== by 0x1721B4: qmp_output_free (qmp-output-visitor.c:223) ==8458== by 0x16ED8E: visit_free (qapi-visit-core.c:34) ==8458== by 0x130F39: visitor_output_teardown (test-qmp-output-visitor.c:44) ==8458== by 0x130FE1: visitor_reset (test-qmp-output-visitor.c:59) ==8458== by 0x1326EF: test_visitor_out_struct_errors (test-qmp-output-visitor.c:283) ==8458== by 0x50A8940: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2) ==8458== by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2) ==8458== Block was alloc'd at ==8458== at 0x4C2DA60: calloc (vg_replace_malloc.c:711) ==8458== by 0x5088E70: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.4800.2) ==8458== by 0x175EAF: qdict_new (qdict.c:33) ==8458== by 0x171CAB: qmp_output_start_struct (qmp-output-visitor.c:108) ==8458== by 0x16EE4A: visit_start_struct (qapi-visit-core.c:47) ==8458== by 0x1326E3: test_visitor_out_struct_errors (test-qmp-output-visitor.c:282) ==8458== by 0x50A8940: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2) ==8458== by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2) ==8458== by 0x50A8B0E: ??? (in /usr/lib64/libglib-2.0.so.0.4800.2) ==8458== by 0x50A8D1D: g_test_run_suite (in /usr/lib64/libglib-2.0.so.0.4800.2) ==8458== by 0x50A8D40: g_test_run (in /usr/lib64/libglib-2.0.so.0.4800.2) ==8458== by 0x134F8D: main (test-qmp-output-visitor.c:899) diff --git i/tests/test-qmp-output-visitor.c w/tests/test-qmp-output-visitor.c index 5e926d3..f1b5591 100644 --- i/tests/test-qmp-output-visitor.c +++ w/tests/test-qmp-output-visitor.c @@ -265,18 +265,22 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data, EnumOne bad_values[] = { ENUM_ONE__MAX, -1 }; UserDefOne u = {0}; UserDefOne *pu = &u; - Error *err; + Error *err = NULL; int i; + /* First check: error within struct is detected */ for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) { - err = NULL; u.has_enum1 = true; u.enum1 = bad_values[i]; visit_type_UserDefOne(data->ov, "unused", &pu, &err); - g_assert(err); - error_free(err); + error_free_or_abort(&err); visitor_reset(data); } + + /* Second check: aborting visit early doesn't leak */ + visit_start_struct(data->ov, NULL, NULL, 0, &error_abort); + visit_start_struct(data->ov, "nested", NULL, 0, &error_abort); + visitor_reset(data); } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor 2016-10-21 22:10 ` Eric Blake @ 2016-10-24 6:56 ` Markus Armbruster 0 siblings, 0 replies; 6+ messages in thread From: Markus Armbruster @ 2016-10-24 6:56 UTC (permalink / raw) To: Eric Blake; +Cc: Pino Toscano, qemu-devel, mdroth Eric Blake <eblake@redhat.com> writes: > On 10/21/2016 04:32 PM, Eric Blake wrote: >> 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? >> >> 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. > > And below is my hack to test-qmp-output-visitor, which shows (at least > under valgrind) that we don't want the patch. I should probably turn > that into a formal patch submission; but should I piggyback the existing > test or add a new one? With the patch omitted, valgrind is happy; with > the patch applied, I see: > > /visitor/output/struct-errors: ==8458== Invalid read of size 8 > ==8458== at 0x175DC2: qobject_decref (qobject.h:81) > ==8458== by 0x1767A5: qentry_destroy (qdict.c:413) > ==8458== by 0x17690B: qdict_destroy_obj (qdict.c:451) > ==8458== by 0x1784AB: qobject_destroy (qobject.c:29) > ==8458== by 0x171927: qobject_decref (qobject.h:83) > ==8458== by 0x1721B4: qmp_output_free (qmp-output-visitor.c:223) > ==8458== by 0x16ED8E: visit_free (qapi-visit-core.c:34) > ==8458== by 0x130F39: visitor_output_teardown > (test-qmp-output-visitor.c:44) > ==8458== by 0x130FE1: visitor_reset (test-qmp-output-visitor.c:59) > ==8458== by 0x1326EF: test_visitor_out_struct_errors > (test-qmp-output-visitor.c:283) [...] > > > diff --git i/tests/test-qmp-output-visitor.c > w/tests/test-qmp-output-visitor.c > index 5e926d3..f1b5591 100644 > --- i/tests/test-qmp-output-visitor.c > +++ w/tests/test-qmp-output-visitor.c > @@ -265,18 +265,22 @@ static void > test_visitor_out_struct_errors(TestOutputVisitorData *data, > EnumOne bad_values[] = { ENUM_ONE__MAX, -1 }; > UserDefOne u = {0}; > UserDefOne *pu = &u; > - Error *err; > + Error *err = NULL; > int i; > > + /* First check: error within struct is detected */ > for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) { > - err = NULL; > u.has_enum1 = true; > u.enum1 = bad_values[i]; > visit_type_UserDefOne(data->ov, "unused", &pu, &err); > - g_assert(err); > - error_free(err); > + error_free_or_abort(&err); > visitor_reset(data); > } Just cleanup so far. > + > + /* Second check: aborting visit early doesn't leak */ > + visit_start_struct(data->ov, NULL, NULL, 0, &error_abort); > + visit_start_struct(data->ov, "nested", NULL, 0, &error_abort); > + visitor_reset(data); > } Since this isn't about errors, adding it to test_visitor_out_struct_errors() is perhaps questionable. Use your judgement. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-24 6:56 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-18 10:37 [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor Pino Toscano 2016-10-21 14:01 ` Markus Armbruster 2016-10-21 15:39 ` Eric Blake 2016-10-21 21:32 ` Eric Blake 2016-10-21 22:10 ` Eric Blake 2016-10-24 6:56 ` Markus Armbruster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).