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