Il 12/09/2014 01:20, Michael Roth ha scritto:
> In some cases an input visitor might bail out on filling out a
> struct for various reasons, such as missing fields when running
> in strict mode. In the case of a QAPI Union type, this may lead
> to cases where the .kind field which encodes the union type
> is uninitialized. Subsequently, other visitors, such as the
> dealloc visitor, may use this .kind value as if it were
> initialized, leading to assumptions about the union type which
> in this case may lead to segfaults. For example, freeing an
> integer value.
>
> However, we can generally rely on the fact that the always-present
> .data void * field that we generate for these union types will
> always be NULL in cases where .kind is uninitialized (at least,
> there shouldn't be a reason where we'd do this purposefully).
>
> So pass this information on to Visitor implementation via these
> optional start_union/end_union interfaces so this information
> can be used to guard against the situation above. We will make
> use of this information in a subsequent patch for the dealloc
> visitor.
>
> Cc: qemu-sta...@nongnu.org
> Reported-by: Fam Zheng <f...@redhat.com>
> Suggested-by: Paolo Bonzini <pbonz...@redhat.com>
> Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> ---
> include/qapi/visitor-impl.h | 2 ++
> include/qapi/visitor.h | 2 ++
> qapi/qapi-visit-core.c | 15 +++++++++++++++
> scripts/qapi-visit.py | 6 ++++++
> 4 files changed, 25 insertions(+)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index ecc0183..09bb0fd 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -55,6 +55,8 @@ struct Visitor
> void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error
> **errp);
> /* visit_type_size() falls back to (*type_uint64)() if type_size is
> unset */
> void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error
> **errp);
> + bool (*start_union)(Visitor *v, bool data_present, Error **errp);
> + void (*end_union)(Visitor *v, bool data_present, Error **errp);
> };
>
> void input_type_enum(Visitor *v, int *obj, const char *strings[],
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 4a0178f..5934f59 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -58,5 +58,7 @@ void visit_type_size(Visitor *v, uint64_t *obj, const char
> *name, Error **errp);
> void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
> void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
> void visit_type_number(Visitor *v, double *obj, const char *name, Error
> **errp);
> +bool visit_start_union(Visitor *v, bool data_present, Error **errp);
> +void visit_end_union(Visitor *v, bool data_present, Error **errp);
>
> #endif
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 55f8d40..b66b93a 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -58,6 +58,21 @@ void visit_end_list(Visitor *v, Error **errp)
> v->end_list(v, errp);
> }
>
> +bool visit_start_union(Visitor *v, bool data_present, Error **errp)
> +{
> + if (v->start_union) {
> + return v->start_union(v, data_present, errp);
> + }
> + return true;
> +}
Should output visitors set an error, or even abort, if the data is not
present, thus avoiding a NULL-pointer dereference later on?
But I guess this is really just cosmetic, so you can add my
Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>
for the whole series.
Paolo
> +void visit_end_union(Visitor *v, bool data_present, Error **errp)
> +{
> + if (v->end_union) {
> + v->end_union(v, data_present, errp);
> + }
> +}
> +
> void visit_optional(Visitor *v, bool *present, const char *name,
> Error **errp)
> {
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index c129697..4520da9 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -357,6 +357,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj,
> const char *name, Error **e
> if (err) {
> goto out_obj;
> }
> + if (!visit_start_union(m, !!(*obj)->data, &err)) {
> + goto out_obj;
> + }
> switch ((*obj)->kind) {
> ''',
> disc_type = disc_type,
> @@ -385,6 +388,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj,
> const char *name, Error **e
> out_obj:
> error_propagate(errp, err);
> err = NULL;
> + visit_end_union(m, !!(*obj)->data, &err);
> + error_propagate(errp, err);
> + err = NULL;
> }
> visit_end_struct(m, &err);
> out:
>
