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

[Michael Roth]

Quoting Paolo Bonzini (2014-09-12 05:17:12)
> 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?

I think there's at least one corner case where this might cause issues:

{ 'union': 'UserDefUnion',
  'base': 'UserDefZero',
  'data': { 'a' : 'int', 'b' : 'UserDefB' } }

If UserDefUnion.a is 0, UserDefUnion.data will cast it to a NULL value and
cause the output visitor to bail, when really it should just be left to
continue on serializing the integer.

To guard against that sort of thing I think we'd need a way to verify .kind
field is initialized, so that kind of brings us back to original problem.
But if we do decide to audit or change visitors/users to encode this
information in .kind it would be fairly simply to extend
visit_start_union/visit_end_union to pass that information along as well.

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