[Qemu-devel] [PATCH v3 08/38] 9pfs: Fix CLI parsing crash on error

Markus Armbruster posted 38 patches 7 years ago
There is a newer version of this series
[Qemu-devel] [PATCH v3 08/38] 9pfs: Fix CLI parsing crash on error
Posted by Markus Armbruster 7 years ago
Calling error_report() in a function that takes an Error ** argument
is suspicious.  9p-handle.c's handle_parse_opts() does that, and then
fails without setting an error.  Wrong.  Its caller crashes when it
tries to report the error:

    $ qemu-system-x86_64 -nodefaults -fsdev id=foo,fsdriver=handle
    qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: warning: handle backend is deprecated
    qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: fsdev: No path specified
    Segmentation fault (core dumped)

Screwed up when commit 91cda4e8f37 (v2.12.0) converted the function to
Error.  Fix by calling error_setg() instead of error_report().

Fixes: 91cda4e8f372602795e3a2f4bd2e3adaf9f82255
Cc: Greg Kurz <groug@kaod.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-handle.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
index f3641dbe4a..3465b1ef30 100644
--- a/hw/9pfs/9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -19,6 +19,7 @@
 #include <grp.h>
 #include <sys/socket.h>
 #include <sys/un.h>
+#include "qapi/error.h"
 #include "qemu/xattr.h"
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
@@ -655,12 +656,13 @@ static int handle_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
     warn_report("handle backend is deprecated");
 
     if (sec_model) {
-        error_report("Invalid argument security_model specified with handle fsdriver");
+        error_setg(errp,
+                   "Invalid argument security_model specified with handle fsdriver");
         return -1;
     }
 
     if (!path) {
-        error_report("fsdev: No path specified");
+        error_setg(errp, "fsdev: No path specified");
         return -1;
     }
     fse->path = g_strdup(path);
-- 
2.17.1


Re: [Qemu-devel] [PATCH v3 08/38] 9pfs: Fix CLI parsing crash on error
Posted by Greg Kurz 7 years ago
On Tue, 16 Oct 2018 19:41:28 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Calling error_report() in a function that takes an Error ** argument
> is suspicious.  9p-handle.c's handle_parse_opts() does that, and then
> fails without setting an error.  Wrong.  Its caller crashes when it
> tries to report the error:
> 
>     $ qemu-system-x86_64 -nodefaults -fsdev id=foo,fsdriver=handle
>     qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: warning: handle backend is deprecated
>     qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: fsdev: No path specified
>     Segmentation fault (core dumped)
> 
> Screwed up when commit 91cda4e8f37 (v2.12.0) converted the function to
> Error.  Fix by calling error_setg() instead of error_report().
> 
> Fixes: 91cda4e8f372602795e3a2f4bd2e3adaf9f82255
> Cc: Greg Kurz <groug@kaod.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Acked-by: Greg Kurz <groug@kaod.org>
> ---

Hi Markus,

FWIW you had a Reviewed-by from Eric.

https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg03297.html

Cheers,

--
Greg

>  hw/9pfs/9p-handle.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
> index f3641dbe4a..3465b1ef30 100644
> --- a/hw/9pfs/9p-handle.c
> +++ b/hw/9pfs/9p-handle.c
> @@ -19,6 +19,7 @@
>  #include <grp.h>
>  #include <sys/socket.h>
>  #include <sys/un.h>
> +#include "qapi/error.h"
>  #include "qemu/xattr.h"
>  #include "qemu/cutils.h"
>  #include "qemu/error-report.h"
> @@ -655,12 +656,13 @@ static int handle_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
>      warn_report("handle backend is deprecated");
>  
>      if (sec_model) {
> -        error_report("Invalid argument security_model specified with handle fsdriver");
> +        error_setg(errp,
> +                   "Invalid argument security_model specified with handle fsdriver");
>          return -1;
>      }
>  
>      if (!path) {
> -        error_report("fsdev: No path specified");
> +        error_setg(errp, "fsdev: No path specified");
>          return -1;
>      }
>      fse->path = g_strdup(path);


Re: [Qemu-devel] [PATCH v3 08/38] 9pfs: Fix CLI parsing crash on error
Posted by Markus Armbruster 7 years ago
Greg Kurz <groug@kaod.org> writes:

> On Tue, 16 Oct 2018 19:41:28 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Calling error_report() in a function that takes an Error ** argument
>> is suspicious.  9p-handle.c's handle_parse_opts() does that, and then
>> fails without setting an error.  Wrong.  Its caller crashes when it
>> tries to report the error:
>> 
>>     $ qemu-system-x86_64 -nodefaults -fsdev id=foo,fsdriver=handle
>>     qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: warning: handle backend is deprecated
>>     qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: fsdev: No path specified
>>     Segmentation fault (core dumped)
>> 
>> Screwed up when commit 91cda4e8f37 (v2.12.0) converted the function to
>> Error.  Fix by calling error_setg() instead of error_report().
>> 
>> Fixes: 91cda4e8f372602795e3a2f4bd2e3adaf9f82255
>> Cc: Greg Kurz <groug@kaod.org>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Acked-by: Greg Kurz <groug@kaod.org>
>> ---
>
> Hi Markus,
>
> FWIW you had a Reviewed-by from Eric.
>
> https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg03297.html

Fixed in v4.  Thanks!

Re: [Qemu-devel] [PATCH v3 08/38] 9pfs: Fix CLI parsing crash on error
Posted by Eric Blake 7 years ago
On 10/17/18 2:15 AM, Greg Kurz wrote:
> On Tue, 16 Oct 2018 19:41:28 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
>> Calling error_report() in a function that takes an Error ** argument
>> is suspicious.  9p-handle.c's handle_parse_opts() does that, and then
>> fails without setting an error.  Wrong.  Its caller crashes when it
>> tries to report the error:
>>
>>      $ qemu-system-x86_64 -nodefaults -fsdev id=foo,fsdriver=handle
>>      qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: warning: handle backend is deprecated
>>      qemu-system-x86_64: -fsdev id=foo,fsdriver=handle: fsdev: No path specified
>>      Segmentation fault (core dumped)
>>
>> Screwed up when commit 91cda4e8f37 (v2.12.0) converted the function to
>> Error.  Fix by calling error_setg() instead of error_report().
>>
>> Fixes: 91cda4e8f372602795e3a2f4bd2e3adaf9f82255
>> Cc: Greg Kurz <groug@kaod.org>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Acked-by: Greg Kurz <groug@kaod.org>
>> ---
> 
> Hi Markus,
> 
> FWIW you had a Reviewed-by from Eric.
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg03297.html

Most likely crossed mail. Although I composed my v2 review Monday, an 
ISP outage prevented it from hitting the list until Tuesday after Markus 
had prepared v3. (And the fact that a small fire near Dallas was able to 
take out internet access for more than a million AT&T customers for more 
than 12 hours makes you appreciate how much we really rely on the 
Internet in modern society)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org