[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qapi: output visitor crashes qemu if it encount
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value |
Date: |
Mon, 3 Mar 2014 11:53:35 -0500 |
On Sun, 02 Mar 2014 12:15:45 +0200
Marcel Apfelbaum <address@hidden> wrote:
> On Fri, 2014-02-28 at 11:37 -0500, Luiz Capitulino wrote:
> > 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?
> Hi Luiz, thanks for the review!
>
> I will send an RFC V2 soon and CC you to it. Basically if you have a string
> property
> with a NULL value and you access it with obj_property_get_str, QEMU will
> crush.
> You can see this in the series.
Yes, saw it. Fix looks OK.
> > > 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()?
> You lost me here, I debugged the above crush and this was the exact place the
> check
> should be in order to avoid it. It is a "get" path.
If we allow stack elements to be NULL, then I'd guess that the same problem
might as well happen with qmp_output_last().
> >
> > Btw, sorry for the long delay.
> No problem, I am going to send this as part of machine QOMify series.
>
> Thanks,
> Marcel
>
> > >
> > > >
> > > > >
> > > > > 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>
> > > >
> > >
> > >
> > >
> >
>
>
>