Re: [Qemu-devel] [PATCH 1/3] qapi: add visit_start_union and visit_end_union

[Paolo Bonzini]

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: address@hidden
> Reported-by: Fam Zheng <address@hidden>
> Suggested-by: Paolo Bonzini <address@hidden>
> Signed-off-by: Michael Roth <address@hidden>
> ---
>  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 <address@hidden>

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:
>