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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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

Your choice.  I certainly don't want the silly 'no' block your series.

>>> @@ -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.

Got it, thanks!

[...]