[RFC 0/5] RFC: require error handling for dynamically created objects

[RFC 0/5] RFC: require error handling for dynamically created objects

[Daniel P. Berrangé]

With code like

    Object *obj = object_new(TYPE_BLAH)

the caller can be pretty confident that they will successfully create
an object instance of TYPE_BLAH. They know exactly what type has been
requested, so it passing an abstract type for example, it is a clear
programmer error that they'll get an assertion failure.

Conversely with code like

   void somefunc(const char *typename) {
      Object * obj = object_new(typename)
      ...
   }

all bets are off, because the call of object_new() knows nothing
about what 'typename' resolves to. It could easily be an abstract
type. As a result, many code paths have added a manual check ahead
of time

   if (object_class_is_abstract(typename)) {
      error_setg(errp, ....)
   }

...except for where we forget to do this, such as qdev_new().

Overall 'object_new' is a bad design because it is inherantly
unsafe to call with unvalidated typenames.

This problem is made worse by the proposal to introduce the idea
of 'singleton' classes[1].

Thus, this series suggests a way to improve safety at build
time. The core idea is to allow 'object_new' to continue to be
used *if-and-only-if* given a static, const string, because that
scenario indicates the caller is aware of what type they are
creating at build time.

A new 'object_new_dynamic' method is proposed for cases where
the typename is dynamically chosen at runtime. This method has
an "Error **errp" parameter, which can report when an abstract
type is created, leaving the assert()s only for scenarios which
are unambiguous programmer errors.

With a little macro magic, we guarantee a compile error is
generated if 'object_new' is called with a dynamic type, forcing
all potentially unsafe code over to object_new_dynamic.

This is more tractable than adding 'Error **errp' to 'object_new'
as only a handful of places use a dynamic type name.

NB, this is an RFC as it is not fully complete.

 * I have only converted  enough object_new -> object_new_dynamic
   to make the x86_64-softmu target compile. It probably fails on
   other targets.

 * I have not run any test suites yet, so they may or may not pass

 * I stubbed qdev_new to just pass &error_fatal. qdev_new needs
   the same conceptual fix to introcce qdev_new_dynamic with
   the macro magic to force its use

Obviously if there's agreement that this conceptual idea is valid,
then all these gaps would be fixed.

With this series, my objections to Peter Xu's singleton series[1]
would be largely nullified.

[1] https://lists.nongnu.org/archive/html/qemu-devel/2024-10/msg05524.html

Daniel P. Berrangé (5):
  qom: refactor checking abstract property when creating instances
  qom: allow failure of object_new_with_class
  convert code to object_new_dynamic() where appropriate
  qom: introduce object_new_dynamic()
  qom: enforce use of static, const string with object_new()

 accel/accel-user.c               |  3 +-
 chardev/char.c                   |  5 +++-
 hw/core/bus.c                    |  2 +-
 hw/core/cpu-common.c             |  2 +-
 hw/core/qdev.c                   |  4 +--
 hw/i386/x86-common.c             |  5 +++-
 hw/i386/xen/xen-pvh.c            |  2 +-
 hw/vfio/common.c                 |  6 +++-
 hw/vfio/container.c              |  6 +++-
 include/qom/object.h             | 48 ++++++++++++++++++++++++++++++--
 net/net.c                        | 10 ++++---
 qom/object.c                     | 38 +++++++++++++++++--------
 qom/object_interfaces.c          |  7 ++---
 qom/qom-qmp-cmds.c               | 15 ++++++----
 system/vl.c                      |  6 ++--
 target/i386/cpu-apic.c           |  8 +++++-
 target/i386/cpu-sysemu.c         | 11 ++++++--
 target/i386/cpu.c                |  4 +--
 target/s390x/cpu_models_sysemu.c |  7 +++--
 tests/unit/check-qom-interface.c |  3 +-
 tests/unit/test-smp-parse.c      | 20 ++++++-------
 21 files changed, 151 insertions(+), 61 deletions(-)

-- 
2.46.0

[RFC 1/5] qom: refactor checking abstract property when creating instances

[Daniel P. Berrangé]

Push an Error object into object_initialize_with_type, so that
reporting of attempts to create an abstract type is handled at
the lowest level.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qom/object.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 11424cf471..8eed5f6ed3 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -545,14 +545,19 @@ static void object_class_property_init_all(Object *obj)
     }
 }
 
-static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type)
+static bool object_initialize_with_type(Object *obj, size_t size, TypeImpl *type, Error **errp)
 {
     type_initialize(type);
 
     g_assert(type->instance_size >= sizeof(Object));
-    g_assert(type->abstract == false);
     g_assert(size >= type->instance_size);
 
+    if (type->abstract) {
+        error_setg(errp, "Abstract type '%s' cannot be instantiated",
+                   type->name);
+        return false;
+    }
+
     memset(obj, 0, type->instance_size);
     obj->class = type->class;
     object_ref(obj);
@@ -561,6 +566,8 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type
                                             NULL, object_property_free);
     object_init_with_type(obj, type);
     object_post_init_with_type(obj, type);
+
+    return true;
 }
 
 void object_initialize(void *data, size_t size, const char *typename)
@@ -583,7 +590,7 @@ void object_initialize(void *data, size_t size, const char *typename)
         abort();
     }
 
-    object_initialize_with_type(data, size, type);
+    object_initialize_with_type(data, size, type, &error_abort);
 }
 
 bool object_initialize_child_with_props(Object *parentobj,
@@ -755,7 +762,7 @@ typedef union {
 } qemu_max_align_t;
 #endif
 
-static Object *object_new_with_type(Type type)
+static Object *object_new_with_type(Type type, Error **errp)
 {
     Object *obj;
     size_t size, align;
@@ -779,7 +786,10 @@ static Object *object_new_with_type(Type type)
         obj_free = qemu_vfree;
     }
 
-    object_initialize_with_type(obj, size, type);
+    if (!object_initialize_with_type(obj, size, type, errp)) {
+        g_free(obj);
+        return NULL;
+    }
     obj->free = obj_free;
 
     return obj;
@@ -787,14 +797,14 @@ static Object *object_new_with_type(Type type)
 
 Object *object_new_with_class(ObjectClass *klass)
 {
-    return object_new_with_type(klass->type);
+    return object_new_with_type(klass->type, &error_abort);
 }
 
 Object *object_new(const char *typename)
 {
     TypeImpl *ti = type_get_by_name(typename);
 
-    return object_new_with_type(ti);
+    return object_new_with_type(ti, &error_abort);
 }
 
 
@@ -831,11 +841,9 @@ Object *object_new_with_propv(const char *typename,
         return NULL;
     }
 
-    if (object_class_is_abstract(klass)) {
-        error_setg(errp, "object type '%s' is abstract", typename);
+    if (!(obj = object_new_with_type(klass->type, errp))) {
         return NULL;
     }
-    obj = object_new_with_type(klass->type);
 
     if (!object_set_propv(obj, errp, vargs)) {
         goto error;
-- 
2.46.0

[RFC 2/5] qom: allow failure of object_new_with_class

[Daniel P. Berrangé]

Since object_new_with_class() accepts a non-const parameter for
the class, callers should be prepared for failures from unexpected
input. Add an Error parameter for this and make callers check.
If the caller does not already have an Error parameter, it is
satisfactory to use &error_abort if the class parameter choice is
not driven by untrusted user input.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 accel/accel-user.c               |  3 ++-
 include/qom/object.h             |  9 +++++++--
 net/net.c                        | 10 ++++++----
 qom/object.c                     |  4 ++--
 system/vl.c                      |  6 ++++--
 target/i386/cpu-apic.c           |  8 +++++++-
 target/i386/cpu-sysemu.c         | 11 ++++++++---
 target/i386/cpu.c                |  4 ++--
 target/s390x/cpu_models_sysemu.c |  7 +++++--
 9 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/accel/accel-user.c b/accel/accel-user.c
index 22b6a1a1a8..04ba4ab920 100644
--- a/accel/accel-user.c
+++ b/accel/accel-user.c
@@ -18,7 +18,8 @@ AccelState *current_accel(void)
         AccelClass *ac = accel_find("tcg");
 
         g_assert(ac != NULL);
-        accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
+        accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac),
+                                            &error_abort));
     }
     return accel;
 }
diff --git a/include/qom/object.h b/include/qom/object.h
index 2af9854675..222c60e205 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -606,14 +606,19 @@ struct InterfaceClass
 /**
  * object_new_with_class:
  * @klass: The class to instantiate.
+ * @errp: pointer to be filled with error details on failure
  *
  * This function will initialize a new object using heap allocated memory.
  * The returned object has a reference count of 1, and will be freed when
  * the last reference is dropped.
  *
- * Returns: The newly allocated and instantiated object.
+ * If an instance of @klass is not permitted to be instantiated, an
+ * error will be raised. This can happen if @klass is abstract.
+ *
+ * Returns: The newly allocated and instantiated object, or NULL
+ * on error.
  */
-Object *object_new_with_class(ObjectClass *klass);
+Object *object_new_with_class(ObjectClass *klass, Error **errp);
 
 /**
  * object_new:
diff --git a/net/net.c b/net/net.c
index d9b23a8f8c..7fb5e966f3 100644
--- a/net/net.c
+++ b/net/net.c
@@ -944,11 +944,13 @@ GPtrArray *qemu_get_nic_models(const char *device_type)
              * create this property during instance_init, so we have to create
              * a temporary instance here to be able to check it.
              */
-            Object *obj = object_new_with_class(OBJECT_CLASS(dc));
-            if (object_property_find(obj, "netdev")) {
-                g_ptr_array_add(nic_models, (gpointer)name);
+            Object *obj = object_new_with_class(OBJECT_CLASS(dc), NULL);
+            if (obj) {
+                if (object_property_find(obj, "netdev")) {
+                    g_ptr_array_add(nic_models, (gpointer)name);
+                }
+                object_unref(obj);
             }
-            object_unref(obj);
         }
         next = list->next;
         g_slist_free_1(list);
diff --git a/qom/object.c b/qom/object.c
index 8eed5f6ed3..1f139aa9c8 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -795,9 +795,9 @@ static Object *object_new_with_type(Type type, Error **errp)
     return obj;
 }
 
-Object *object_new_with_class(ObjectClass *klass)
+Object *object_new_with_class(ObjectClass *klass, Error **errp)
 {
-    return object_new_with_type(klass->type, &error_abort);
+    return object_new_with_type(klass->type, errp);
 }
 
 Object *object_new(const char *typename)
diff --git a/system/vl.c b/system/vl.c
index d217b3d64d..f4eec7f35c 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2117,7 +2117,8 @@ static void qemu_create_machine(QDict *qdict)
     MachineClass *machine_class = select_machine(qdict, &error_fatal);
     object_set_machine_compat_props(machine_class->compat_props);
 
-    current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class)));
+    current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class),
+                                                    &error_fatal));
     object_property_add_child(object_get_root(), "machine",
                               OBJECT(current_machine));
     object_property_add_child(container_get(OBJECT(current_machine),
@@ -2327,7 +2328,8 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
         }
         goto bad;
     }
-    accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
+    accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac),
+                                        &error_fatal));
     object_apply_compat_props(OBJECT(accel));
     qemu_opt_foreach(opts, accelerator_set_property,
                      accel,
diff --git a/target/i386/cpu-apic.c b/target/i386/cpu-apic.c
index d397ec94dc..8a518c50c7 100644
--- a/target/i386/cpu-apic.c
+++ b/target/i386/cpu-apic.c
@@ -43,12 +43,18 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
     APICCommonState *apic;
     APICCommonClass *apic_class = apic_get_class(errp);
+    Object *apicobj;
 
     if (!apic_class) {
         return;
     }
 
-    cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
+    apicobj = object_new_with_class(OBJECT_CLASS(apic_class),
+                                    errp);
+    if (!apicobj) {
+        return;
+    }
+    cpu->apic_state = DEVICE(apicobj);
     object_property_add_child(OBJECT(cpu), "lapic",
                               OBJECT(cpu->apic_state));
     object_unref(OBJECT(cpu->apic_state));
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 227ac021f6..612ff09e57 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -156,15 +156,20 @@ static X86CPU *x86_cpu_from_model(const char *model, QObject *props,
 {
     X86CPU *xc = NULL;
     X86CPUClass *xcc;
+    Object *xcobj;
     Error *err = NULL;
 
     xcc = X86_CPU_CLASS(cpu_class_by_name(TYPE_X86_CPU, model));
     if (xcc == NULL) {
-        error_setg(&err, "CPU model '%s' not found", model);
-        goto out;
+        error_setg(errp, "CPU model '%s' not found", model);
+        return NULL;
     }
 
-    xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
+    xcobj = object_new_with_class(OBJECT_CLASS(xcc), errp);
+    if (!xcobj) {
+        return NULL;
+    }
+    xc = X86_CPU(xcobj);
     if (props) {
         object_apply_props(OBJECT(xc), props, props_arg_name, &err);
         if (err) {
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1ff1af032e..8760f408fa 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5894,7 +5894,7 @@ static GSList *get_sorted_cpu_model_list(void)
 
 static char *x86_cpu_class_get_model_id(X86CPUClass *xc)
 {
-    Object *obj = object_new_with_class(OBJECT_CLASS(xc));
+    Object *obj = object_new_with_class(OBJECT_CLASS(xc), &error_abort);
     char *r = object_property_get_str(obj, "model-id", &error_abort);
     object_unref(obj);
     return r;
@@ -5992,7 +5992,7 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
         return;
     }
 
-    xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
+    xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc), &error_abort));
 
     x86_cpu_expand_features(xc, &err);
     if (err) {
diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
index f6df691b66..7fe3093056 100644
--- a/target/s390x/cpu_models_sysemu.c
+++ b/target/s390x/cpu_models_sysemu.c
@@ -69,7 +69,7 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
     if (cpu_list_data->model) {
         Object *obj;
         S390CPU *sc;
-        obj = object_new_with_class(klass);
+        obj = object_new_with_class(klass, &error_abort);
         sc = S390_CPU(obj);
         if (sc->model) {
             info->has_unavailable_features = true;
@@ -116,7 +116,10 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
         error_setg(errp, "The CPU definition '%s' requires KVM", info->name);
         return;
     }
-    obj = object_new_with_class(oc);
+    obj = object_new_with_class(oc, errp);
+    if (!obj) {
+        return;
+    }
     cpu = S390_CPU(obj);
 
     if (!cpu->model) {
-- 
2.46.0

[RFC 3/5] convert code to object_new_dynamic() where appropriate

[Daniel P. Berrangé]

In cases where object_new() is not being passed a static, const
string, the caller cannot be sure what type they are instantiating.
There is a risk that instantiation could fail, if it is an abstract
type.

Convert such cases over to use object_new_dynamic() such that they
are forced to expect failure. In some cases failure can be easily
propagated, but in others using &error_abort maintainers existing
behaviour.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char.c                   |  5 ++++-
 hw/core/bus.c                    |  2 +-
 hw/core/cpu-common.c             |  2 +-
 hw/core/qdev.c                   |  4 ++--
 hw/i386/x86-common.c             |  5 ++++-
 hw/i386/xen/xen-pvh.c            |  2 +-
 hw/vfio/common.c                 |  6 +++++-
 hw/vfio/container.c              |  6 +++++-
 qom/object_interfaces.c          |  7 ++-----
 qom/qom-qmp-cmds.c               | 15 +++++++++------
 tests/unit/check-qom-interface.c |  3 ++-
 tests/unit/test-smp-parse.c      | 20 ++++++++++----------
 12 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index a1722aa076..4de9a496a2 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -996,7 +996,10 @@ static Chardev *chardev_new(const char *id, const char *typename,
     assert(g_str_has_prefix(typename, "chardev-"));
     assert(id);
 
-    obj = object_new(typename);
+    if (!(obj = object_new_dynamic(typename, errp))) {
+        return NULL;
+    }
+    
     chr = CHARDEV(obj);
     chr->handover_yank_instance = handover_yank_instance;
     chr->label = g_strdup(id);
diff --git a/hw/core/bus.c b/hw/core/bus.c
index b9d89495cd..2de714b274 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -163,7 +163,7 @@ BusState *qbus_new(const char *typename, DeviceState *parent, const char *name)
 {
     BusState *bus;
 
-    bus = BUS(object_new(typename));
+    bus = BUS(object_new_dynamic(typename, &error_abort));
     qbus_init_internal(bus, parent, name);
 
     return bus;
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 09c7903594..8768ae39ed 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -57,7 +57,7 @@ bool cpu_exists(int64_t id)
 CPUState *cpu_create(const char *typename)
 {
     Error *err = NULL;
-    CPUState *cpu = CPU(object_new(typename));
+    CPUState *cpu = CPU(object_new_dynamic(typename, &error_abort));
     if (!qdev_realize(DEVICE(cpu), NULL, &err)) {
         error_report_err(err);
         object_unref(OBJECT(cpu));
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index db36f54d91..a143d8b721 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -162,7 +162,7 @@ DeviceState *qdev_new(const char *name)
         error_report("unknown type '%s'", name);
         abort();
     }
-    return DEVICE(object_new(name));
+    return DEVICE(object_new_dynamic(name, &error_abort));
 }
 
 DeviceState *qdev_try_new(const char *name)
@@ -170,7 +170,7 @@ DeviceState *qdev_try_new(const char *name)
     if (!module_object_class_by_name(name)) {
         return NULL;
     }
-    return DEVICE(object_new(name));
+    return DEVICE(object_new_dynamic(name, &error_abort));
 }
 
 static QTAILQ_HEAD(, DeviceListener) device_listeners
diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index b86c38212e..b75443ec26 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -55,7 +55,10 @@ static size_t pvh_start_addr;
 
 static void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp)
 {
-    Object *cpu = object_new(MACHINE(x86ms)->cpu_type);
+    Object *cpu = object_new_dynamic(MACHINE(x86ms)->cpu_type, errp);
+    if (!cpu) {
+        return;
+    }
 
     if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
         goto out;
diff --git a/hw/i386/xen/xen-pvh.c b/hw/i386/xen/xen-pvh.c
index f1f02d3311..9aeb4e430b 100644
--- a/hw/i386/xen/xen-pvh.c
+++ b/hw/i386/xen/xen-pvh.c
@@ -28,7 +28,7 @@ struct XenPVHx86State {
 static DeviceState *xen_pvh_cpu_new(MachineState *ms,
                                     int64_t apic_id)
 {
-    Object *cpu = object_new(ms->cpu_type);
+    Object *cpu = object_new_dynamic(ms->cpu_type, &error_abort);
 
     object_property_add_child(OBJECT(ms), "cpu[*]", cpu);
     object_property_set_uint(cpu, "apic-id", apic_id, &error_fatal);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 36d0cf6585..d57097a194 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1550,7 +1550,11 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev,
 
 
     if (!vbasedev->mdev) {
-        hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
+        Object *obj = object_new_dynamic(ops->hiod_typename, errp);
+        if (!obj) {
+            return false;
+        }
+        hiod = HOST_IOMMU_DEVICE(obj);
         vbasedev->hiod = hiod;
     }
 
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 9ccdb639ac..5964009a3c 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -419,6 +419,7 @@ static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
 {
     int iommu_type;
     const char *vioc_name;
+    Object *obj;
     VFIOContainer *container;
 
     iommu_type = vfio_get_iommu_type(fd, errp);
@@ -432,7 +433,10 @@ static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
 
     vioc_name = vfio_get_iommu_class_name(iommu_type);
 
-    container = VFIO_IOMMU_LEGACY(object_new(vioc_name));
+    if (!(obj = object_new_dynamic(vioc_name, NULL))) {
+        return NULL;
+    }
+    container = VFIO_IOMMU_LEGACY(obj);
     container->fd = fd;
     container->iommu_type = iommu_type;
     return container;
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index e0833c8bfe..756fca3649 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -102,13 +102,10 @@ Object *user_creatable_add_type(const char *type, const char *id,
         return NULL;
     }
 
-    if (object_class_is_abstract(klass)) {
-        error_setg(errp, "object type '%s' is abstract", type);
+    assert(qdict);
+    if (!(obj = object_new_dynamic(type, errp))) {
         return NULL;
     }
-
-    assert(qdict);
-    obj = object_new(type);
     object_set_properties_from_qdict(obj, qdict, v, &local_err);
     if (local_err) {
         goto out;
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index e91a235347..b1590231d7 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -134,14 +134,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
         return NULL;
     }
 
-    if (!object_class_dynamic_cast(klass, TYPE_DEVICE)
-        || object_class_is_abstract(klass)) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename",
-                   "a non-abstract device type");
+    if (!object_class_dynamic_cast(klass, TYPE_DEVICE)) {
+        error_setg(errp, "Object type '%s' is not a device",
+                   typename);
         return NULL;
     }
 
-    obj = object_new(typename);
+    if (!(obj = object_new_dynamic(typename, errp))) {
+        return NULL;
+    }
 
     object_property_iter_init(&iter, obj);
     while ((prop = object_property_iter_next(&iter))) {
@@ -202,7 +203,9 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
     if (object_class_is_abstract(klass)) {
         object_class_property_iter_init(&iter, klass);
     } else {
-        obj = object_new(typename);
+        if (!(obj = object_new_dynamic(typename, errp))) {
+            return NULL;
+        }
         object_property_iter_init(&iter, obj);
     }
     while ((prop = object_property_iter_next(&iter))) {
diff --git a/tests/unit/check-qom-interface.c b/tests/unit/check-qom-interface.c
index c99be97ed8..e4532ae814 100644
--- a/tests/unit/check-qom-interface.c
+++ b/tests/unit/check-qom-interface.c
@@ -13,6 +13,7 @@
 
 #include "qom/object.h"
 #include "qemu/module.h"
+#include "qapi/error.h"
 
 
 #define TYPE_TEST_IF "test-interface"
@@ -67,7 +68,7 @@ static const TypeInfo intermediate_impl_info = {
 
 static void test_interface_impl(const char *type)
 {
-    Object *obj = object_new(type);
+    Object *obj = object_new_dynamic(type, &error_abort);
     TestIf *iobj = TEST_IF(obj);
     TestIfClass *ioc = TEST_IF_GET_CLASS(iobj);
 
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index f9bccb56ab..f4893d5f24 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -1008,7 +1008,7 @@ static void machine_full_topo_class_init(ObjectClass *oc, void *data)
 static void test_generic_valid(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1027,7 +1027,7 @@ static void test_generic_valid(const void *opaque)
 static void test_generic_invalid(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1046,7 +1046,7 @@ static void test_generic_invalid(const void *opaque)
 static void test_with_modules(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1096,7 +1096,7 @@ static void test_with_modules(const void *opaque)
 static void test_with_dies(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1146,7 +1146,7 @@ static void test_with_dies(const void *opaque)
 static void test_with_modules_dies(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1207,7 +1207,7 @@ static void test_with_modules_dies(const void *opaque)
 static void test_with_clusters(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1257,7 +1257,7 @@ static void test_with_clusters(const void *opaque)
 static void test_with_books(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1307,7 +1307,7 @@ static void test_with_books(const void *opaque)
 static void test_with_drawers(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1357,7 +1357,7 @@ static void test_with_drawers(const void *opaque)
 static void test_with_drawers_books(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
@@ -1418,7 +1418,7 @@ static void test_with_drawers_books(const void *opaque)
 static void test_full_topo(const void *opaque)
 {
     const char *machine_type = opaque;
-    Object *obj = object_new(machine_type);
+    Object *obj = object_new_dynamic(machine_type, &error_abort);
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
     SMPTestData data = {};
-- 
2.46.0

[RFC 4/5] qom: introduce object_new_dynamic()

[Daniel P. Berrangé]

object_new() has a failure scenario where it will assert() if given
an abstract type. Callers which are creating objects based on user
input, or unknown/untrusted type names, must manually check the
result of object_class_is_abstract() before calling object_new()
to propagate an Error, instead of asserting.

Introduce a object_new_dynamic() method which is a counterpart to
object_new() that directly returns an Error, instead of asserting.
This new method is to be used where the typename is specified
dynamically by code separate from the immediate caller.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qom/object.h | 27 +++++++++++++++++++++++++++
 qom/object.c         |  6 ++++++
 2 files changed, 33 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index 222c60e205..8c2f3551c5 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -624,14 +624,41 @@ Object *object_new_with_class(ObjectClass *klass, Error **errp);
  * object_new:
  * @typename: The name of the type of the object to instantiate.
  *
+ * This method should be used where @typename is statically specified
+ * from a const string at build time, where the caller does not expect
+ * failure to be possible.
+ *
  * This function will initialize a new object using heap allocated memory.
  * The returned object has a reference count of 1, and will be freed when
  * the last reference is dropped.
  *
+ * If an instance of @typename is not permitted to be instantiated, an
+ * assert will be raised. This can happen if @typename is abstract.
+ *
  * Returns: The newly allocated and instantiated object.
  */
 Object *object_new(const char *typename);
 
+/**
+ * object_new_dynamic:
+ * @typename: The name of the type of the object to instantiate.
+ * @errp: pointer to be filled with error details on failure
+ *
+ * This method should be used where @typename is dynamically chosen
+ * at runtime, which has the possibility of unexpected choices leading
+ * to failures.
+ *
+ * This function will initialize a new object using heap allocated memory.
+ * The returned object has a reference count of 1, and will be freed when
+ * the last reference is dropped.
+ *
+ * If an instance of @typename is not permitted to be instantiated, an
+ * error will be raised. This can happen if @typename is abstract.
+ *
+ * Returns: The newly allocated and instantiated object.
+ */
+Object *object_new_dynamic(const char *typename, Error **errp);
+
 /**
  * object_new_with_props:
  * @typename:  The name of the type of the object to instantiate.
diff --git a/qom/object.c b/qom/object.c
index 1f139aa9c8..1ed62dc2c9 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -807,6 +807,12 @@ Object *object_new(const char *typename)
     return object_new_with_type(ti, &error_abort);
 }
 
+Object *object_new_dynamic(const char *typename, Error **errp)
+{
+    TypeImpl *ti = type_get_by_name(typename);
+
+    return object_new_with_type(ti, errp);
+}
 
 Object *object_new_with_props(const char *typename,
                               Object *parent,
-- 
2.46.0

[RFC 5/5] qom: enforce use of static, const string with object_new()

[Daniel P. Berrangé]

Since object_new() will assert(), it should only be used in scenarios
where the caller knows exactly what type it is asking to be created,
and can thus be confident in avoiding abstract types.

Enforce this by using a macro wrapper which types to paste "" to the
type name. This will generate a compile error if not passed a static
const string, forcing callers to use object_new_dynamic() instead.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qom/object.h | 12 +++++++++++-
 qom/object.c         |  2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 8c2f3551c5..6a21cb6ca0 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -637,7 +637,17 @@ Object *object_new_with_class(ObjectClass *klass, Error **errp);
  *
  * Returns: The newly allocated and instantiated object.
  */
-Object *object_new(const char *typename);
+
+/*
+ * NB, object_new_helper is just an internal helper, wrapped by
+ * the object_new() macro which prevents invokation unless given
+ * a static, const string.
+ *
+ * Code should call object_new(), or object_new_dynamic(), not
+ * object_new_helper().
+ */
+Object *object_new_helper(const char *typename);
+#define object_new(typename) object_new_static(typename "")
 
 /**
  * object_new_dynamic:
diff --git a/qom/object.c b/qom/object.c
index 1ed62dc2c9..36c1c82815 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -800,7 +800,7 @@ Object *object_new_with_class(ObjectClass *klass, Error **errp)
     return object_new_with_type(klass->type, errp);
 }
 
-Object *object_new(const char *typename)
+Object *object_new_helper(const char *typename)
 {
     TypeImpl *ti = type_get_by_name(typename);
 
-- 
2.46.0

Re: [RFC 2/5] qom: allow failure of object_new_with_class

[Peter Xu]

On Thu, Oct 31, 2024 at 03:53:47PM +0000, Daniel P. Berrangé wrote:
> Since object_new_with_class() accepts a non-const parameter for
> the class, callers should be prepared for failures from unexpected
> input. Add an Error parameter for this and make callers check.
> If the caller does not already have an Error parameter, it is
> satisfactory to use &error_abort if the class parameter choice is
> not driven by untrusted user input.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  accel/accel-user.c               |  3 ++-
>  include/qom/object.h             |  9 +++++++--
>  net/net.c                        | 10 ++++++----
>  qom/object.c                     |  4 ++--
>  system/vl.c                      |  6 ++++--
>  target/i386/cpu-apic.c           |  8 +++++++-
>  target/i386/cpu-sysemu.c         | 11 ++++++++---
>  target/i386/cpu.c                |  4 ++--
>  target/s390x/cpu_models_sysemu.c |  7 +++++--
>  9 files changed, 43 insertions(+), 19 deletions(-)
> 
> diff --git a/accel/accel-user.c b/accel/accel-user.c
> index 22b6a1a1a8..04ba4ab920 100644
> --- a/accel/accel-user.c
> +++ b/accel/accel-user.c
> @@ -18,7 +18,8 @@ AccelState *current_accel(void)
>          AccelClass *ac = accel_find("tcg");
>  
>          g_assert(ac != NULL);
> -        accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
> +        accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac),
> +                                            &error_abort));
>      }
>      return accel;
>  }
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 2af9854675..222c60e205 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -606,14 +606,19 @@ struct InterfaceClass
>  /**
>   * object_new_with_class:
>   * @klass: The class to instantiate.
> + * @errp: pointer to be filled with error details on failure
>   *
>   * This function will initialize a new object using heap allocated memory.
>   * The returned object has a reference count of 1, and will be freed when
>   * the last reference is dropped.
>   *
> - * Returns: The newly allocated and instantiated object.
> + * If an instance of @klass is not permitted to be instantiated, an
> + * error will be raised. This can happen if @klass is abstract.
> + *
> + * Returns: The newly allocated and instantiated object, or NULL
> + * on error.
>   */
> -Object *object_new_with_class(ObjectClass *klass);
> +Object *object_new_with_class(ObjectClass *klass, Error **errp);
>  
>  /**
>   * object_new:
> diff --git a/net/net.c b/net/net.c
> index d9b23a8f8c..7fb5e966f3 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -944,11 +944,13 @@ GPtrArray *qemu_get_nic_models(const char *device_type)
>               * create this property during instance_init, so we have to create
>               * a temporary instance here to be able to check it.
>               */
> -            Object *obj = object_new_with_class(OBJECT_CLASS(dc));
> -            if (object_property_find(obj, "netdev")) {
> -                g_ptr_array_add(nic_models, (gpointer)name);
> +            Object *obj = object_new_with_class(OBJECT_CLASS(dc), NULL);

One trivial comment: I kind of understand why NULL was chosen, but I don't
think it's easily understandable on why it's better.

When object_new() can have side effect and might fail, logically it could
be better that it asserts failure here when new NICs added (that can start
to fail it here.. while we shouldn't have such now), instead of silently
not showing up in the module list.  So at least we notify the net developer
something might be off (while IIUC this function is trying to list all NIC
modules QEMU supports).

It's a pity virtio-net must have the "netdev" property until
instance_init(), or it should really use object_class_property_iter_init()
with zero side effect..

> +            if (obj) {
> +                if (object_property_find(obj, "netdev")) {
> +                    g_ptr_array_add(nic_models, (gpointer)name);
> +                }
> +                object_unref(obj);
>              }
> -            object_unref(obj);
>          }
>          next = list->next;
>          g_slist_free_1(list);
> diff --git a/qom/object.c b/qom/object.c
> index 8eed5f6ed3..1f139aa9c8 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -795,9 +795,9 @@ static Object *object_new_with_type(Type type, Error **errp)
>      return obj;
>  }
>  
> -Object *object_new_with_class(ObjectClass *klass)
> +Object *object_new_with_class(ObjectClass *klass, Error **errp)
>  {
> -    return object_new_with_type(klass->type, &error_abort);
> +    return object_new_with_type(klass->type, errp);
>  }
>  
>  Object *object_new(const char *typename)
> diff --git a/system/vl.c b/system/vl.c
> index d217b3d64d..f4eec7f35c 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -2117,7 +2117,8 @@ static void qemu_create_machine(QDict *qdict)
>      MachineClass *machine_class = select_machine(qdict, &error_fatal);
>      object_set_machine_compat_props(machine_class->compat_props);
>  
> -    current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class)));
> +    current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class),
> +                                                    &error_fatal));
>      object_property_add_child(object_get_root(), "machine",
>                                OBJECT(current_machine));
>      object_property_add_child(container_get(OBJECT(current_machine),
> @@ -2327,7 +2328,8 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>          }
>          goto bad;
>      }
> -    accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
> +    accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac),
> +                                        &error_fatal));
>      object_apply_compat_props(OBJECT(accel));
>      qemu_opt_foreach(opts, accelerator_set_property,
>                       accel,
> diff --git a/target/i386/cpu-apic.c b/target/i386/cpu-apic.c
> index d397ec94dc..8a518c50c7 100644
> --- a/target/i386/cpu-apic.c
> +++ b/target/i386/cpu-apic.c
> @@ -43,12 +43,18 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>  {
>      APICCommonState *apic;
>      APICCommonClass *apic_class = apic_get_class(errp);
> +    Object *apicobj;
>  
>      if (!apic_class) {
>          return;
>      }
>  
> -    cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
> +    apicobj = object_new_with_class(OBJECT_CLASS(apic_class),
> +                                    errp);
> +    if (!apicobj) {
> +        return;
> +    }
> +    cpu->apic_state = DEVICE(apicobj);
>      object_property_add_child(OBJECT(cpu), "lapic",
>                                OBJECT(cpu->apic_state));
>      object_unref(OBJECT(cpu->apic_state));
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 227ac021f6..612ff09e57 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -156,15 +156,20 @@ static X86CPU *x86_cpu_from_model(const char *model, QObject *props,
>  {
>      X86CPU *xc = NULL;
>      X86CPUClass *xcc;
> +    Object *xcobj;
>      Error *err = NULL;
>  
>      xcc = X86_CPU_CLASS(cpu_class_by_name(TYPE_X86_CPU, model));
>      if (xcc == NULL) {
> -        error_setg(&err, "CPU model '%s' not found", model);
> -        goto out;
> +        error_setg(errp, "CPU model '%s' not found", model);
> +        return NULL;
>      }
>  
> -    xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
> +    xcobj = object_new_with_class(OBJECT_CLASS(xcc), errp);
> +    if (!xcobj) {
> +        return NULL;
> +    }
> +    xc = X86_CPU(xcobj);
>      if (props) {
>          object_apply_props(OBJECT(xc), props, props_arg_name, &err);
>          if (err) {
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1ff1af032e..8760f408fa 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5894,7 +5894,7 @@ static GSList *get_sorted_cpu_model_list(void)
>  
>  static char *x86_cpu_class_get_model_id(X86CPUClass *xc)
>  {
> -    Object *obj = object_new_with_class(OBJECT_CLASS(xc));
> +    Object *obj = object_new_with_class(OBJECT_CLASS(xc), &error_abort);
>      char *r = object_property_get_str(obj, "model-id", &error_abort);
>      object_unref(obj);
>      return r;
> @@ -5992,7 +5992,7 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
>          return;
>      }
>  
> -    xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
> +    xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc), &error_abort));
>  
>      x86_cpu_expand_features(xc, &err);
>      if (err) {
> diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
> index f6df691b66..7fe3093056 100644
> --- a/target/s390x/cpu_models_sysemu.c
> +++ b/target/s390x/cpu_models_sysemu.c
> @@ -69,7 +69,7 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>      if (cpu_list_data->model) {
>          Object *obj;
>          S390CPU *sc;
> -        obj = object_new_with_class(klass);
> +        obj = object_new_with_class(klass, &error_abort);
>          sc = S390_CPU(obj);
>          if (sc->model) {
>              info->has_unavailable_features = true;
> @@ -116,7 +116,10 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
>          error_setg(errp, "The CPU definition '%s' requires KVM", info->name);
>          return;
>      }
> -    obj = object_new_with_class(oc);
> +    obj = object_new_with_class(oc, errp);
> +    if (!obj) {
> +        return;
> +    }
>      cpu = S390_CPU(obj);
>  
>      if (!cpu->model) {
> -- 
> 2.46.0
> 

-- 
Peter Xu

Re: [RFC 3/5] convert code to object_new_dynamic() where appropriate

[Peter Xu]

On Thu, Oct 31, 2024 at 03:53:48PM +0000, Daniel P. Berrangé wrote:
> In cases where object_new() is not being passed a static, const
> string, the caller cannot be sure what type they are instantiating.
> There is a risk that instantiation could fail, if it is an abstract
> type.
> 
> Convert such cases over to use object_new_dynamic() such that they
> are forced to expect failure. In some cases failure can be easily
> propagated, but in others using &error_abort maintainers existing
> behaviour.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char.c                   |  5 ++++-
>  hw/core/bus.c                    |  2 +-
>  hw/core/cpu-common.c             |  2 +-
>  hw/core/qdev.c                   |  4 ++--
>  hw/i386/x86-common.c             |  5 ++++-
>  hw/i386/xen/xen-pvh.c            |  2 +-
>  hw/vfio/common.c                 |  6 +++++-
>  hw/vfio/container.c              |  6 +++++-
>  qom/object_interfaces.c          |  7 ++-----
>  qom/qom-qmp-cmds.c               | 15 +++++++++------
>  tests/unit/check-qom-interface.c |  3 ++-
>  tests/unit/test-smp-parse.c      | 20 ++++++++++----------
>  12 files changed, 46 insertions(+), 31 deletions(-)

I think we could leave the test cases alone without _dynamic(), because
they do test static types (even if they used "opaque"..), and they should
really (and forever) assert on failures..

IMHO we should keep _dynamic() usages small if we'd like to introduce it,
only in the paths where its failure will be properly handled.  Basically, I
think we shouldn't use _dynamic() if we know it'll be error_abort.. because
that's fundamentally identical to object_new().

A small set of _dynamic() usages also keep us easy to track all the paths
where QEMU can create some totally random objects too.

-- 
Peter Xu

Re: [RFC 4/5] qom: introduce object_new_dynamic()

[Peter Xu]

On Thu, Oct 31, 2024 at 03:53:49PM +0000, Daniel P. Berrangé wrote:
> object_new() has a failure scenario where it will assert() if given
> an abstract type. Callers which are creating objects based on user
> input, or unknown/untrusted type names, must manually check the
> result of object_class_is_abstract() before calling object_new()
> to propagate an Error, instead of asserting.
> 
> Introduce a object_new_dynamic() method which is a counterpart to
> object_new() that directly returns an Error, instead of asserting.
> This new method is to be used where the typename is specified
> dynamically by code separate from the immediate caller.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Needs some patch order changes.. v.s. the previous one.

Thanks,

-- 
Peter Xu

Re: [RFC 5/5] qom: enforce use of static, const string with object_new()

[Peter Xu]

On Thu, Oct 31, 2024 at 03:53:50PM +0000, Daniel P. Berrangé wrote:
> Since object_new() will assert(), it should only be used in scenarios
> where the caller knows exactly what type it is asking to be created,
> and can thus be confident in avoiding abstract types.
> 
> Enforce this by using a macro wrapper which types to paste "" to the
> type name. This will generate a compile error if not passed a static
> const string, forcing callers to use object_new_dynamic() instead.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/qom/object.h | 12 +++++++++++-
>  qom/object.c         |  2 +-
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 8c2f3551c5..6a21cb6ca0 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -637,7 +637,17 @@ Object *object_new_with_class(ObjectClass *klass, Error **errp);
>   *
>   * Returns: The newly allocated and instantiated object.
>   */
> -Object *object_new(const char *typename);
> +
> +/*
> + * NB, object_new_helper is just an internal helper, wrapped by
> + * the object_new() macro which prevents invokation unless given
> + * a static, const string.
> + *
> + * Code should call object_new(), or object_new_dynamic(), not
> + * object_new_helper().
> + */
> +Object *object_new_helper(const char *typename);

Nit; personally I'd call it object_new_internal().  No strong opinions.

> +#define object_new(typename) object_new_static(typename "")

Interesting trick on const check.. I see why the test cases need change
now.  Feel free to ignore the comment there then..

Could be an improvement to enforce error checks on new dynamic allocations.
This should be better than my patch 1 indeed.

Thanks,

-- 
Peter Xu

Re: [RFC 0/5] RFC: require error handling for dynamically created objects

[Peter Xu]

On Thu, Oct 31, 2024 at 03:53:45PM +0000, Daniel P. Berrangé wrote:
> With code like
> 
>     Object *obj = object_new(TYPE_BLAH)
> 
> the caller can be pretty confident that they will successfully create
> an object instance of TYPE_BLAH. They know exactly what type has been
> requested, so it passing an abstract type for example, it is a clear
> programmer error that they'll get an assertion failure.
> 
> Conversely with code like
> 
>    void somefunc(const char *typename) {
>       Object * obj = object_new(typename)
>       ...
>    }
> 
> all bets are off, because the call of object_new() knows nothing
> about what 'typename' resolves to. It could easily be an abstract
> type. As a result, many code paths have added a manual check ahead
> of time
> 
>    if (object_class_is_abstract(typename)) {
>       error_setg(errp, ....)
>    }
> 
> ...except for where we forget to do this, such as qdev_new().
> 
> Overall 'object_new' is a bad design because it is inherantly
> unsafe to call with unvalidated typenames.
> 
> This problem is made worse by the proposal to introduce the idea
> of 'singleton' classes[1].
> 
> Thus, this series suggests a way to improve safety at build
> time. The core idea is to allow 'object_new' to continue to be
> used *if-and-only-if* given a static, const string, because that
> scenario indicates the caller is aware of what type they are
> creating at build time.
> 
> A new 'object_new_dynamic' method is proposed for cases where
> the typename is dynamically chosen at runtime. This method has
> an "Error **errp" parameter, which can report when an abstract
> type is created, leaving the assert()s only for scenarios which
> are unambiguous programmer errors.
> 
> With a little macro magic, we guarantee a compile error is
> generated if 'object_new' is called with a dynamic type, forcing
> all potentially unsafe code over to object_new_dynamic.
> 
> This is more tractable than adding 'Error **errp' to 'object_new'
> as only a handful of places use a dynamic type name.
> 
> NB, this is an RFC as it is not fully complete.
> 
>  * I have only converted  enough object_new -> object_new_dynamic
>    to make the x86_64-softmu target compile. It probably fails on
>    other targets.
> 
>  * I have not run any test suites yet, so they may or may not pass
> 
>  * I stubbed qdev_new to just pass &error_fatal. qdev_new needs
>    the same conceptual fix to introcce qdev_new_dynamic with
>    the macro magic to force its use

I suppose this is the only missing path in my patch 1 in qdev_new()..

Even if we don't convert all of them, at least we can still convert the
easiest (qdev_device_add_from_qdict()) so as to drop the abstract check in
qdev_get_device_class().

The other one we can drop already after this series applied is the one in
char_get_class().  I think after that, all explicit abstract checks for
for object instantiations should be all gone.

-- 
Peter Xu

Re: [RFC 2/5] qom: allow failure of object_new_with_class

[Daniel P. Berrangé]

On Thu, Oct 31, 2024 at 03:09:40PM -0400, Peter Xu wrote:
> On Thu, Oct 31, 2024 at 03:53:47PM +0000, Daniel P. Berrangé wrote:
> > Since object_new_with_class() accepts a non-const parameter for
> > the class, callers should be prepared for failures from unexpected
> > input. Add an Error parameter for this and make callers check.
> > If the caller does not already have an Error parameter, it is
> > satisfactory to use &error_abort if the class parameter choice is
> > not driven by untrusted user input.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  accel/accel-user.c               |  3 ++-
> >  include/qom/object.h             |  9 +++++++--
> >  net/net.c                        | 10 ++++++----
> >  qom/object.c                     |  4 ++--
> >  system/vl.c                      |  6 ++++--
> >  target/i386/cpu-apic.c           |  8 +++++++-
> >  target/i386/cpu-sysemu.c         | 11 ++++++++---
> >  target/i386/cpu.c                |  4 ++--
> >  target/s390x/cpu_models_sysemu.c |  7 +++++--
> >  9 files changed, 43 insertions(+), 19 deletions(-)
> > 
> > diff --git a/accel/accel-user.c b/accel/accel-user.c
> > index 22b6a1a1a8..04ba4ab920 100644
> > --- a/accel/accel-user.c
> > +++ b/accel/accel-user.c
> > @@ -18,7 +18,8 @@ AccelState *current_accel(void)
> >          AccelClass *ac = accel_find("tcg");
> >  
> >          g_assert(ac != NULL);
> > -        accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
> > +        accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac),
> > +                                            &error_abort));
> >      }
> >      return accel;
> >  }
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index 2af9854675..222c60e205 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -606,14 +606,19 @@ struct InterfaceClass
> >  /**
> >   * object_new_with_class:
> >   * @klass: The class to instantiate.
> > + * @errp: pointer to be filled with error details on failure
> >   *
> >   * This function will initialize a new object using heap allocated memory.
> >   * The returned object has a reference count of 1, and will be freed when
> >   * the last reference is dropped.
> >   *
> > - * Returns: The newly allocated and instantiated object.
> > + * If an instance of @klass is not permitted to be instantiated, an
> > + * error will be raised. This can happen if @klass is abstract.
> > + *
> > + * Returns: The newly allocated and instantiated object, or NULL
> > + * on error.
> >   */
> > -Object *object_new_with_class(ObjectClass *klass);
> > +Object *object_new_with_class(ObjectClass *klass, Error **errp);
> >  
> >  /**
> >   * object_new:
> > diff --git a/net/net.c b/net/net.c
> > index d9b23a8f8c..7fb5e966f3 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -944,11 +944,13 @@ GPtrArray *qemu_get_nic_models(const char *device_type)
> >               * create this property during instance_init, so we have to create
> >               * a temporary instance here to be able to check it.
> >               */
> > -            Object *obj = object_new_with_class(OBJECT_CLASS(dc));
> > -            if (object_property_find(obj, "netdev")) {
> > -                g_ptr_array_add(nic_models, (gpointer)name);
> > +            Object *obj = object_new_with_class(OBJECT_CLASS(dc), NULL);
> 
> One trivial comment: I kind of understand why NULL was chosen, but I don't
> think it's easily understandable on why it's better.
> 
> When object_new() can have side effect and might fail, logically it could
> be better that it asserts failure here when new NICs added (that can start
> to fail it here.. while we shouldn't have such now), instead of silently
> not showing up in the module list.  So at least we notify the net developer
> something might be off (while IIUC this function is trying to list all NIC
> modules QEMU supports).

This change is a mistake. This commit ought to be keeping failure
behaviour broadly the same, just pushing error handling up a level.
So from that POV, I should have used &error_abort here.

Any relaxation of error handling to be more graceful here ought
to be a separately justified commit.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC 3/5] convert code to object_new_dynamic() where appropriate

[Daniel P. Berrangé]

On Thu, Oct 31, 2024 at 03:21:17PM -0400, Peter Xu wrote:
> On Thu, Oct 31, 2024 at 03:53:48PM +0000, Daniel P. Berrangé wrote:
> > In cases where object_new() is not being passed a static, const
> > string, the caller cannot be sure what type they are instantiating.
> > There is a risk that instantiation could fail, if it is an abstract
> > type.
> > 
> > Convert such cases over to use object_new_dynamic() such that they
> > are forced to expect failure. In some cases failure can be easily
> > propagated, but in others using &error_abort maintainers existing
> > behaviour.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  chardev/char.c                   |  5 ++++-
> >  hw/core/bus.c                    |  2 +-
> >  hw/core/cpu-common.c             |  2 +-
> >  hw/core/qdev.c                   |  4 ++--
> >  hw/i386/x86-common.c             |  5 ++++-
> >  hw/i386/xen/xen-pvh.c            |  2 +-
> >  hw/vfio/common.c                 |  6 +++++-
> >  hw/vfio/container.c              |  6 +++++-
> >  qom/object_interfaces.c          |  7 ++-----
> >  qom/qom-qmp-cmds.c               | 15 +++++++++------
> >  tests/unit/check-qom-interface.c |  3 ++-
> >  tests/unit/test-smp-parse.c      | 20 ++++++++++----------
> >  12 files changed, 46 insertions(+), 31 deletions(-)
> 
> I think we could leave the test cases alone without _dynamic(), because
> they do test static types (even if they used "opaque"..), and they should
> really (and forever) assert on failures..
> 
> IMHO we should keep _dynamic() usages small if we'd like to introduce it,
> only in the paths where its failure will be properly handled.  Basically, I
> think we shouldn't use _dynamic() if we know it'll be error_abort.. because
> that's fundamentally identical to object_new().

For the sake of other readers, here's what you already figured out from
looking at patch 5...

The end of this series will enforce that the argument to object_new()
is a static, const string. It will become a compile error to pass a
variable to object_new(), and thus all such cases need switching to
object_new_dynamic(), even if they just end up passing in &error_abort
in some cases. 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC 4/5] qom: introduce object_new_dynamic()

[Daniel P. Berrangé]

On Thu, Oct 31, 2024 at 03:22:07PM -0400, Peter Xu wrote:
> On Thu, Oct 31, 2024 at 03:53:49PM +0000, Daniel P. Berrangé wrote:
> > object_new() has a failure scenario where it will assert() if given
> > an abstract type. Callers which are creating objects based on user
> > input, or unknown/untrusted type names, must manually check the
> > result of object_class_is_abstract() before calling object_new()
> > to propagate an Error, instead of asserting.
> > 
> > Introduce a object_new_dynamic() method which is a counterpart to
> > object_new() that directly returns an Error, instead of asserting.
> > This new method is to be used where the typename is specified
> > dynamically by code separate from the immediate caller.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Needs some patch order changes.. v.s. the previous one.

Opps, yes, of course.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|