| Message ID | 20240915145339.1368029-1-alexjlzheng@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qom: fix NULL pointer in object_initialize_with_type() | expand |
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 >
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
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); }