Re: [Qemu-devel] [PATCH v6 20/26] qapi: Fix output visitor to return qnull() instead of NULL

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3914 bytes --]

On 09/12/2015 02:10 AM, Markus Armbruster wrote:

> Relatively harmless, because the qnull_ singleton is static.  Worth
> fixing anyway, of course.
> 
>>> I'm still investigating, and may be able to find the patch
>>
>> Squash this in, and you can have:
>> Reviewed-by: Eric Blake <eblake@redhat.com>

Well, your further questions are spot on; my squash proposal isn't quite
right.


> I put the qnull() in qmp_output_first() to avoid (QObject *)0 entirely.
> Also because that's where I found the FIXME :)
> 
> You lift it into one of two callers.  Impact:
> 
> * qmp_output_get_qobject()
> 
>   - master: return NULL when !e || !e->value
> 
>   - my patch: return qnull() when !e
>               return NULL when !e->value
> 
>   - your patch: return qnull() when !e || !e->value

The leak in your patch was that qnull() increments the count, then
qmp_output_get_object() also increments the count.

I guess I'll have to investigate where e->value can be set to NULL to
see if my idea was safe, or if we'd have to do something even different.

If this were the only caller, then I guess we could always do something
like this in qmp_output_first(), leaving the possibility of returning
NULL for e->value:

if (!e) {
    obj = qnull();
    qobject_decref(obj); /* Caller will again increment refcount */
    return obj;
}

But it's not the only caller.

> 
> * qmp_output_visitor_cleanup()
> 
>   - master: root = NULL when when !e || !e->value
> 
>   - my patch: root = qnull() when !e
>               root = NULL when !e->value
> 
>   - your patch: root = NULL when when !e || !e->value

And calls qobject_decref(root), but that is safe on NULL.

Here, your patch ends up with a net 0 refcnt on qnull() (incremented in
qmp_output_first(), decremented in the cleanup), but my idea above to
pre-decrement would be wrong.

Another option would be to keep your patch to qmp_output_first(), but
then fix qmp_output_get_object() to special case it it has an instance
of QNull (no refcnt change) vs. anything else (qobject_incref).  But
that feels gross.

> 
> where e = QTAILQ_LAST(&qov->stack, QStack)
> 
> Questions:
> 
> * How can e become NULL?
> 
>   The only place that pushes onto &qov->stack appears to be
>   qmp_output_push_obj().  Can obviously push e with !e->value, but can't
>   push null e.

My understanding is that qov->stack corresponds to nesting levels of {}
or [] in the JSON code.  The testsuite shows a case where the stack is
empty as one case where e is NULL, but if e is non-NULL, I'm not sure
whether e->value can ever be NULL.  I'll have to read the code more closely.

> 
> * What about qmp_output_last()?
> 
>   Why does qmp_output_first() check for !e, but not qmp_output_last()?
> 
>   My patch makes them less symmetric (bad smell).  Yours makes them more
>   symmetric, but not quite.

Which is awkward in its own right.

> 
> * How does this contraption work?

Indeed. Without reading further, we're both shooting in the dark for
something that makes tests pass, but without being a clean interface.

How about this: go ahead with your series as proposed, with the squash
hunk to tests/ to avoid the leak in the testsuite, but leaving the leak
in qmp_output_get_object(), and we address the leak in a followup patch.
 refcnt is size_t, so on 32-bit platforms, it could roll over after 4G
repeats of the leak and cause catastrophe, but I don't think we are
outputting qnull() frequently enough for the leak to bite while we wait
for a followup patch.

The followup patch(es) will then have to include some contract
documentation, so that we track what we learned while investigating the
code, and so that the next reader has more than just code to start from.


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