[v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option

[RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option

Posted by Philippe Mathieu-Daudé 2 years, 5 months ago

When not all fields of the --global option are provided,
QEMU might crash:

  $ qemu-system-x86_64 -global driver=isa-fdc
  qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
  string_input_visitor_new: Assertion `str' failed.
  Aborted (core dumped)

Fix by only allowing --global with all 3 fields:

  $ qemu-system-x86_64 -global driver=isa-fdc
  Invalid 'global' option format. It must be provided as:
    --global <driver>.<property>=<value>

Reported-by: Thomas Huth <thuth@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v3: Change qemu_global_option (Markus)

Supersedes: <20211119122911.365036-1-philmd@redhat.com>
---
 softmmu/qdev-monitor.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 01f3834db57..558272b147c 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
         qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
         return 0;
     }
+    printf("Invalid 'global' option format. It must be provided as:\n");
+    printf("  --global <driver>.<property>=<value>\n");
 
-    opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
-    if (!opts) {
-        return -1;
-    }
-
-    return 0;
+    return -1;
 }
 
 bool qmp_command_available(const QmpCommand *cmd, Error **errp)
-- 
2.31.1

Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option

Posted by BALATON Zoltan 2 years, 5 months ago

On Fri, 19 Nov 2021, Philippe Mathieu-Daudé wrote:
> When not all fields of the --global option are provided,
> QEMU might crash:
>
>  $ qemu-system-x86_64 -global driver=isa-fdc
>  qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
>  string_input_visitor_new: Assertion `str' failed.
>  Aborted (core dumped)
>
> Fix by only allowing --global with all 3 fields:
>
>  $ qemu-system-x86_64 -global driver=isa-fdc
>  Invalid 'global' option format. It must be provided as:
>    --global <driver>.<property>=<value>
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v3: Change qemu_global_option (Markus)
>
> Supersedes: <20211119122911.365036-1-philmd@redhat.com>
> ---
> softmmu/qdev-monitor.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 01f3834db57..558272b147c 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
>         qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
>         return 0;
>     }
> +    printf("Invalid 'global' option format. It must be provided as:\n");
> +    printf("  --global <driver>.<property>=<value>\n");

Should these be something else tnan plain printf? (Such as qemu_prinf or 
qdev_printf or similar? Not sure how these work but plain printf in QEMU 
is usually not what's meant.)

Regards,
BALATON Zoltan

> -    opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
> -    if (!opts) {
> -        return -1;
> -    }
> -
> -    return 0;
> +    return -1;
> }
>
> bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>

Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option

Posted by Philippe Mathieu-Daudé 2 years, 5 months ago

On 11/19/21 19:46, BALATON Zoltan wrote:
> On Fri, 19 Nov 2021, Philippe Mathieu-Daudé wrote:
>> When not all fields of the --global option are provided,
>> QEMU might crash:
>>
>>  $ qemu-system-x86_64 -global driver=isa-fdc
>>  qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
>>  string_input_visitor_new: Assertion `str' failed.
>>  Aborted (core dumped)
>>
>> Fix by only allowing --global with all 3 fields:
>>
>>  $ qemu-system-x86_64 -global driver=isa-fdc
>>  Invalid 'global' option format. It must be provided as:
>>    --global <driver>.<property>=<value>
>>
>> Reported-by: Thomas Huth <thuth@redhat.com>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v3: Change qemu_global_option (Markus)
>>
>> Supersedes: <20211119122911.365036-1-philmd@redhat.com>
>> ---
>> softmmu/qdev-monitor.c | 9 +++------
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> index 01f3834db57..558272b147c 100644
>> --- a/softmmu/qdev-monitor.c
>> +++ b/softmmu/qdev-monitor.c
>> @@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
>>         qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
>>         return 0;
>>     }
>> +    printf("Invalid 'global' option format. It must be provided as:\n");
>> +    printf("  --global <driver>.<property>=<value>\n");
> 
> Should these be something else tnan plain printf? (Such as qemu_prinf or
> qdev_printf or similar? Not sure how these work but plain printf in QEMU
> is usually not what's meant.)

I thought so first, but qemu_opts_print_help() calls plain printf()...

> Regards,
> BALATON Zoltan
> 
>> -    opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
>> -    if (!opts) {
>> -        return -1;
>> -    }

Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option

Posted by Markus Armbruster 2 years, 5 months ago

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> When not all fields of the --global option are provided,
> QEMU might crash:
>
>   $ qemu-system-x86_64 -global driver=isa-fdc
>   qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
>   string_input_visitor_new: Assertion `str' failed.
>   Aborted (core dumped)
>
> Fix by only allowing --global with all 3 fields:
>
>   $ qemu-system-x86_64 -global driver=isa-fdc
>   Invalid 'global' option format. It must be provided as:
>     --global <driver>.<property>=<value>
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v3: Change qemu_global_option (Markus)
>
> Supersedes: <20211119122911.365036-1-philmd@redhat.com>
> ---
>  softmmu/qdev-monitor.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 01f3834db57..558272b147c 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
>          qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
>          return 0;
>      }
> +    printf("Invalid 'global' option format. It must be provided as:\n");
> +    printf("  --global <driver>.<property>=<value>\n");
>  
> -    opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
> -    if (!opts) {
> -        return -1;
> -    }
> -
> -    return 0;
> +    return -1;
>  }
>  
>  bool qmp_command_available(const QmpCommand *cmd, Error **errp)

This drops a documented part of the external interface:

    $ qemu-system-x86_64 -help | grep -C 1 global
                    i.e. -set drive.$id.file=/path/to/image
    -global driver.property=value
--> -global driver=driver,property=property,value=value
                    set a global default for a driver property
    -boot [order=drives][,once=drives][,menu=on|off]

It goes back to commit 3751d7c43f "vl: allow full-blown QemuOpts syntax
for -global", v2.4.0.

The appropriate fix is to check @opts for presence of all three
parameters.

Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option

Posted by Daniel P. Berrangé 2 years, 5 months ago

On Sat, Nov 20, 2021 at 07:53:20AM +0100, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
> > When not all fields of the --global option are provided,
> > QEMU might crash:
> >
> >   $ qemu-system-x86_64 -global driver=isa-fdc
> >   qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
> >   string_input_visitor_new: Assertion `str' failed.
> >   Aborted (core dumped)
> >
> > Fix by only allowing --global with all 3 fields:
> >
> >   $ qemu-system-x86_64 -global driver=isa-fdc
> >   Invalid 'global' option format. It must be provided as:
> >     --global <driver>.<property>=<value>
> >
> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> > v3: Change qemu_global_option (Markus)
> >
> > Supersedes: <20211119122911.365036-1-philmd@redhat.com>
> > ---
> >  softmmu/qdev-monitor.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> > index 01f3834db57..558272b147c 100644
> > --- a/softmmu/qdev-monitor.c
> > +++ b/softmmu/qdev-monitor.c
> > @@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
> >          qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
> >          return 0;
> >      }
> > +    printf("Invalid 'global' option format. It must be provided as:\n");
> > +    printf("  --global <driver>.<property>=<value>\n");
> >  
> > -    opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
> > -    if (!opts) {
> > -        return -1;
> > -    }
> > -
> > -    return 0;
> > +    return -1;
> >  }
> >  
> >  bool qmp_command_available(const QmpCommand *cmd, Error **errp)
> 
> This drops a documented part of the external interface:
> 
>     $ qemu-system-x86_64 -help | grep -C 1 global
>                     i.e. -set drive.$id.file=/path/to/image
>     -global driver.property=value
> --> -global driver=driver,property=property,value=value
>                     set a global default for a driver property

This doc makes it look like the two syntaxes are functionally
equivalent, but it seems that's not quite the case.

libvirt uses the driver.propert=value syntax for everything
except one case

  -global driver=cfi.pflash01,property=secure,value=on

for that one if we try to use

  -global cfi.pflash01.secure=on

it complains

  qemu-system-x86_64: warning: global cfi.pflash01.secure has invalid class name

what's going on here ?

>     -boot [order=drives][,once=drives][,menu=on|off]
> 
> It goes back to commit 3751d7c43f "vl: allow full-blown QemuOpts syntax
> for -global", v2.4.0.
> 
> The appropriate fix is to check @opts for presence of all three
> parameters.
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option

Posted by Markus Armbruster 2 years, 5 months ago

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Sat, Nov 20, 2021 at 07:53:20AM +0100, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> 
>> > When not all fields of the --global option are provided,
>> > QEMU might crash:
>> >
>> >   $ qemu-system-x86_64 -global driver=isa-fdc
>> >   qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
>> >   string_input_visitor_new: Assertion `str' failed.
>> >   Aborted (core dumped)
>> >
>> > Fix by only allowing --global with all 3 fields:
>> >
>> >   $ qemu-system-x86_64 -global driver=isa-fdc
>> >   Invalid 'global' option format. It must be provided as:
>> >     --global <driver>.<property>=<value>
>> >
>> > Reported-by: Thomas Huth <thuth@redhat.com>
>> > Suggested-by: Markus Armbruster <armbru@redhat.com>
>> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
>> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> > ---
>> > v3: Change qemu_global_option (Markus)
>> >
>> > Supersedes: <20211119122911.365036-1-philmd@redhat.com>
>> > ---
>> >  softmmu/qdev-monitor.c | 9 +++------
>> >  1 file changed, 3 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> > index 01f3834db57..558272b147c 100644
>> > --- a/softmmu/qdev-monitor.c
>> > +++ b/softmmu/qdev-monitor.c
>> > @@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
>> >          qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
>> >          return 0;
>> >      }
>> > +    printf("Invalid 'global' option format. It must be provided as:\n");
>> > +    printf("  --global <driver>.<property>=<value>\n");
>> >  
>> > -    opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
>> > -    if (!opts) {
>> > -        return -1;
>> > -    }
>> > -
>> > -    return 0;
>> > +    return -1;
>> >  }
>> >  
>> >  bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>> 
>> This drops a documented part of the external interface:
>> 
>>     $ qemu-system-x86_64 -help | grep -C 1 global
>>                     i.e. -set drive.$id.file=/path/to/image
>>     -global driver.property=value
>> --> -global driver=driver,property=property,value=value
>>                     set a global default for a driver property
>
> This doc makes it look like the two syntaxes are functionally
> equivalent, but it seems that's not quite the case.
>
> libvirt uses the driver.propert=value syntax for everything
> except one case
>
>   -global driver=cfi.pflash01,property=secure,value=on
>
> for that one if we try to use
>
>   -global cfi.pflash01.secure=on
>
> it complains
>
>   qemu-system-x86_64: warning: global cfi.pflash01.secure has invalid class name
>
> what's going on here ?

Off-the-cuff guess: cfi.pflash01.secure=on gets parsed as

    driver=cfi
    property=pflash01.secure
    value=on

Once again our "anything goes" attitude to naming wastes us time and
thus money.

In contrast, QAPI restricts names to "only ASCII letters, digits,
hyphen, and underscore" (see docs/devel/qapi-code-gen.rst section Naming
rules and reserved names).

Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option

Posted by Paolo Bonzini 2 years, 5 months ago

On 11/22/21 15:32, Markus Armbruster wrote:
>>    qemu-system-x86_64: warning: global cfi.pflash01.secure has invalid class name
>>
>> what's going on here ?
> Off-the-cuff guess: cfi.pflash01.secure=on gets parsed as
> 
>      driver=cfi
>      property=pflash01.secure
>      value=on
> 
> Once again our "anything goes" attitude to naming wastes us time and
> thus money.

I'd blame more the sscanf parsing.  Anyway, -global 
driver=...,property=...,value=... works just fine in all cases, it's 
just more verbose---and it might even be easier to use for Libvirt, if 
it can use its usual QemuOpts-building facilities.

Anyhow, this patch breaks existing clients, as pointed out by Markus.

Paolo

> In contrast, QAPI restricts names to "only ASCII letters, digits,
> hyphen, and underscore" (see docs/devel/qapi-code-gen.rst section Naming
> rules and reserved names).