Re: [Qemu-devel] [PATCH v8 19/35] qmp: Fix reference-counting of qnull on empty output visit

[Eric Blake]

On 01/05/2016 07:05 AM, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <address@hidden> wrote:
>> Commit 6c2f9a15 ensured that we would not return NULL when the
>> caller used an output visitor but had nothing to visit. But
>> in doing so, it added a FIXME about a reference count leak
>> that could abort qemu in the (unlikely) case of SIZE_MAX such
>> visits (more plausible on 32-bit).  (Although that commit
>> suggested we might fix it in time for 2.5, we ran out of time;
>> fortunately, it is unlikely enough to bite that it was not
>> worth worrying about during the 2.5 release.)
>>
>> This fixes things by documenting the internal contracts, and
>> explaining why the internal function can return NULL and only
>> the public facing interface needs to worry about qnull(),
>> thus avoiding over-referencing the qnull_ global object.
>>
>> It does not, however, fix the stupidity of the stack mixing
>> up two separate pieces of information; add a FIXME to explain
>> that issue.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> Cc: address@hidden
>>

>> +++ b/qapi/qmp-output-visitor.c
>> @@ -29,6 +29,15 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
>>  struct QmpOutputVisitor
>>  {
>>      Visitor visitor;
>> +    /* FIXME: we are abusing stack to hold two separate pieces of
>> +     * information: the current root object in slot 0, and the stack
>> +     * of N objects still being built in slots 1 through N (for N+1
>> +     * slots in use).  Worse, our behavior is inconsistent:
>> +     * qmp_output_add_obj() visiting two top-level scalars in a row
>> +     * discards the first in favor of the second, but visiting two
>> +     * top-level objects in a row tries to append the second object
>> +     * into the first (since the first object was placed in the stack
>> +     * in both slot 0 and 1, but only popped from slot 1).  */
> 
> I skipped checking thoroughly this comment, since it's a bit
> off-topic, although it looks ok.
> 
> Later, oh well, it's fixed in next commit. Imho it's not strictly
> necessary in this commit.

I added the comment based on Markus' request that I document how the
stack is used; but yes, it does feel like a bit of churn since it
changes in the next commit.

If there's a reason to respin, I might change it to:

    Visitor visitor;
    /* Stack holds two pieces of information: the current root object in
     * slot 0, then a stack of N objects still being built in slots 1
     * through N (for N+1 slots in use).
     * FIXME: The root object should be stored separately from the
     * stack, particularly since qmp_output_add_obj() behaves
     * differently when visiting two top-level scalars in a row than
     * it does for two objects (the second object is appended to the
     * first, since the first is placed in both slots 0 and 1 but only
     * popped from slot 1).   */

> 
> Reviewed-by: Marc-André Lureau <address@hidden>
> 
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature