Re: [Qemu-devel] [PATCH v6 05/23] qmp: Fix reference-counting of qnull on empty output visit

[Markus Armbruster]

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).

The commit message says "We'll want to fix it for real before the
release."  Is this patch for 2.5?  For what it's worth, it applies to
master.

> 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>
>
> ---
> v6: no change
> ---
>  qapi/qmp-output-visitor.c       | 30 ++++++++++++++++++++++++++++--
>  tests/test-qmp-output-visitor.c |  2 ++
>  2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index e66ab3c..9d0f9d1 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -29,6 +29,12 @@ 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, and the stack of objects
> +     * still being built.  Worse, our behavior is inconsistent:
> +     * 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.  */

Uhh, I'll simply take your word for it.

>      QStack stack;
>  };
>
> @@ -41,10 +47,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;

Keep in mind for later: we never push a null value on the stack, and
there is no other way to grow the stack.  Therefore, the stack holds
only non-null values.

>      if (qobject_type(e->value) == QTYPE_QLIST) {
>          e->is_list_head = true;
> @@ -52,16 +60,20 @@ 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 */
>  static QObject *qmp_output_pop(QmpOutputVisitor *qov)
>  {
>      QStackEntry *e = QTAILQ_FIRST(&qov->stack);
>      QObject *value;
> +
> +    assert(e);

I figure this assert the stack isn't empty.  Correct?

>      QTAILQ_REMOVE(&qov->stack, e, node);
>      value = e->value;
>      g_free(e);
>      return value;
>  }
>
> +/* Grab the root QObject, if any, in preparation to empty the stack */
>  static QObject *qmp_output_first(QmpOutputVisitor *qov)
>  {
>      QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
> @@ -72,24 +84,32 @@ static QObject *qmp_output_first(QmpOutputVisitor *qov)
       /*
        * FIXME Wrong, because qmp_output_get_qobject() will increment
        * the refcnt *again*.  We need to think through how visitors
>       * handle null.
>       */

Sure you don't want to drop the FIXME?

>      if (!e) {

I figure this means the stack is empty.  Now I wish I understood how we
use the stack.
    
> -        return qnull();
> +        /* No root */
> +        return NULL;
>      }
> -
> +    assert(e->value);

Safe because the stack holds only non-null values.

>      return e->value;
>  }

We return null exactly when the stack is empty (whatever that may mean).

>
> +/* Grab the most recent QObject from the stack, which must exist */
>  static QObject *qmp_output_last(QmpOutputVisitor *qov)
>  {
>      QStackEntry *e = QTAILQ_FIRST(&qov->stack);
> +
> +    assert(e);
>      return e->value;
>  }

Can't return null, because the stack holds only non-null values.

assert(e && e->value) to match qmp_output_first()?

>
> +/* Add @value to the current QObject being built.
> + * If the stack is visiting a dictionary or list, @value is now owned
> + * by that container. Otherwise, @value is now the root.  */
>  static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
>                                 QObject *value)
>  {
>      QObject *cur;
>
>      if (QTAILQ_EMPTY(&qov->stack)) {
> +        /* Stack was empty, track this object as root */
>          qmp_output_push_obj(qov, value);
>          return;
>      }
> @@ -98,13 +118,16 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, 
> const char *name,
>
>      switch (qobject_type(cur)) {
>      case QTYPE_QDICT:
> +        assert(name);

Okay, but it really only converts one kind of crash into another one.

>          qdict_put_obj(qobject_to_qdict(cur), name, value);
>          break;
>      case QTYPE_QLIST:
>          qlist_append_obj(qobject_to_qlist(cur), value);
>          break;
>      default:
> +        /* The previous root was a scalar, replace it with a new root */
>          qobject_decref(qmp_output_pop(qov));
> +        assert(QTAILQ_EMPTY(&qov->stack));
>          qmp_output_push_obj(qov, value);
>          break;
>      }

Hand me the bucket.

> @@ -205,11 +228,14 @@ static void qmp_output_type_any(Visitor *v, QObject 
> **obj, const char *name,
>      qmp_output_add_obj(qov, name, *obj);
>  }
>
> +/* Finish building, and return the root object. Will not be NULL. */
>  QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
>  {
>      QObject *obj = qmp_output_first(qov);
>      if (obj) {

Non-empty stack.

>          qobject_incref(obj);
> +    } else {

Empty stack.

> +        obj = qnull();
>      }
>      return obj;
>  }

Thanks for your comments.  They help a lot with understanding the code,
and they also demonstrate that it's muddled crap %-}

So, how does this contraption work?

A visitor cab encounter NULL only when it visits pointers (d'oh!).
Searching qapi-visit-core.c for **obj finds start_struct(),
start_implicit_struct(), type_str(), type_any().

As far as I can tell, start_implicit_struct() is for unboxed structs, so
NULL must not happen there.  This visitor doesn't implement it anyway.

qmp_output_type_str() substitutes "" for NULL.  Ooookay.

qmp_output_type_any() crashes on NULL.  Can this happen?

qmp_output_start_struct() ignores its obj argument.  My best guess is
that this substitutes an empty dictionary for NULL.

Okay, I give up: how do we get an empty stack and return qnull()?

> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 3078442..8e6fc33 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -461,6 +461,8 @@ static void test_visitor_out_empty(TestOutputVisitorData 
> *data,
>
>      arg = qmp_output_get_qobject(data->qov);
>      g_assert(qobject_type(arg) == QTYPE_QNULL);
> +    /* Check that qnull reference counting is sane */
> +    g_assert(arg->refcnt == 2);
>      qobject_decref(arg);
>  }