From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50567) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYc11-0006Y2-67 for qemu-devel@nongnu.org; Mon, 29 Sep 2014 10:33:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XYc0w-0002xI-Q9 for qemu-devel@nongnu.org; Mon, 29 Sep 2014 10:33:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46224) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYc0w-0002vh-Iv for qemu-devel@nongnu.org; Mon, 29 Sep 2014 10:33:22 -0400 Message-ID: <54296DA9.2030801@redhat.com> Date: Mon, 29 Sep 2014 08:33:13 -0600 From: Eric Blake MIME-Version: 1.0 References: <1411165504-18198-1-git-send-email-eblake@redhat.com> <1411165504-18198-15-git-send-email-eblake@redhat.com> <87tx3qstww.fsf@blackfin.pond.sub.org> In-Reply-To: <87tx3qstww.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="cBssphWKmq9x0KOF5LQH87qEPXQinwsFT" Subject: Re: [Qemu-devel] [PATCH v4 14/19] qapi: More rigorous checking for type safety bypass List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Fam Zheng , qemu-devel@nongnu.org, wenchaoqemu@gmail.com, Luiz Capitulino This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --cBssphWKmq9x0KOF5LQH87qEPXQinwsFT Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/29/2014 02:38 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Now that we have a way to validate every type, we can also be >> stricter about enforcing that callers that want to bypass >> type safety in generated code. Prior to this patch, it didn't >> matter what value was associated with the key 'gen', but it >> looked odd that 'gen':'yes' could result in bypassing the >> generated code. These changes also enforce the changes made >> earlier in the series for documentation and consolidation of >> using '**' as the wildcard type. >=20 > Perhaps it's worth mentioning that the schema has always used 'gen': > 'no'. >=20 > That said, 'no' is silly. The natural JSON for a flag is false / true!= > Since you're touching it anyway, consider making it an optional boolean= > defaulting to false. Same for the other silly 'no': success-response. I'd love to, except that the .json parser does not yet allow literal true/false JSON keywords. Fam had a patch back in May that would fix that; maybe I'll fold in his patch to my v5 as a prereq patch: https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03916.html >> @@ -256,13 +256,13 @@ def discriminator_find_enum_define(expr): >> return find_enum(discriminator_type) >> >> def check_type(expr_info, source, data, allow_array =3D False, >> - allowed_names =3D [], allow_dict =3D True): >> + allowed_names =3D [], allow_dict =3D True, allow_star = =3D False): >> global all_types >> >> if data is None: >> return >> >> - if data =3D=3D '**': >> + if allow_star and data =3D=3D '**': >> return [1] >> >> # Check if array type for data is okay >> @@ -278,6 +278,10 @@ def check_type(expr_info, source, data, allow_arr= ay =3D False, >> >> # Check if type name for data is okay >> if isinstance(data, basestring): >> + if data =3D=3D '**': >> + raise QAPIExprError(expr_info, >> + "%s uses '**' but did not request 'ge= n':'no'" >> + % source) >> if not data in all_types: >> raise QAPIExprError(expr_info, >> "%s references unknown type '%s'" >=20 > I'm blind: I can't see where this error gets suppressed when we have > 'gen': 'no'. 'gen':'no' triggers the caller to pass allow_star=3DTrue to check_type [2]; then at point [1] we short-circuit and exit if '**' was passed. Therefore, if we get here and still have '**', then allow_star is still false, which means 'gen':'no' was not passed and it is user error. >=20 >> @@ -299,24 +303,27 @@ def check_type(expr_info, source, data, allow_ar= ray =3D False, >> check_type(expr_info, "member '%s' of %s" % (key, source), va= lue, >> allow_array=3DTrue, >> allowed_names=3D['built-in', 'union', 'struct', 'e= num'], >> - allow_dict=3DTrue) >> + allow_dict=3DTrue, allow_star=3Dallow_star) >> >=20 > allow_star applies recursively. Correct, because... >=20 >> def check_command(expr, expr_info): >> global commands >> name =3D expr['command'] >> + allow_star =3D expr.has_key('gen') >> + [2] The other trick to note here is that this only checks if 'gen' is a key; but at [3], which is run earlier, we further required that 'gen' can only appear if it has value 'no'. >> if name in commands: >> raise QAPIExprError(expr_info, >> "command '%s' is already defined" % name)= >> commands.append(name) >> check_type(expr_info, "'data' for command '%s'" % name, >> expr.get('data'), allow_array=3DTrue, >> - allowed_names=3D['union', 'struct']) >> + allowed_names=3D['union', 'struct'], allow_star=3Dallo= w_star) >> check_type(expr_info, "'base' for command '%s'" % name, >> expr.get('base'), allowed_names=3D['struct'], >> allow_dict=3DFalse) >> check_type(expr_info, "'returns' for command '%s'" % name, >> expr.get('returns'), allow_array=3DTrue, >> - allowed_names=3D['built-in', 'union', 'struct', 'enum'= ]) >> + allowed_names=3D['built-in', 'union', 'struct', 'enum'= ], >> + allow_star=3Dallow_star) >> >=20 > ... it applies exactly to a command's 'data' and 'returns' when it has > 'gen': 'no'. Good. >=20 >> def check_event(expr, expr_info): >> global events >> @@ -450,6 +457,10 @@ def check_keys(expr_elem, meta, required, optiona= l=3D[]): >> raise QAPIExprError(info, >> "%s '%s' has unknown key '%s'" >> % (meta, name, key)) >> + if (key =3D=3D 'gen' or key =3D=3D 'success-response') and va= lue !=3D 'no': >> + raise QAPIExprError(info, >> + "'%s' of %s '%s' should only have val= ue 'no'" >> + % (key, meta, name)) [3] >> for key in required: >> if not expr.has_key(key): >> raise QAPIExprError(info, > [...] >=20 >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --cBssphWKmq9x0KOF5LQH87qEPXQinwsFT Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg iQEcBAEBCAAGBQJUKW2pAAoJEKeha0olJ0Nq0wQH/2b2uEECiasIRCFEkWH6aLo9 jm5KbbsHfYvCLkarhmLy2dyENMlP+Hzq2Catf7rn1IymqXV6brvlkMhQSABYhd84 sn++imSS8gDmuWod1RcG7MYlqo91OHuZejoc9tpcSvZTsIDv+HRhpnZAnv8WtgCs Hs81P+pVt4waz1/C3NUke2Q/PKBSnZhGrOc9CZhhXE/r+EfBoMRsPhss9CEjBjP9 HrAEtwmXr6h+43wT+b+Y+kOPZx8yz4e54iTLDYKQhxm1JOHJDfXUddcU61TZXiuD pF1Qy+Ry6Kp9EWehUnBItsiK5we86h6fs/YlAe/LL313tOASYxmI1da9XT7A4BM= =QLVz -----END PGP SIGNATURE----- --cBssphWKmq9x0KOF5LQH87qEPXQinwsFT--