Re: [Qemu-devel] [PATCH v22 04/25] improve assertion in qemu_opt_get functions

[Eric Blake]

On 03/10/2014 01:31 AM, Chunyan Liu wrote:
> In qemu_opt_set functions, if desc doen't exist but opts_accepts_any is true, 
> it

s/doen't/doesn't/

I mentioned the same problem against v20.  It is very depressing when
review comments are not addressed.

> won't report error, but can still alloc an opt for the option and save it.
> However, after that, when doing qemu_opt_get, this option could be found in 
> opts
> but opt->desc is NULL. This is correct, should not be treated as error.
> 
> This patch would fix vvfat issue after changing to QemuOpts.
> 
> Signed-off-by: Chunyan Liu <address@hidden>
> ---
>  util/qemu-option.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index c7639e8..df79235 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -603,7 +603,9 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, 
> bool defval)
>          }
>          return defval;
>      }
> -    assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL);
> +    if (opt->desc) {
> +        assert(opt->desc->type == QEMU_OPT_BOOL);
> +    }
>      return opt->value.boolean;

I'm not sure I like this.  opt->value is a union, but opt_set() does NOT
populate the union when opts_accepts_any() fails.  Previously, we were
using opt->desc->type as the discriminator for which branch of the union
is valid.  But with your patch, if an option was set as a string, but
then queried as a boolean, we may be reading bogus contents from the
union.  Or even worse, if someone sets the uint member of the union to
0x100000000 via qemu_opt_set_number(), then later calls
qemu_opt_get_bool, the boolean member _might_ read as true on some
platforms and false on others, depending on things such as host endianness.

How is vvfat broken without this patch?  That is, what specific option
are you setting without specifying its type, that later triggers the
assertion when you try to get the option via a specific type?

I'm wondering if the fix should look more like:

if (opt->desc) {
    assert(opt->desc->type == QEMU_OPT_BOOL);
    return opt->value.boolean;
} else {
    code to parse opt->str
}

so that you are not dereferencing an undefined state of the union.

> @@ -625,7 +627,9 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const char 
> *name, uint64_t defval)
>          }
>          return defval;
>      }
> -    assert(opt->desc && opt->desc->type == QEMU_OPT_NUMBER);
> +    if (opt->desc) {
> +        assert(opt->desc->type == QEMU_OPT_NUMBER);
> +    }
>      return opt->value.uint;
>  }
>  
> @@ -645,7 +649,9 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char 
> *name, uint64_t defval)
>          }
>          return defval;
>      }
> -    assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE);
> +    if (opt->desc) {
> +        assert(opt->desc->type == QEMU_OPT_SIZE);
> +    }
>      return opt->value.uint;

Same problem in these two spots.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature