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::hotplug-handler
    aspeed.i2c.slave::vmstate-if
    pc-i440fx-3.0-machine::nmi

... so that we have to allow ".", ":" and "+" for now, too, and
we unfortunately even cannot enforce the rule that names must start
with a letter yet. Still, having at least some rules enforced here
should be way better than nothing.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qom/object.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/qom/object.c b/qom/object.c
index 95c0dc8285..8ff85e0239 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -101,6 +101,20 @@ static TypeImpl *type_table_lookup(const char *name)
     return g_hash_table_lookup(type_table_get(), name);
 }
 
+static bool type_name_is_valid(const char *name)
+{
+    const int slen = strlen(name);
+
+    for (int i = 0; i < slen; i++) {
+        if (!g_ascii_isalnum (name[i]) && name[i] != '-' && name[i] != '_' &&
+            name[i] != '.' && name[i] != ':' && name[i] != '+') {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 static TypeImpl *type_new(const TypeInfo *info)
 {
     TypeImpl *ti = g_malloc0(sizeof(*ti));
@@ -113,6 +127,11 @@ static TypeImpl *type_new(const TypeInfo *info)
         abort();
     }
 
+    if (!type_name_is_valid(info->name)) {
+        fprintf(stderr, "Registering `%s' with illegal type name\n", info->name);
+        abort();
+    }
+
     ti->name = g_strdup(info->name);
     ti->parent = g_strdup(info->parent);
 
-- 
2.41.0

On Tue, 14 Nov 2023 at 13:05, 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::hotplug-handler
>     aspeed.i2c.slave::vmstate-if
>     pc-i440fx-3.0-machine::nmi

I think all these '::' are specifically interface types --
see type_initialize_interface(), which constructs the
interface type name by gluing together the class name and
the interface name with a '::'. The rule we ought to be
requiring for ':' I think is "no : in the type name, unless
it is the one generated by type_initialize_interface()".

I think we could do that by having the type_name_is_valid()
checks done in:
 * type_initialize_interface(), on ti->name and interface_type->name
 * type_register_internal(), on info->name

If we do that, can we take ':' out of the list of characters
we permit in type_name_is_valid() ?

thanks
-- PMM

On 14/11/2023 14.21, Peter Maydell wrote:
> On Tue, 14 Nov 2023 at 13:05, 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::hotplug-handler
>>      aspeed.i2c.slave::vmstate-if
>>      pc-i440fx-3.0-machine::nmi
> 
> I think all these '::' are specifically interface types --
> see type_initialize_interface(), which constructs the
> interface type name by gluing together the class name and
> the interface name with a '::'. The rule we ought to be
> requiring for ':' I think is "no : in the type name, unless
> it is the one generated by type_initialize_interface()".
> 
> I think we could do that by having the type_name_is_valid()
> checks done in:
>   * type_initialize_interface(), on ti->name and interface_type->name
>   * type_register_internal(), on info->name
> 
> If we do that, can we take ':' out of the list of characters
> we permit in type_name_is_valid() ?

Thanks, that's a very good idea! Actually, after looking at the code for a 
while, I think it should even be enough to add a check to 
type_register_internal(), since type_initialize_interface() is rather used 
with base types and interface types that should have been registered already 
via type_register...() before.

There just seem to be two stragglers left:

1) #define TYPE_DUMMY "qemu:dummy" in tests/unit/test-io-task.c ...
    easy to fix, it's just a unit test anyway

2) #define TYPE_RAM_DISCARD_MANAGER "qemu:ram-discard-manager"
    in include/exec/memory.h ... I believe it should be OK to
    simply rename it, since it's about an interface type...
    Or do we use these interface names in migration streams, too?

I'll try to come up with some patches to rename them ... let's see how it 
goes...

  Thomas

On 16/11/23 12:17, Thomas Huth wrote:
> On 14/11/2023 14.21, Peter Maydell wrote:
>> On Tue, 14 Nov 2023 at 13:05, Thomas Huth <thuth@redhat.com> wrote:

> There just seem to be two stragglers left:
> 
> 1) #define TYPE_DUMMY "qemu:dummy" in tests/unit/test-io-task.c ...
>     easy to fix, it's just a unit test anyway
> 
> 2) #define TYPE_RAM_DISCARD_MANAGER "qemu:ram-discard-manager"
>     in include/exec/memory.h ... I believe it should be OK to
>     simply rename it, since it's about an interface type...
>     Or do we use these interface names in migration streams, too?

No, QOM interfaces aren't part of any state, so not migrated.

On Thu, 16 Nov 2023 at 11:17, Thomas Huth <thuth@redhat.com> wrote:
> 2) #define TYPE_RAM_DISCARD_MANAGER "qemu:ram-discard-manager"
>     in include/exec/memory.h ... I believe it should be OK to
>     simply rename it, since it's about an interface type...
>     Or do we use these interface names in migration streams, too?

No, QOM type names are separate from migration stream section
names (unless somebody has explicitly set the migration stream
section name to be the QOM type name using ".name = TYPE_WHATEVER"
in a VMStateDescription struct, which in this case they haven't;
and I'm not sure that even the VMStateDescription names end up
on the wire).

It should be safe to change the type name string here.

thanks
-- PMM