Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/4] qapi: output visitor crashes qemu if it encounters a NULL value

[Markus Armbruster]

Marcel Apfelbaum <address@hidden> writes:

> A NULL value is not added to visitor's stack, but there
> is no check for that when the visitor tries to return
> that value, leading to Qemu crash.
>
> Reviewed-by: Eric Blake <address@hidden>
> Signed-off-by: Marcel Apfelbaum <address@hidden>
> ---
>  qapi/qmp-output-visitor.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index 74a5684..0562f49 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -66,6 +66,11 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov)
>  static QObject *qmp_output_first(QmpOutputVisitor *qov)
>  {
>      QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
> +
> +    if (!e) {
> +        return NULL;
> +    }
> +
>      return e->value;
>  }

Let's see how this thing works.

The visitor's mutable state is a QStack, which is stack of (QObject,
bool).  We can ignore the bool; it's just for qmp_output_next_list().

Visits start with an empty stack.  See qmp_output_visitor_new().

qmp_output_first() returns the object on the bottom of the stack.
qmp_output_last() returns the object on the top of the stack.

<rant>
When you implement a stack with a double-ended queue, you're totally
free to pick either end of the queue for top of stack.  You're also free
to name your functions accessing top and the bottom of the stack however
you like.  "Of course" the author picked queue end and function names
for maximum confusion:

    static QObject *qmp_output_first(QmpOutputVisitor *qov)
    {
        QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
        return e->value;
    }

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

I hate you.
</rant>

The result of the visit sits at the bottom of the stack.  Empty stack,
null result.  See qmp_output_get_qobject().

Visiting a scalar type creates the appropriate scalar QObject, and
"adds" it.  We'll find out what "adding" means shortly.  See
qmp_output_type_{int,bool,str,number}().

Special case: null strings get converted to empty strings.  See
qmp_output_type_str().

Starting a struct visit creates a QDict, adds it, and pushes it onto the
stack.  Ending it pops it from the stack.  See
qmp_output_{start,end}_struct().

Starting a list visit creates a QList, adds it, and pushes it onto the
stack.  Ending it pops it from the stack.  See
qmp_output_{start,end}_list().

Visiting a list member does nothing interesting; see
qmp_output_next_list().  Aside: I suspect the GenericList traversal
stuff now done in every next_list() method should be done in the visitor
core instead.

Now let's figure out what it means to "add" an object.  This is
qmp_output_add_obj().

If the stack is still empty, the object is the root object, and it gets
pushed.

Else, if the object on top of the stack is a QDict, we're visiting a
struct.  Enter the object into the QDict.

Else, if the object on top of the stack is a QList, we're visiting a
list.  Append the object to the QList.

Else, the object on top of the stack must be scalar, and I think it must
be the root object.  We replace it by the object being added.  WTF?

This feels more complicated than it could be.  Anyway, how could a null
object end up at the bottom of the stack, so that qmp_output_first()
chokes on it?  I can't see that.

If it can get added, then why can it be seen only by qmp_output_first(),
but not by qmp_output_last() and qmp_output_pop()?