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

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

[alexjlzheng]

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
	          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().

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);
 }
-- 
2.41.1

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

[Jinliang Zheng]

ping

<alexjlzheng@gmail.com> 于2024年9月15日周日 22:53写道：
>
> 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
>                   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().
>
> 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);
>  }
> --
> 2.41.1
>

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