* [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).