Re: [Qemu-devel] [PATCH v4 14/19] qapi: More rigorous checking for type safety bypass

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5302 bytes --]

On 09/29/2014 02:38 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]