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

[Thomas Huth <thuth@redhat.com>]

On 17/11/2023 13.25, Philippe Mathieu-Daudé wrote:
> On 17/11/23 12:44, Thomas Huth 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>
>> ---
>>   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);
> 
> Shouldn't we use error_report() instead of fprintf()? Regardless,

It doesn't work here yet - the type registration happens so early that we 
cannot use error_report() here yet.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!

  Thomas