From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33422) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XSSsH-0000Fi-Ew for qemu-devel@nongnu.org; Fri, 12 Sep 2014 11:35:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XSSs9-0001cX-Nn for qemu-devel@nongnu.org; Fri, 12 Sep 2014 11:35:01 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:58912) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XSSs9-0001cG-I4 for qemu-devel@nongnu.org; Fri, 12 Sep 2014 11:34:53 -0400 Received: from /spool/local by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 12 Sep 2014 09:34:52 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <5412C828.2070004@redhat.com> References: <1410477659-9163-1-git-send-email-mdroth@linux.vnet.ibm.com> <1410477659-9163-2-git-send-email-mdroth@linux.vnet.ibm.com> <5412C828.2070004@redhat.com> Message-ID: <20140912153447.19243.81596@loki> Date: Fri, 12 Sep 2014 10:34:47 -0500 Subject: Re: [Qemu-devel] [PATCH 1/3] qapi: add visit_start_union and visit_end_union List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: lcapitulino@redhat.com, famz@redhat.com, qemu-stable@nongnu.org, armbru@redhat.com 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-stable@nongnu.org > > Reported-by: Fam Zheng > > Suggested-by: Paolo Bonzini > > Signed-off-by: Michael Roth > > --- > > 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, Err= or **errp); > > /* visit_type_size() falls back to (*type_uint64)() if type_size i= s unset */ > > void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Err= or **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, Erro= r **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 > = > 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 =3D 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 =3D NULL; > > + visit_end_union(m, !!(*obj)->data, &err); > > + error_propagate(errp, err); > > + err =3D NULL; > > } > > visit_end_struct(m, &err); > > out: > >