Re: [PATCH] qom: fix NULL pointer in object_initialize_with_type()

[Peter Maydell]

On Sun, 15 Sept 2024 at 17:12, <alexjlzheng@gmail.com> wrote:
>
> From: Jinliang Zheng <alexjlzheng@tencent.com>
>
> Currently, object_initialize_with_type() calls 
> object_class_property_init_all()
> before initializing Object->properties. This may cause Object->properties to
> still be NULL when we call object_property_add() on Object.
>
> For exmaple, if we extend DEFINE_PROP_ARRAY() to a version with a default 
> value
> other than 0:
>         #define DEFINE_PROP_ARRAY_EXAMPLE(_name, _state, _field,        \
>                                 _arrayfield, _arrayprop, _arraytype)    \
>                 DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name),              \
>                         _state, _field, qdev_prop_arraylen_virtio_net,  \
>                         uint32_t,                                       \
>                         .set_default = true,                            \
>                         .defval.u = <non-zero>,                         \
>                         .arrayinfo = &(_arrayprop),                     \
>                         .arrayfieldsize = sizeof(_arraytype),           \
>                         .arrayoffset = offsetof(_state, _arrayfield))
> We should have:
>         object_initialize_with_type
>           object_class_property_init_all
>             ObjectProperty->init() / object_property_init_defval
>               ...
>                 set_prop_arraylen

There's no set_prop_arraylen function in the codebase. Which
function do you mean here ?

>                   object_property_add
>                     object_property_try_add
>                       g_hash_table_insert(Object->properties)   <- NULL
>           obj->properties = g_hash_table_new_full()             <- 
> initializing
>
> This patch fixes the above problem by exchanging the order of 
> Ojbect->properties
> initialization and object_class_property_init_all().

So this happens only if the initializer for a class property attempts
to add a property to the object, right? That seems quite niche,
and it would be good to clarify that in the commit message.

I do wonder if it's something we ever intended to support.
In particular note that you cannot currently validly add a *class*
property to the class in the initializer for a class property
(because it would invalidate the iterator over the class properties).

Do you have a more concrete example of something you want to do
that you're currently running into this with?

> Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> ---
>  qom/object.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 157a45c5f8..734b52f048 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -556,9 +556,9 @@ static void object_initialize_with_type(Object *obj, 
> size_t size, TypeImpl *type
>      memset(obj, 0, type->instance_size);
>      obj->class = type->class;
>      object_ref(obj);
> -    object_class_property_init_all(obj);
>      obj->properties = g_hash_table_new_full(g_str_hash, g_str_equal,
>                                              NULL, object_property_free);
> +    object_class_property_init_all(obj);
>      object_init_with_type(obj, type);
>      object_post_init_with_type(obj, type);
>  }

On the other hand doing the initialization in this order
is certainly safe, so if it's all we need to do to handle
class prop initializers adding object properties maybe it's
fine to do so.

thanks
-- PMM