* [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).