Re: [PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.04.2020 um 11:42 hat Markus Armbruster geschrieben:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> >> The next commits will put it to use.
>> >>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>   util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
>> >>   1 file changed, 56 insertions(+), 46 deletions(-)
>> >>
>> >
>> >> +static const char *get_opt_name_value(const char *params,
>> >> +                                      const char *firstname,
>> >> +                                      char **name, char **value)
>> >> +{
>> >> +    const char *p, *pe, *pc;
>> >> +
>> >> +    pe = strchr(params, '=');
>> >> +    pc = strchr(params, ',');
>> >> +
>> >> +    if (!pe || (pc && pc < pe)) {
>> >> +        /* found "foo,more" */
>> >> +        if (firstname) {
>> >> +            /* implicitly named first option */
>> >> +            *name = g_strdup(firstname);
>> >> +            p = get_opt_value(params, value);
>> >
>> > Is this correct even when params is "foo,,more"?  But...
>> >
>> >>   static void opts_do_parse(QemuOpts *opts, const char *params,
>> >>                             const char *firstname, bool prepend,
>> >>                             bool *invalidp, Error **errp)
>> >>   {
>> >> -    char *option = NULL;
>> >> -    char *value = NULL;
>> >> -    const char *p,*pe,*pc;
>> >>       Error *local_err = NULL;
>> >> +    char *option, *value;
>> >> +    const char *p;
>> >>   -    for (p = params; *p != '\0'; p++) {
>> >> -        pe = strchr(p, '=');
>> >> -        pc = strchr(p, ',');
>> >> -        if (!pe || (pc && pc < pe)) {
>> >> -            /* found "foo,more" */
>> >> -            if (p == params && firstname) {
>> >> -                /* implicitly named first option */
>> >> -                option = g_strdup(firstname);
>> >> -                p = get_opt_value(p, &value);
>> >
>> > ...in this patch, it is just code motion, so if it is a bug, it's
>> > pre-existing and worth a separate fix.
>> 
>> If @p points to "foo,,more", possibly followed by additional characters,
>> then we have pc && pc < pe, and enter this conditional.  This means that
>> 
>>     foo,,more=42
>> 
>> gets parsed as two name=value, either as
>> 
>>     name        value
>>     -----------------------
>>     @firstname  "foo,more"
>>     ""          "42"
>> 
>> if @firstname is non-null
>
> Hm, how that? get_opt_value() doesn't stop at '=', so shouldn't you get
> a single option @firstname with a value of "foo,more=42"?
>
>      name        value
>      -----------------------
>      @firstname  "foo,more=42"
>
>> or else as
>> 
>>     name        value
>>     -----------------
>>     "foo,,more" "on"
>>     ""          "42"
>
> Here get_opt_name() stops at the first comma. You get a positive flag
> "foo" and the remaing option string starts with a comma, so the
> condition will still be true for the next loop iteration. Now you get a
> positive flag with an empty name "". Finally, the rest is parsed as an
> option, so we get:
>
>      name        value
>      -----------------------
>      "foo"       "on"
>      ""          "on"
>      "more"      "42"
>
> Actually, at this point I thought I could just check, and gdb confirms
> my reading for both cases.

You're right.  I should know better than trying to predict what the
QemuOpts parser does.

> This is still crazy, but perhaps less so than the interpretations you
> suggested.

Permitting anti-social characters in the NAME part of NAME=VALUE is
crazy.  To findout which kind of crazy exactly, run the code and
observe.  Do not blindly trust the maintainer's explanations, because
he's quite possibly just as confused as everybody else.