Re: [Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor

[Eric Blake <eblake@redhat.com>]

[-- 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 --]