From ad2041a292caa7b33dab947c064977e4830eb762 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 31 Jan 2025 12:30:07 +0100 Subject: [PATCH] gobject: Be consistent in using atomic logic to handle the GParamSpecPool We init it atomically but then we don't really use it in such way and it may lead to races at read/write times --- gobject/gobject.c | 69 +++++++++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 26 deletions(-) diff --git a/gobject/gobject.c b/gobject/gobject.c index d7f3d7a91..8ab8e26e3 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -228,7 +228,7 @@ static GQuark quark_closure_array = 0; static GQuark quark_weak_notifies = 0; static GQuark quark_toggle_refs = 0; static GQuark quark_notify_queue; -static GParamSpecPool *pspec_pool = NULL; +static GParamSpecPool *pspec_pool = NULL; /* atomic */ static gulong gobject_signals[LAST_SIGNAL] = { 0, }; static guint (*floating_flag_handler) (GObject*, gint) = object_floating_flag_handler; static GQuark quark_weak_locations = 0; @@ -919,15 +919,22 @@ _g_object_type_init (void) * first caller of this function will win the race. Any other access to * the GParamSpecPool is done under its own mutex. */ -static inline void -g_object_init_pspec_pool (void) +static inline GParamSpecPool * +g_object_maybe_init_pspec_pool (void) { - if (G_UNLIKELY (g_atomic_pointer_get (&pspec_pool) == NULL)) + GParamSpecPool *pool = g_atomic_pointer_get (&pspec_pool); + + if (G_UNLIKELY (pool == NULL)) { - GParamSpecPool *pool = g_param_spec_pool_new (TRUE); - if (!g_atomic_pointer_compare_and_exchange (&pspec_pool, NULL, pool)) - g_param_spec_pool_free (pool); + GParamSpecPool *new_pool = g_param_spec_pool_new (TRUE); + if (g_atomic_pointer_compare_and_exchange_full (&pspec_pool, NULL, + new_pool, &pool)) + pool = g_steal_pointer (&new_pool); + + g_clear_pointer (&new_pool, g_param_spec_pool_free); } + + return pool; } static void @@ -954,18 +961,19 @@ static void g_object_base_class_finalize (GObjectClass *class) { GList *list, *node; + GParamSpecPool *param_spec_pool; _g_signals_destroy (G_OBJECT_CLASS_TYPE (class)); g_slist_free (class->construct_properties); class->construct_properties = NULL; class->n_construct_properties = 0; - list = g_param_spec_pool_list_owned (pspec_pool, G_OBJECT_CLASS_TYPE (class)); + param_spec_pool = g_atomic_pointer_get (&pspec_pool); + list = g_param_spec_pool_list_owned (param_spec_pool, G_OBJECT_CLASS_TYPE (class)); for (node = list; node; node = node->next) { GParamSpec *pspec = node->data; - - g_param_spec_pool_remove (pspec_pool, pspec); + g_param_spec_pool_remove (param_spec_pool, pspec); PARAM_SPEC_SET_PARAM_ID (pspec, 0); g_param_spec_unref (pspec); } @@ -981,7 +989,7 @@ g_object_do_class_init (GObjectClass *class) quark_toggle_refs = g_quark_from_static_string ("GObject-toggle-references"); quark_notify_queue = g_quark_from_static_string ("GObject-notify-queue"); - g_object_init_pspec_pool (); + g_object_maybe_init_pspec_pool (); class->constructor = g_object_constructor; class->constructed = g_object_constructed; @@ -1049,11 +1057,12 @@ install_property_internal (GType g_type, guint property_id, GParamSpec *pspec) { + GParamSpecPool *param_spec_pool; g_param_spec_ref_sink (pspec); - g_object_init_pspec_pool (); + param_spec_pool = g_object_maybe_init_pspec_pool (); - if (g_param_spec_pool_lookup (pspec_pool, pspec->name, g_type, FALSE)) + if (g_param_spec_pool_lookup (param_spec_pool, pspec->name, g_type, FALSE)) { g_critical ("When installing property: type '%s' already has a property named '%s'", g_type_name (g_type), @@ -1063,7 +1072,7 @@ install_property_internal (GType g_type, } PARAM_SPEC_SET_PARAM_ID (pspec, property_id); - g_param_spec_pool_insert (pspec_pool, g_steal_pointer (&pspec), g_type); + g_param_spec_pool_insert (param_spec_pool, g_steal_pointer (&pspec), g_type); return TRUE; } @@ -1116,7 +1125,8 @@ validate_and_install_class_property (GObjectClass *class, /* for property overrides of construct properties, we have to get rid * of the overridden inherited construct property */ - pspec = g_param_spec_pool_lookup (pspec_pool, pspec->name, parent_type, TRUE); + pspec = g_param_spec_pool_lookup (g_atomic_pointer_get (&pspec_pool), + pspec->name, parent_type, TRUE); if (pspec && pspec->flags & (G_PARAM_CONSTRUCT | G_PARAM_CONSTRUCT_ONLY)) { class->construct_properties = g_slist_remove (class->construct_properties, pspec); @@ -1232,7 +1242,7 @@ find_pspec (GObjectClass *class, } } - return g_param_spec_pool_lookup (pspec_pool, + return g_param_spec_pool_lookup (g_atomic_pointer_get (&pspec_pool), property_name, ((GTypeClass *)class)->g_type, TRUE); @@ -1475,13 +1485,14 @@ g_object_interface_find_property (gpointer g_iface, const gchar *property_name) { GTypeInterface *iface_class = g_iface; + GParamSpecPool *param_spec_pool; g_return_val_if_fail (G_TYPE_IS_INTERFACE (iface_class->g_type), NULL); g_return_val_if_fail (property_name != NULL, NULL); - g_object_init_pspec_pool (); + param_spec_pool = g_object_maybe_init_pspec_pool (); - return g_param_spec_pool_lookup (pspec_pool, + return g_param_spec_pool_lookup (param_spec_pool, property_name, iface_class->g_type, FALSE); @@ -1518,6 +1529,7 @@ g_object_class_override_property (GObjectClass *oclass, guint property_id, const gchar *name) { + GParamSpecPool *param_spec_pool; GParamSpec *overridden = NULL; GParamSpec *new; GType parent_type; @@ -1526,11 +1538,13 @@ g_object_class_override_property (GObjectClass *oclass, g_return_if_fail (property_id > 0); g_return_if_fail (name != NULL); + param_spec_pool = g_atomic_pointer_get (&pspec_pool); + /* Find the overridden property; first check parent types */ parent_type = g_type_parent (G_OBJECT_CLASS_TYPE (oclass)); if (parent_type != G_TYPE_NONE) - overridden = g_param_spec_pool_lookup (pspec_pool, + overridden = g_param_spec_pool_lookup (param_spec_pool, name, parent_type, TRUE); @@ -1544,7 +1558,7 @@ g_object_class_override_property (GObjectClass *oclass, ifaces = g_type_interfaces (G_OBJECT_CLASS_TYPE (oclass), &n_ifaces); while (n_ifaces-- && !overridden) { - overridden = g_param_spec_pool_lookup (pspec_pool, + overridden = g_param_spec_pool_lookup (param_spec_pool, name, ifaces[n_ifaces], FALSE); @@ -1583,7 +1597,7 @@ g_object_class_list_properties (GObjectClass *class, g_return_val_if_fail (G_IS_OBJECT_CLASS (class), NULL); - pspecs = g_param_spec_pool_list (pspec_pool, + pspecs = g_param_spec_pool_list (g_atomic_pointer_get (&pspec_pool), G_OBJECT_CLASS_TYPE (class), &n); if (n_properties_p) @@ -1616,14 +1630,15 @@ g_object_interface_list_properties (gpointer g_iface, guint *n_properties_p) { GTypeInterface *iface_class = g_iface; + GParamSpecPool *param_spec_pool; GParamSpec **pspecs; guint n; g_return_val_if_fail (G_TYPE_IS_INTERFACE (iface_class->g_type), NULL); - g_object_init_pspec_pool (); + param_spec_pool = g_object_maybe_init_pspec_pool (); - pspecs = g_param_spec_pool_list (pspec_pool, + pspecs = g_param_spec_pool_list (param_spec_pool, iface_class->g_type, &n); if (n_properties_p) @@ -1960,7 +1975,7 @@ g_object_notify (GObject *object, * (by, e.g. calling g_object_class_find_property()) * because g_object_notify_queue_add() does that */ - pspec = g_param_spec_pool_lookup (pspec_pool, + pspec = g_param_spec_pool_lookup (g_atomic_pointer_get (&pspec_pool), property_name, G_OBJECT_TYPE (object), TRUE); @@ -2213,6 +2228,7 @@ object_interface_check_properties (gpointer check_data, { GTypeInterface *iface_class = g_iface; GObjectClass *class; + GParamSpecPool *param_spec_pool; GType iface_type = iface_class->g_type; GParamSpec **pspecs; guint n; @@ -2225,11 +2241,12 @@ object_interface_check_properties (gpointer check_data, if (!G_IS_OBJECT_CLASS (class)) goto out; - pspecs = g_param_spec_pool_list (pspec_pool, iface_type, &n); + param_spec_pool = g_atomic_pointer_get (&pspec_pool); + pspecs = g_param_spec_pool_list (param_spec_pool, iface_type, &n); while (n--) { - GParamSpec *class_pspec = g_param_spec_pool_lookup (pspec_pool, + GParamSpec *class_pspec = g_param_spec_pool_lookup (param_spec_pool, pspecs[n]->name, G_OBJECT_CLASS_TYPE (class), TRUE);