[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor |
Date: |
Fri, 21 Oct 2016 10:39:53 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
On 10/21/2016 09:01 AM, Markus Armbruster wrote:
> Pino Toscano <address@hidden> 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 <address@hidden>
>> ---
>>
>> 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