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

[Luiz Capitulino]

On Mon, 17 Feb 2014 20:01:42 +0200
Marcel Apfelbaum <address@hidden> wrote:

> On Mon, 2014-02-17 at 10:38 -0700, Eric Blake wrote:
> > On 02/17/2014 04:52 AM, Marcel Apfelbaum wrote:
> > > 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.
> > 
> > Do you have an easy formula for reproducing the crash?
> 
> Hi Eric, thank you for your review!
> 
> In order to reproduce this you need to use object_property_get_str
> on an object with a string property with a null value.

I tried looking into this but got a bit lost in the abstraction maze.
Can you point me to your series and how to reproduce it?

> I don't know if in the current code base we have this scenario, but
> I am trying to QOMify the QemuMachine and properties as "kernel" may be NULL.
> 
> Either way (if NULL properties are not wanted), IMHO it is recommended to 
> cover such cases in order to avoid QEMU crash. 

I agree, but I want to be sure we're fixing the right thing and not only
papering over a bug. Also, Why is the check not needed in qmp_output_last()?

Btw, sorry for the long delay.

> 
> > 
> > > 
> > > 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;
> > > +    }
> > > +
> > 
> > The code looks okay to me, but without a formula, my review is fairly weak:
> Appreciated,
> Marcel
> 
> > 
> > Reviewed-by: Eric Blake <address@hidden>
> > 
> 
> 
>