Re: [Qemu-devel] [PATCH v9 19/37] 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).  (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
> @@ -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).  */
>      QStack stack;
>  };
>
> @@ -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 */
>  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 root QObject, if any, in preparation to empty the stack */

Why is this "in preparation to empty the stack"?

>  static QObject *qmp_output_first(QmpOutputVisitor *qov)
>  {
>      QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
>
> -    /*
> -     * FIXME Wrong, because qmp_output_get_qobject() will increment
> -     * the refcnt *again*.  We need to think through how visitors
> -     * handle null.
> -     */
>      if (!e) {
> -        return qnull();
> +        /* No root */
> +        return NULL;
>      }
> -
> +    assert(e->value);
>      return e->value;
>  }

Two callers to check: qmp_output_get_qobject() and
qmp_output_visitor_cleanup().  See below.

>
> +/* 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 && e->value);
>      return e->value;
>  }
>
> +/* 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 +116,17 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, 
> const char *name,
>
>      switch (qobject_type(cur)) {
>      case QTYPE_QDICT:
> +        assert(name);
>          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 */
> +        /* FIXME this is abusing the stack; see comment above */
>          qobject_decref(qmp_output_pop(qov));
> +        assert(QTAILQ_EMPTY(&qov->stack));
>          qmp_output_push_obj(qov, value);
>          break;
>      }
> @@ -205,11 +227,14 @@ static void qmp_output_type_any(Visitor *v, const char 
> *name, QObject **obj,
>      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) {
>          qobject_incref(obj);
> +    } else {
> +        obj = qnull();
>      }

qmp_output_first() now returns NULL instead of qnull().  This changes
compensates; taken together, the beavior is the same, except we avoid
the qobject_incref() in the qnull() case.  That's the bug fix.

Aside: I believe obj could not be null before this patch, so the
conditional was redundant.  No need to complicate the commit message
with this, I think.

>      return obj;
>  }

Unchanged:

   void qmp_output_visitor_cleanup(QmpOutputVisitor *v)
   {
       QStackEntry *e, *tmp;

       /* The bottom QStackEntry, if any, owns the root QObject. See the
        * qmp_output_push_obj() invocations in qmp_output_add_obj(). */
       QObject *root = QTAILQ_EMPTY(&v->stack) ? NULL : qmp_output_first(v);

Calls qmp_output_first() only when the stack isn't empty, and then
qmp_output_first() behaves exactly as before this patch.  Good.

       QTAILQ_FOREACH_SAFE(e, &v->stack, node, tmp) {
           QTAILQ_REMOVE(&v->stack, e, node);
           g_free(e);
       }

       qobject_decref(root);
       g_free(v);
   }

> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 4df94bc..26dc752 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);
>  }