Re: [PATCH v3 5/5] qom/object: Limit type names to alphanumerical and some few special characters

[Alistair Francis]

On Fri, Nov 17, 2023 at 9:46 PM Thomas Huth <thuth@redhat.com> wrote:
>
> QOM names currently don't have any enforced naming rules. This
> can be problematic, e.g. when they are used on the command line
> for the "-device" option (where the comma is used to separate
> properties). To avoid that such problematic type names come in
> again, let's restrict the set of acceptable characters during the
> type registration.
>
> Ideally, we'd apply here the same rules as for QAPI, i.e. all type
> names should begin with a letter, and contain only ASCII letters,
> digits, hyphen, and underscore. However, we already have so many
> pre-existing types like:
>
>     486-x86_64-cpu
>     cfi.pflash01
>     power5+_v2.1-spapr-cpu-core
>     virt-2.6-machine
>     pc-i440fx-3.0-machine
>
> ... so that we have to allow "." and "+" for now, too. While the
> dot is used in a lot of places, the "+" can fortunately be limited
> to two classes of legacy names ("power" and "Sun-UltraSparc" CPUs).
>
> We also cannot enforce the rule that names must start with a letter
> yet, since there are lot of types that start with a digit. Still,
> at least limiting the first characters to the alphanumerical range
> should be way better than nothing.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  qom/object.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/qom/object.c b/qom/object.c
> index 95c0dc8285..654e1afaf2 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -138,9 +138,50 @@ static TypeImpl *type_new(const TypeInfo *info)
>      return ti;
>  }
>
> +static bool type_name_is_valid(const char *name)
> +{
> +    const int slen = strlen(name);
> +    int plen;
> +
> +    g_assert(slen > 1);
> +
> +    /*
> +     * Ideally, the name should start with a letter - however, we've got
> +     * too many names starting with a digit already, so allow digits here,
> +     * too (except '0' which is not used yet)
> +     */
> +    if (!g_ascii_isalnum(name[0]) || name[0] == '0') {
> +        return false;
> +    }
> +
> +    plen = strspn(name, "abcdefghijklmnopqrstuvwxyz"
> +                        "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> +                        "0123456789-_.");
> +
> +    /* Allow some legacy names with '+' in it for compatibility reasons */
> +    if (name[plen] == '+') {
> +        if (plen == 6 && g_str_has_prefix(name, "power")) {
> +            /* Allow "power5+" and "power7+" CPU names*/
> +            return true;
> +        }
> +        if (plen >= 17 && g_str_has_prefix(name, "Sun-UltraSparc-I")) {
> +            /* Allow "Sun-UltraSparc-IV+" and "Sun-UltraSparc-IIIi+" */
> +            return true;
> +        }
> +    }
> +
> +    return plen == slen;
> +}
> +
>  static TypeImpl *type_register_internal(const TypeInfo *info)
>  {
>      TypeImpl *ti;
> +
> +    if (!type_name_is_valid(info->name)) {
> +        fprintf(stderr, "Registering '%s' with illegal type name\n", 
> info->name);
> +        abort();
> +    }
> +
>      ti = type_new(info);
>
>      type_table_add(ti);
> --
> 2.42.0
>
>