* [Qemu-devel] [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value @ 2014-02-17 11:52 Marcel Apfelbaum 2014-02-17 17:38 ` Eric Blake 0 siblings, 1 reply; 6+ messages in thread From: Marcel Apfelbaum @ 2014-02-17 11:52 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, mdroth, lcapitulino 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(+) 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; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value 2014-02-17 11:52 [Qemu-devel] [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value Marcel Apfelbaum @ 2014-02-17 17:38 ` Eric Blake 2014-02-17 18:01 ` Marcel Apfelbaum 0 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2014-02-17 17:38 UTC (permalink / raw) To: Marcel Apfelbaum, qemu-devel; +Cc: pbonzini, mdroth, lcapitulino [-- Attachment #1: Type: text/plain, Size: 1085 bytes --] 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> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value 2014-02-17 17:38 ` Eric Blake @ 2014-02-17 18:01 ` Marcel Apfelbaum 2014-02-28 16:37 ` Luiz Capitulino 0 siblings, 1 reply; 6+ messages in thread From: Marcel Apfelbaum @ 2014-02-17 18:01 UTC (permalink / raw) To: Eric Blake; +Cc: pbonzini, lcapitulino, qemu-devel, mdroth 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> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value 2014-02-17 18:01 ` Marcel Apfelbaum @ 2014-02-28 16:37 ` Luiz Capitulino 2014-03-02 10:15 ` Marcel Apfelbaum 0 siblings, 1 reply; 6+ messages in thread From: Luiz Capitulino @ 2014-02-28 16:37 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: pbonzini, qemu-devel, mdroth 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> > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value 2014-02-28 16:37 ` Luiz Capitulino @ 2014-03-02 10:15 ` Marcel Apfelbaum 2014-03-03 16:53 ` Luiz Capitulino 0 siblings, 1 reply; 6+ messages in thread From: Marcel Apfelbaum @ 2014-03-02 10:15 UTC (permalink / raw) To: Luiz Capitulino; +Cc: pbonzini, qemu-devel, mdroth 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> > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value 2014-03-02 10:15 ` Marcel Apfelbaum @ 2014-03-03 16:53 ` Luiz Capitulino 0 siblings, 0 replies; 6+ messages in thread From: Luiz Capitulino @ 2014-03-03 16:53 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: pbonzini, qemu-devel, mdroth 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> > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-03-03 16:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-17 11:52 [Qemu-devel] [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value Marcel Apfelbaum 2014-02-17 17:38 ` Eric Blake 2014-02-17 18:01 ` Marcel Apfelbaum 2014-02-28 16:37 ` Luiz Capitulino 2014-03-02 10:15 ` Marcel Apfelbaum 2014-03-03 16:53 ` Luiz Capitulino
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).