Re: [PATCH 1/2] qom: Reject temporary object resurrection

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jun 15, 2026 at 01:11:06PM +0900, Akihiko Odaki wrote:
> If object_ref() is called during finalization, it will temporarily
> "resurrect" the object. Although object_finalize() asserts that no
> resurrecting reference remains before freeing the object, the assertion
> cannot catch the case where the resurrecting reference is dropped before
> freeing the object.

What's the real world problem here ? 

> 
> One way to catch this would be to add a check in object_ref() to reject
> resurrecting references before they are created. However, object_ref()
> is frequently called so it is better to minimize the overhead.
> 
> To avoid adding the overhead, change how the reference count is
> represented and let an existing assertion detect resurrection. More
> concretely, obj->ref now stores the current reference count minus 1. The
> stored count is therefore 0 after object_initialize(), and the final
> object_unref() decrements it from 0 to UINT32_MAX and starts
> finalization. A resurrecting object_ref() will then trip the existing
> obj->ref < INT_MAX assertion.

Please no, this is horribly surprising semantics.

> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>  include/qom/object.h            |  4 ++--
>  block/throttle-groups.c         |  2 +-
>  qom/object.c                    | 15 ++++++---------
>  tests/unit/check-qom-proplist.c |  8 ++++----
>  4 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 11f55613fcd0..c4dcf65dcf7f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1121,7 +1121,7 @@ GSList *object_class_get_list_sorted(const char *implements_type,
>   * @obj: the object
>   *
>   * Increase the reference count of a object.  A object cannot be freed as long
> - * as its reference count is greater than zero.
> + * as a reference remains.
>   * Returns: @obj
>   */
>  Object *object_ref(void *obj);
> @@ -1131,7 +1131,7 @@ Object *object_ref(void *obj);
>   * @obj: the object
>   *
>   * Decrease the reference count of a object.  A object cannot be freed as long
> - * as its reference count is greater than zero.
> + * as a reference remains.
>   */
>  void object_unref(void *obj);
>  
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index 4b1b1944c20a..3b2a89f5849d 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -962,7 +962,7 @@ static void throttle_group_get_limits(Object *obj, Visitor *v,
>  
>  static bool throttle_group_can_be_deleted(UserCreatable *uc)
>  {
> -    return OBJECT(uc)->ref == 1;
> +    return OBJECT(uc)->ref == 0;
>  }
>  
>  static void throttle_group_obj_class_init(ObjectClass *klass,
> diff --git a/qom/object.c b/qom/object.c
> index 0ac201de4c15..761eff1337e6 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -497,7 +497,6 @@ 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);
> @@ -560,11 +559,10 @@ bool object_initialize_child_with_propsv(Object *parentobj,
>  
>  out:
>      /*
> -     * We want @obj's reference to be 1 on success, 0 on failure.
> -     * On success, it's 2: one taken by object_initialize(), and one
> -     * by object_property_add_child().
> -     * On failure in object_initialize() or earlier, it's 1.
> -     * On failure afterwards, it's also 1: object_unparent() releases
> +     * We want @obj's reference to be 0 on success, UINT32_MAX on failure.
> +     * On success, it's 1: one taken by object_property_add_child().
> +     * On failure in object_initialize() or earlier, it's 0.
> +     * On failure afterwards, it's also 0: object_unparent() releases
>       * the reference taken by object_property_add_child().
>       */
>      object_unref(obj);
> @@ -668,7 +666,6 @@ static void object_finalize(void *data)
>      object_property_del_all(obj);
>      object_deinit(obj, ti);
>  
> -    g_assert(obj->ref == 0);
>      g_assert(obj->parent == NULL);
>      if (obj->free) {
>          obj->free(obj);
> @@ -1329,10 +1326,10 @@ void object_unref(void *objptr)
>      if (!obj) {
>          return;
>      }
> -    g_assert(obj->ref > 0);
> +    g_assert(obj->ref < INT_MAX);
>  
>      /* parent always holds a reference to its children */
> -    if (qatomic_fetch_dec(&obj->ref) == 1) {
> +    if (qatomic_fetch_dec(&obj->ref) == 0) {
>          object_finalize(obj);
>      }
>  }
> diff --git a/tests/unit/check-qom-proplist.c b/tests/unit/check-qom-proplist.c
> index 89de92b7d91f..b5527dd52afd 100644
> --- a/tests/unit/check-qom-proplist.c
> +++ b/tests/unit/check-qom-proplist.c
> @@ -351,7 +351,7 @@ static void test_dummy_createv_tree(void)
>                                NULL));
>  
>      g_assert(err == NULL);
> -    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
> +    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
>      g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
>      g_assert(dobj->bv == true);
>      g_assert(dobj->av == DUMMY_PLATYPUS);
> @@ -375,7 +375,7 @@ static void test_dummy_createv_parentless(void)
>                                           NULL));
>  
>      g_assert(err == NULL);
> -    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
> +    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
>      g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
>      g_assert(dobj->bv == true);
>      g_assert(dobj->av == DUMMY_PLATYPUS);
> @@ -414,7 +414,7 @@ static void test_dummy_createlist_tree(void)
>                          NULL));
>  
>      g_assert(err == NULL);
> -    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
> +    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
>      g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
>      g_assert(dobj->bv == true);
>      g_assert(dobj->av == DUMMY_PLATYPUS);
> @@ -450,7 +450,7 @@ static void test_dummy_createlist_parentless(void)
>                                NULL));
>  
>      g_assert(err == NULL);
> -    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
> +    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
>      g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
>      g_assert(dobj->bv == true);
>      g_assert(dobj->av == DUMMY_PLATYPUS);
> 
> -- 
> 2.54.0
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|