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

[Kevin Wolf]

Am 14.04.2020 um 11:42 hat Markus Armbruster geschrieben:
> Eric Blake <address@hidden> writes:
> 
> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
> >> The next commits will put it to use.
> >>
> >> Signed-off-by: Markus Armbruster <address@hidden>
> >> ---
> >>   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.

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

Kevin