| Message ID | 20231114130415.283228-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qom/object: Limit type names to alphanumerical and some few special characters | expand |
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 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
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.
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);
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(+)