[Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on qdev_device_help()

[Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on qdev_device_help()

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Normally, qmp_device_list_properties() may return NULL when
a device haven't special properties excpet Object and DeviceState
properties, such as virtio-balloon-device.

We just need check local_err instead of prop_list.

Example:

Segmentation fault (core dumped)

The backtrace as below:

Program received signal SIGSEGV, Segmentation fault.
0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
152         return err->msg;
(gdb) bt
#0  0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
#1  0x000055555572fce9 in qdev_device_help (opts=0x5555562fdfe0) at qdev-monitor.c:210
#2  0x000055555574a6f2 in device_help_func (opts=0x5555562fdfe0, opaque=0x0) at vl.c:2362
#3  0x00005555559c0a33 in qemu_opts_foreach (list=0x555555dd0b40 <qemu_device_opts>, 
    func=0x55555574a6ca <device_help_func>, opaque=0x0, abort_on_failure=0) at util/qemu-option.c:1072
#4  0x000055555574f514 in main (argc=3, argv=0x7fffffffe218, envp=0x7fffffffe238) at vl.c:4246

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 qdev-monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index fb9ee24..5ec6606 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -206,7 +206,7 @@ int qdev_device_help(QemuOpts *opts)
     }
 
     prop_list = qmp_device_list_properties(driver, &local_err);
-    if (!prop_list) {
+    if (local_err) {
         error_printf("%s\n", error_get_pretty(local_err));
         error_free(local_err);
         return 1;
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on qdev_device_help()

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> Normally, qmp_device_list_properties() may return NULL when
> a device haven't special properties excpet Object and DeviceState
> properties, such as virtio-balloon-device.
>
> We just need check local_err instead of prop_list.
>
> Example:
>
> Segmentation fault (core dumped)
>
> The backtrace as below:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
> 152         return err->msg;
> (gdb) bt
> #0  0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
> #1  0x000055555572fce9 in qdev_device_help (opts=0x5555562fdfe0) at qdev-monitor.c:210
> #2  0x000055555574a6f2 in device_help_func (opts=0x5555562fdfe0, opaque=0x0) at vl.c:2362
> #3  0x00005555559c0a33 in qemu_opts_foreach (list=0x555555dd0b40 <qemu_device_opts>, 
>     func=0x55555574a6ca <device_help_func>, opaque=0x0, abort_on_failure=0) at util/qemu-option.c:1072
> #4  0x000055555574f514 in main (argc=3, argv=0x7fffffffe218, envp=0x7fffffffe238) at vl.c:4246
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  qdev-monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index fb9ee24..5ec6606 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -206,7 +206,7 @@ int qdev_device_help(QemuOpts *opts)
>      }
>  
>      prop_list = qmp_device_list_properties(driver, &local_err);
> -    if (!prop_list) {
> +    if (local_err) {
>          error_printf("%s\n", error_get_pretty(local_err));
>          error_free(local_err);
>          return 1;

Doesn't this leak prop_list when local_err && prop_list?

Returning both a value in need of destruction and an error object is at
least highly unusual, and probably plain wrong.

Should qmp_device_list_properties() return NULL when it sets an error?

Re: [Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on qdev_device_help()

[Gonglei (Arei)]

> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Tuesday, September 16, 2014 3:28 PM
> Subject: Re: [Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on
> qdev_device_help()
> 
> <arei.gonglei@huawei.com> writes:
> 
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > Normally, qmp_device_list_properties() may return NULL when
> > a device haven't special properties excpet Object and DeviceState
> > properties, such as virtio-balloon-device.
> >
> > We just need check local_err instead of prop_list.
> >
> > Example:
> >
> > Segmentation fault (core dumped)
> >
> > The backtrace as below:
> >
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
> > 152         return err->msg;
> > (gdb) bt
> > #0  0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
> > #1  0x000055555572fce9 in qdev_device_help (opts=0x5555562fdfe0) at
> qdev-monitor.c:210
> > #2  0x000055555574a6f2 in device_help_func (opts=0x5555562fdfe0,
> opaque=0x0) at vl.c:2362
> > #3  0x00005555559c0a33 in qemu_opts_foreach (list=0x555555dd0b40
> <qemu_device_opts>,
> >     func=0x55555574a6ca <device_help_func>, opaque=0x0,
> abort_on_failure=0) at util/qemu-option.c:1072
> > #4  0x000055555574f514 in main (argc=3, argv=0x7fffffffe218,
> envp=0x7fffffffe238) at vl.c:4246
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  qdev-monitor.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index fb9ee24..5ec6606 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -206,7 +206,7 @@ int qdev_device_help(QemuOpts *opts)
> >      }
> >
> >      prop_list = qmp_device_list_properties(driver, &local_err);
> > -    if (!prop_list) {
> > +    if (local_err) {
> >          error_printf("%s\n", error_get_pretty(local_err));
> >          error_free(local_err);
> >          return 1;
> 
> Doesn't this leak prop_list when local_err && prop_list?
> 
No, it will not happen this situation.

> Returning both a value in need of destruction and an error object is at
> least highly unusual, and probably plain wrong.
> 
> Should qmp_device_list_properties() return NULL when it sets an error?

Yes, it was.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on qdev_device_help()

[Markus Armbruster]

"Gonglei (Arei)" <arei.gonglei@huawei.com> writes:

>> From: Markus Armbruster [mailto:armbru@redhat.com]
>> Sent: Tuesday, September 16, 2014 3:28 PM
>> Subject: Re: [Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on
>> qdev_device_help()
>> 
>> <arei.gonglei@huawei.com> writes:
>> 
>> > From: Gonglei <arei.gonglei@huawei.com>
>> >
>> > Normally, qmp_device_list_properties() may return NULL when
>> > a device haven't special properties excpet Object and DeviceState
>> > properties, such as virtio-balloon-device.
>> >
>> > We just need check local_err instead of prop_list.
>> >
>> > Example:
>> >
>> > Segmentation fault (core dumped)
>> >
>> > The backtrace as below:
>> >
>> > Program received signal SIGSEGV, Segmentation fault.
>> > 0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
>> > 152         return err->msg;
>> > (gdb) bt
>> > #0  0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
>> > #1  0x000055555572fce9 in qdev_device_help (opts=0x5555562fdfe0) at
>> qdev-monitor.c:210
>> > #2  0x000055555574a6f2 in device_help_func (opts=0x5555562fdfe0,
>> opaque=0x0) at vl.c:2362
>> > #3  0x00005555559c0a33 in qemu_opts_foreach (list=0x555555dd0b40
>> <qemu_device_opts>,
>> >     func=0x55555574a6ca <device_help_func>, opaque=0x0,
>> abort_on_failure=0) at util/qemu-option.c:1072
>> > #4  0x000055555574f514 in main (argc=3, argv=0x7fffffffe218,
>> envp=0x7fffffffe238) at vl.c:4246
>> >
>> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> > ---
>> >  qdev-monitor.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/qdev-monitor.c b/qdev-monitor.c
>> > index fb9ee24..5ec6606 100644
>> > --- a/qdev-monitor.c
>> > +++ b/qdev-monitor.c
>> > @@ -206,7 +206,7 @@ int qdev_device_help(QemuOpts *opts)
>> >      }
>> >
>> >      prop_list = qmp_device_list_properties(driver, &local_err);
>> > -    if (!prop_list) {
>> > +    if (local_err) {
>> >          error_printf("%s\n", error_get_pretty(local_err));
>> >          error_free(local_err);
>> >          return 1;
>> 
>> Doesn't this leak prop_list when local_err && prop_list?
>> 
> No, it will not happen this situation.
>
>> Returning both a value in need of destruction and an error object is at
>> least highly unusual, and probably plain wrong.
>> 
>> Should qmp_device_list_properties() return NULL when it sets an error?
>
> Yes, it was.

I think I'm starting to understand now.

You backtrace shows qmp_device_list_properties() returned null without
setting an error.  But this is okay, because null means "empty list",
which is a valid return value.

A systematic search for this kind of incorrect error handling would be
nice: search for functions returning QAPI lists, then look for callers
interpreting a null value as error.  Would you be willing to do that?

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on qdev_device_help()

[Gonglei (Arei)]

> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Tuesday, September 16, 2014 4:00 PM
> To: Gonglei (Arei)
> Subject: Re: [Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on
> qdev_device_help()
> 
> "Gonglei (Arei)" <arei.gonglei@huawei.com> writes:
> 
> >> From: Markus Armbruster [mailto:armbru@redhat.com]
> >> Sent: Tuesday, September 16, 2014 3:28 PM
> >> Subject: Re: [Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on
> >> qdev_device_help()
> >>
> >> <arei.gonglei@huawei.com> writes:
> >>
> >> > From: Gonglei <arei.gonglei@huawei.com>
> >> >
> >> > Normally, qmp_device_list_properties() may return NULL when
> >> > a device haven't special properties excpet Object and DeviceState
> >> > properties, such as virtio-balloon-device.
> >> >
> >> > We just need check local_err instead of prop_list.
> >> >
> >> > Example:
> >> >
> >> > Segmentation fault (core dumped)
> >> >
> >> > The backtrace as below:
> >> >
> >> > Program received signal SIGSEGV, Segmentation fault.
> >> > 0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
> >> > 152         return err->msg;
> >> > (gdb) bt
> >> > #0  0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
> >> > #1  0x000055555572fce9 in qdev_device_help (opts=0x5555562fdfe0) at
> >> qdev-monitor.c:210
> >> > #2  0x000055555574a6f2 in device_help_func (opts=0x5555562fdfe0,
> >> opaque=0x0) at vl.c:2362
> >> > #3  0x00005555559c0a33 in qemu_opts_foreach (list=0x555555dd0b40
> >> <qemu_device_opts>,
> >> >     func=0x55555574a6ca <device_help_func>, opaque=0x0,
> >> abort_on_failure=0) at util/qemu-option.c:1072
> >> > #4  0x000055555574f514 in main (argc=3, argv=0x7fffffffe218,
> >> envp=0x7fffffffe238) at vl.c:4246
> >> >
> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> > ---
> >> >  qdev-monitor.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> >> > index fb9ee24..5ec6606 100644
> >> > --- a/qdev-monitor.c
> >> > +++ b/qdev-monitor.c
> >> > @@ -206,7 +206,7 @@ int qdev_device_help(QemuOpts *opts)
> >> >      }
> >> >
> >> >      prop_list = qmp_device_list_properties(driver, &local_err);
> >> > -    if (!prop_list) {
> >> > +    if (local_err) {
> >> >          error_printf("%s\n", error_get_pretty(local_err));
> >> >          error_free(local_err);
> >> >          return 1;
> >>
> >> Doesn't this leak prop_list when local_err && prop_list?
> >>
> > No, it will not happen this situation.
> >
> >> Returning both a value in need of destruction and an error object is at
> >> least highly unusual, and probably plain wrong.
> >>
> >> Should qmp_device_list_properties() return NULL when it sets an error?
> >
> > Yes, it was.
> 
> I think I'm starting to understand now.
> 
> You backtrace shows qmp_device_list_properties() returned null without
> setting an error.  But this is okay, because null means "empty list",
> which is a valid return value.
> 
Yes.

> A systematic search for this kind of incorrect error handling would be
> nice: search for functions returning QAPI lists, then look for callers
> interpreting a null value as error.  Would you be willing to do that?
> 
Yes, I would.

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks. :)

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on qdev_device_help()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

On Tue, Sep 16, 2014 at 10:19:33AM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Normally, qmp_device_list_properties() may return NULL when
> a device haven't special properties excpet Object and DeviceState
> properties, such as virtio-balloon-device.
> 
> We just need check local_err instead of prop_list.
> 
> Example:
> 
> Segmentation fault (core dumped)
> 
> The backtrace as below:
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
> 152         return err->msg;
> (gdb) bt
> #0  0x00005555559af1a8 in error_get_pretty (err=0x0) at util/error.c:152
> #1  0x000055555572fce9 in qdev_device_help (opts=0x5555562fdfe0) at qdev-monitor.c:210
> #2  0x000055555574a6f2 in device_help_func (opts=0x5555562fdfe0, opaque=0x0) at vl.c:2362
> #3  0x00005555559c0a33 in qemu_opts_foreach (list=0x555555dd0b40 <qemu_device_opts>, 
>     func=0x55555574a6ca <device_help_func>, opaque=0x0, abort_on_failure=0) at util/qemu-option.c:1072
> #4  0x000055555574f514 in main (argc=3, argv=0x7fffffffe218, envp=0x7fffffffe238) at vl.c:4246
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  qdev-monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on qdev_device_help()

[Gonglei (Arei)]

> > >>
> > >> Doesn't this leak prop_list when local_err && prop_list?
> > >>
> > > No, it will not happen this situation.
> > >
> > >> Returning both a value in need of destruction and an error object is at
> > >> least highly unusual, and probably plain wrong.
> > >>
> > >> Should qmp_device_list_properties() return NULL when it sets an error?
> > >
> > > Yes, it was.
> >
> > I think I'm starting to understand now.
> >
> > You backtrace shows qmp_device_list_properties() returned null without
> > setting an error.  But this is okay, because null means "empty list",
> > which is a valid return value.
> >
> Yes.
> 
> > A systematic search for this kind of incorrect error handling would be
> > nice: search for functions returning QAPI lists, then look for callers
> > interpreting a null value as error.  Would you be willing to do that?
> >
> Yes, I would.
> 
Hi, Markus
I have finished this work, and not found this kind of error. :)

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH] qdev-monitor: fix segmentation fault on qdev_device_help()

[Markus Armbruster]

"Gonglei (Arei)" <arei.gonglei@huawei.com> writes:

>> > >>
>> > >> Doesn't this leak prop_list when local_err && prop_list?
>> > >>
>> > > No, it will not happen this situation.
>> > >
>> > >> Returning both a value in need of destruction and an error object is at
>> > >> least highly unusual, and probably plain wrong.
>> > >>
>> > >> Should qmp_device_list_properties() return NULL when it sets an error?
>> > >
>> > > Yes, it was.
>> >
>> > I think I'm starting to understand now.
>> >
>> > You backtrace shows qmp_device_list_properties() returned null without
>> > setting an error.  But this is okay, because null means "empty list",
>> > which is a valid return value.
>> >
>> Yes.
>> 
>> > A systematic search for this kind of incorrect error handling would be
>> > nice: search for functions returning QAPI lists, then look for callers
>> > interpreting a null value as error.  Would you be willing to do that?
>> >
>> Yes, I would.
>> 
> Hi, Markus
> I have finished this work, and not found this kind of error. :)

Thanks!