| Message ID | 1392637972-24719-1-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
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? > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > --- > 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: Reviewed-by: Eric Blake <eblake@redhat.com>
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 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. > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > --- > > 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 <eblake@redhat.com> >
On Mon, 17 Feb 2014 20:01:42 +0200 Marcel Apfelbaum <marcel.a@redhat.com> 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 <marcel.a@redhat.com> > > > --- > > > 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 <eblake@redhat.com> > > > > >
On Fri, 2014-02-28 at 11:37 -0500, Luiz Capitulino wrote: > On Mon, 17 Feb 2014 20:01:42 +0200 > Marcel Apfelbaum <marcel.a@redhat.com> 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. > > > 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. > > 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 <marcel.a@redhat.com> > > > > --- > > > > 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 <eblake@redhat.com> > > > > > > > > > >
On Sun, 02 Mar 2014 12:15:45 +0200 Marcel Apfelbaum <marcel.a@redhat.com> wrote: > On Fri, 2014-02-28 at 11:37 -0500, Luiz Capitulino wrote: > > On Mon, 17 Feb 2014 20:01:42 +0200 > > Marcel Apfelbaum <marcel.a@redhat.com> 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 <marcel.a@redhat.com> > > > > > --- > > > > > 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 <eblake@redhat.com> > > > > > > > > > > > > > > > > > >
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; }
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. Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- qapi/qmp-output-visitor.c | 5 +++++ 1 file changed, 5 insertions(+)