[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 14/19] qapi: More rigorous checking for type
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v4 14/19] qapi: More rigorous checking for type safety bypass |
Date: |
Mon, 29 Sep 2014 08:33:13 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 |
On 09/29/2014 02:38 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
>
>> 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.
>
> Perhaps it's worth mentioning that the schema has always used 'gen':
> 'no'.
>
> 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 = False,
>> - allowed_names = [], allow_dict = True):
>> + allowed_names = [], allow_dict = True, allow_star = False):
>> global all_types
>>
>> if data is None:
>> return
>>
>> - if data == '**':
>> + if allow_star and data == '**':
>> return
[1]
>>
>> # Check if array type for data is okay
>> @@ -278,6 +278,10 @@ def check_type(expr_info, source, data, allow_array =
>> False,
>>
>> # Check if type name for data is okay
>> if isinstance(data, basestring):
>> + if data == '**':
>> + raise QAPIExprError(expr_info,
>> + "%s uses '**' but did not request
>> 'gen':'no'"
>> + % source)
>> if not data in all_types:
>> raise QAPIExprError(expr_info,
>> "%s references unknown type '%s'"
>
> 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=True 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.
>
>> @@ -299,24 +303,27 @@ def check_type(expr_info, source, data, allow_array =
>> False,
>> check_type(expr_info, "member '%s' of %s" % (key, source), value,
>> allow_array=True,
>> allowed_names=['built-in', 'union', 'struct', 'enum'],
>> - allow_dict=True)
>> + allow_dict=True, allow_star=allow_star)
>>
>
> allow_star applies recursively. Correct, because...
>
>> def check_command(expr, expr_info):
>> global commands
>> name = expr['command']
>> + allow_star = 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=True,
>> - allowed_names=['union', 'struct'])
>> + allowed_names=['union', 'struct'], allow_star=allow_star)
>> check_type(expr_info, "'base' for command '%s'" % name,
>> expr.get('base'), allowed_names=['struct'],
>> allow_dict=False)
>> check_type(expr_info, "'returns' for command '%s'" % name,
>> expr.get('returns'), allow_array=True,
>> - allowed_names=['built-in', 'union', 'struct', 'enum'])
>> + allowed_names=['built-in', 'union', 'struct', 'enum'],
>> + allow_star=allow_star)
>>
>
> ... it applies exactly to a command's 'data' and 'returns' when it has
> 'gen': 'no'. Good.
>
>> def check_event(expr, expr_info):
>> global events
>> @@ -450,6 +457,10 @@ def check_keys(expr_elem, meta, required, optional=[]):
>> raise QAPIExprError(info,
>> "%s '%s' has unknown key '%s'"
>> % (meta, name, key))
>> + if (key == 'gen' or key == 'success-response') and value != 'no':
>> + raise QAPIExprError(info,
>> + "'%s' of %s '%s' should only have value
>> 'no'"
>> + % (key, meta, name))
[3]
>> for key in required:
>> if not expr.has_key(key):
>> raise QAPIExprError(info,
> [...]
>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v4 17/19] qapi: Drop inline subtype in query-version, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 01/19] qapi: Consistent whitespace in tests/Makefile, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 16/19] qapi: Drop tests for inline subtypes, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 02/19] qapi: Ignore files created during make check, Eric Blake, 2014/09/19
Re: [Qemu-devel] [PATCH v4 00/19] drop qapi nested structs, Eric Blake, 2014/09/26