Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
> Cc: qemu-sta...@nongnu.org
> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>
> ---
> 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);
> }
