| Message ID | 1394436721-21812-5-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
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 <cyliu@suse.com> > --- > 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.
2014-03-11 5:44 GMT+08:00 Eric Blake <eblake@redhat.com>: > 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 <cyliu@suse.com> > > --- > > 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 > >
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; } @@ -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; }
In qemu_opt_set functions, if desc doen't exist but opts_accepts_any is true, it 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 <cyliu@suse.com> --- util/qemu-option.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)