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

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

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 :|