Re: [Qemu-devel] [PATCH v5 21/28] qapi: Require valid names

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> Previous commits demonstrated that the generator overlooked various
> bad naming situations:
> - types, commands, and events need a valid name
> - union and alternate branches cannot be marked optional
>
> The set of valid names includes [a-zA-Z0-9._-] (where '.' is
> useful only in downstream extensions).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  scripts/qapi.py                                    | 57 ++++++++++++++++------
>  tests/qapi-schema/bad-ident.err                    |  1 +
>  tests/qapi-schema/bad-ident.exit                   |  2 +-
>  tests/qapi-schema/bad-ident.json                   |  2 +-
>  tests/qapi-schema/bad-ident.out                    |  3 --
>  tests/qapi-schema/flat-union-bad-discriminator.err |  2 +-
>  .../flat-union-optional-discriminator.err          |  1 +
>  .../flat-union-optional-discriminator.exit         |  2 +-
>  .../flat-union-optional-discriminator.json         |  2 +-
>  .../flat-union-optional-discriminator.out          |  5 --
>  tests/qapi-schema/union-optional-branch.err        |  1 +
>  tests/qapi-schema/union-optional-branch.exit       |  2 +-
>  tests/qapi-schema/union-optional-branch.json       |  2 +-
>  tests/qapi-schema/union-optional-branch.out        |  3 --
>  14 files changed, 53 insertions(+), 32 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index c42683b..ed5385a 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -15,6 +15,7 @@ import re
>  from ordereddict import OrderedDict
>  import os
>  import sys
> +import string
>
>  builtin_types = {
>      'str':      'QTYPE_QSTRING',
> @@ -276,8 +277,27 @@ def discriminator_find_enum_define(expr):
>
>      return find_enum(discriminator_type)
>
> +valid_characters = set(string.ascii_letters + string.digits + '.' + '-' + '_')

strings.ascii_letters depends on the locale...

> +def check_name(expr_info, source, name, allow_optional = False):
> +    membername = name
> +
> +    if not isinstance(name, str):
> +        raise QAPIExprError(expr_info,
> +                            "%s requires a string name" % source)
> +    if name == '**':
> +        return

Doesn't this permit '**' anywhere, not just as pseudo-type in command
arguments and results?

> +    if name.startswith('*'):
> +        membername = name[1:]
> +        if not allow_optional:
> +            raise QAPIExprError(expr_info,
> +                                "%s does not allow optional name '%s'"
> +                                % (source, name))
> +    if not set(membername) <= valid_characters:

... so this check would break if we called locale.setlocale() in this
program.  While I don't think we need to worry about it, I think you
could just as well use something like

    valid_name = re.compile(r"^[A-Za-z0-9-._]+$")

    if not valid_name.match(membername):

> +        raise QAPIExprError(expr_info,
> +                            "%s uses invalid name '%s'" % (source, name))
> +
>  def check_type(expr_info, source, data, allow_array = False,
> -               allowed_metas = [], allow_dict = True):
> +               allowed_metas = [], allow_dict = True, allow_optional = False):
>      global all_names
>
>      if data is None:
> @@ -317,21 +337,23 @@ def check_type(expr_info, source, data, allow_array = False,
>          raise QAPIExprError(expr_info,
>                              "%s should be a type name" % source)
>      for (key, value) in data.items():
> +        check_name(expr_info, "Member of %s" % source, key,
> +                   allow_optional=allow_optional)
>          check_type(expr_info, "Member '%s' of %s" % (key, source), value,
>                     allow_array=True,
>                     allowed_metas=['built-in', 'union', 'alternate', 'struct',
>                                    'enum'],
> -                   allow_dict=True)
> +                   allow_dict=True, allow_optional=True)
>
>  def check_command(expr, expr_info):
>      name = expr['command']
>      check_type(expr_info, "'data' for command '%s'" % name,
>                 expr.get('data'),
> -               allowed_metas=['union', 'struct'])
> +               allowed_metas=['union', 'struct'], allow_optional=True)
>      check_type(expr_info, "'returns' for command '%s'" % name,
>                 expr.get('returns'), allow_array=True,
>                 allowed_metas=['built-in', 'union', 'alternate', 'struct',
> -                              'enum'])
> +                              'enum'], allow_optional=True)
>
>  def check_event(expr, expr_info):
>      global events
> @@ -345,7 +367,8 @@ def check_event(expr, expr_info):
>                              % name)
>      events.append(name)
>      check_type(expr_info, "'data' for event '%s'" % name,
> -               expr.get('data'), allowed_metas=['union', 'struct'])
> +               expr.get('data'), allowed_metas=['union', 'struct'],
> +               allow_optional=True)
>      if params:
>          for argname, argentry, optional, structured in parse_args(params):
>              if structured:
> @@ -385,12 +408,10 @@ def check_union(expr, expr_info):
>                                  "Base '%s' is not a valid base type"
>                                  % base)
>
> -        # The value of member 'discriminator' must name a member of the
> -        # base type.
> -        if not isinstance(discriminator, str):
> -            raise QAPIExprError(expr_info,
> -                                "Flat union '%s' must have a string "
> -                                "discriminator field" % name)
> +        # The value of member 'discriminator' must name a non-optional
> +        # member of the base type.
> +        check_name(expr_info, "Discriminator of flat union '%s'" % name,
> +                   discriminator)
>          discriminator_type = base_fields.get(discriminator)
>          if not discriminator_type:
>              raise QAPIExprError(expr_info,

What happens when I try 'discriminator': '**'?

> @@ -406,6 +427,8 @@ def check_union(expr, expr_info):
>
>      # Check every branch
>      for (key, value) in members.items():
> +        check_name(expr_info, "Member of union '%s'" % name, key)
> +
>          # Each value must name a known type
>          check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
>                     value, allow_array=True,
> @@ -439,6 +462,8 @@ def check_alternate(expr, expr_info):
>
>      # Check every branch
>      for (key, value) in members.items():
> +        check_name(expr_info, "Member of alternate '%s'" % name, key)
> +
>          # Check for conflicts in the generated enum
>          c_key = _generate_enum_string(key)
>          if c_key in values:
> @@ -485,7 +510,8 @@ def check_struct(expr, expr_info):
>      name = expr['type']
>      members = expr['data']
>
> -    check_type(expr_info, "'data' for type '%s'" % name, members)
> +    check_type(expr_info, "'data' for type '%s'" % name, members,
> +               allow_optional=True)
>      check_type(expr_info, "'base' for type '%s'" % name, expr.get('base'),
>                 allowed_metas=['struct'], allow_dict=False)
>
> @@ -676,8 +702,11 @@ def type_name(name):
>          return c_list_type(name[0])
>      return name
>
> -def add_name(name, info, meta, implicit = False):
> +def add_name(name, info, meta, implicit = False, source = None):
>      global all_names
> +    if not source:
> +        source = "'%s'" % meta
> +    check_name(info, source, name)
>      if name in all_names:
>          raise QAPIExprError(info,
>                              "%s '%s' is already defined"
> @@ -691,7 +720,7 @@ def add_name(name, info, meta, implicit = False):
>  def add_struct(definition, info):
>      global struct_types
>      name = definition['type']
> -    add_name(name, info, 'struct')
> +    add_name(name, info, 'struct', source="'type'")
>      struct_types.append(definition)
>
>  def find_struct(name):
[...]