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

[Chunyan Liu]

2014-03-11 5:44 GMT+08:00 Eric Blake <address@hidden>:

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?

Well, now I think it caused by drv (vvfat.c) and proto_drv (raw-posix.c) not
consistent, one is using QemuOpts, the other is using QEMUOptionParameter.
That will cause create_opts became NULL, then when passing a size option,
qemu_opt_set_size is OK, but later qemu_opt_get_size will segment fault.

After solving the drv/proto_drv consistent issue, this problem won't happen.

So, in this patch series, this place could not be changed.

( But with opts_accept_any, I still think this place may bring problem some time
 in future.)

 

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