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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Eric Blake <address@hidden> writes:
>
>> 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, which will be fixed shortly in a future patch.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> Cc: address@hidden
>> Reviewed-by: Marc-André Lureau <address@hidden>
>>
>> ---
>> v9: enhance commit message
>> v8: rebase to earlier changes
>> v7: cc qemu-stable, tweak some asserts, drop stale comment, add more
>> comments
>> v6: no change
>> ---
>>  qapi/qmp-output-visitor.c       | 39 ++++++++++++++++++++++++++++++++-------
>>  tests/test-qmp-output-visitor.c |  2 ++
>>  2 files changed, 34 insertions(+), 7 deletions(-)
>>
>> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
>> index d367148..316f4e4 100644
>> --- a/qapi/qmp-output-visitor.c
>> +++ b/qapi/qmp-output-visitor.c
[...]
>> @@ -41,10 +50,12 @@ static QmpOutputVisitor *to_qov(Visitor *v)
>>      return container_of(v, QmpOutputVisitor, visitor);
>>  }
>>
>> +/* Push @value onto the stack of current QObjects being built */
>>  static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
>>  {
>>      QStackEntry *e = g_malloc0(sizeof(*e));
>>
>> +    assert(value);
>>      e->value = value;
>>      if (qobject_type(e->value) == QTYPE_QLIST) {
>>          e->is_list_head = true;
>> @@ -52,44 +63,51 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, 
>> QObject *value)
>>      QTAILQ_INSERT_HEAD(&qov->stack, e, node);
>>  }
>>
>> +/* Grab and remove the most recent QObject from the stack */

Let's stick to the stack terminology you used for qmp_output_push_obj():

/* Pop a value off the stack of QObjects being built, and return it */

>>  static QObject *qmp_output_pop(QmpOutputVisitor *qov)
>>  {
>>      QStackEntry *e = QTAILQ_FIRST(&qov->stack);
>>      QObject *value;
>> +
>> +    assert(e);
>>      QTAILQ_REMOVE(&qov->stack, e, node);
>>      value = e->value;
>>      g_free(e);
>>      return value;
>
> @value cannot be null here, because we never push nulls.  Worth an
> assertion, just to help readers?
>
>>  }
[...]
>> +/* Grab the most recent QObject from the stack, which must exist */

Stack terminology:

/*
 * Peek at the top of the stack of QObject being built.
 * The stack must not be empty.
 */

>>  static QObject *qmp_output_last(QmpOutputVisitor *qov)
>>  {
>>      QStackEntry *e = QTAILQ_FIRST(&qov->stack);
>> +
>> +    assert(e && e->value);
>>      return e->value;
>>  }
[...]