gobject: fix deadlock with g_weak_ref_set()

In general, we must not call out to external, unknown code while holding
a lock. That is prone to dead lock.

g_object_ref() can emit a toggle notification. In g_weak_ref_set(), we
must not do that while holding the lock.
This commit is contained in:
Thomas Haller 2023-12-23 20:06:03 +01:00
parent b397ef2122
commit e05623001b
2 changed files with 53 additions and 27 deletions

View File

@ -3751,33 +3751,25 @@ g_object_remove_toggle_ref (GObject *object,
g_critical ("%s: couldn't find toggle ref %p(%p)", G_STRFUNC, notify, data); g_critical ("%s: couldn't find toggle ref %p(%p)", G_STRFUNC, notify, data);
} }
/** /* Internal implementation of g_object_ref() which doesn't call out to user code.
* g_object_ref: * @out_toggle_notify and @out_toggle_data *must* be provided, and if non-`NULL`
* @object: (type GObject.Object): a #GObject * values are returned, then the caller *must* call that toggle notify function
* * as soon as it is safe to do so. It may call (or be) user-provided code so should
* Increases the reference count of @object. * only be called once all locks are released. */
* static gpointer
* Since GLib 2.56, if `GLIB_VERSION_MAX_ALLOWED` is 2.56 or greater, the type object_ref (GObject *object,
* of @object will be propagated to the return type (using the GCC typeof() GToggleNotify *out_toggle_notify,
* extension), so any casting the caller needs to do on the return type must be gpointer *out_toggle_data)
* explicit.
*
* Returns: (type GObject.Object) (transfer none): the same @object
*/
gpointer
(g_object_ref) (gpointer _object)
{ {
GObject *object = _object;
GToggleNotify toggle_notify; GToggleNotify toggle_notify;
gpointer toggle_data; gpointer toggle_data;
gint old_ref; gint old_ref;
g_return_val_if_fail (G_IS_OBJECT (object), NULL);
old_ref = g_atomic_int_get (&object->ref_count); old_ref = g_atomic_int_get (&object->ref_count);
retry: retry:
toggle_notify = NULL; toggle_notify = NULL;
toggle_data = NULL;
if (old_ref > 1 && old_ref < G_MAXINT) if (old_ref > 1 && old_ref < G_MAXINT)
{ {
/* Fast-path. We have apparently more than 1 references already. No /* Fast-path. We have apparently more than 1 references already. No
@ -3803,12 +3795,43 @@ retry:
{ {
gboolean object_already_finalized = TRUE; gboolean object_already_finalized = TRUE;
*out_toggle_notify = NULL;
*out_toggle_data = NULL;
g_return_val_if_fail (!object_already_finalized, NULL); g_return_val_if_fail (!object_already_finalized, NULL);
return NULL; return NULL;
} }
TRACE (GOBJECT_OBJECT_REF (object, G_TYPE_FROM_INSTANCE (object), old_ref)); TRACE (GOBJECT_OBJECT_REF (object, G_TYPE_FROM_INSTANCE (object), old_ref));
*out_toggle_notify = toggle_notify;
*out_toggle_data = toggle_data;
return object;
}
/**
* g_object_ref:
* @object: (type GObject.Object): a #GObject
*
* Increases the reference count of @object.
*
* Since GLib 2.56, if `GLIB_VERSION_MAX_ALLOWED` is 2.56 or greater, the type
* of @object will be propagated to the return type (using the GCC typeof()
* extension), so any casting the caller needs to do on the return type must be
* explicit.
*
* Returns: (type GObject.Object) (transfer none): the same @object
*/
gpointer
(g_object_ref) (gpointer _object)
{
GObject *object = _object;
GToggleNotify toggle_notify;
gpointer toggle_data;
g_return_val_if_fail (G_IS_OBJECT (object), NULL);
object = object_ref (object, &toggle_notify, &toggle_data);
if (toggle_notify) if (toggle_notify)
toggle_notify (toggle_data, object, FALSE); toggle_notify (toggle_data, object, FALSE);
@ -5113,20 +5136,25 @@ g_weak_ref_clear (GWeakRef *weak_ref)
gpointer gpointer
g_weak_ref_get (GWeakRef *weak_ref) g_weak_ref_get (GWeakRef *weak_ref)
{ {
gpointer object_or_null; GToggleNotify toggle_notify = NULL;
gpointer toggle_data = NULL;
GObject *object;
g_return_val_if_fail (weak_ref!= NULL, NULL); g_return_val_if_fail (weak_ref, NULL);
g_rw_lock_reader_lock (&weak_locations_lock); g_rw_lock_reader_lock (&weak_locations_lock);
object_or_null = weak_ref->priv.p; object = weak_ref->priv.p;
if (object_or_null != NULL) if (object)
g_object_ref (object_or_null); object = object_ref (object, &toggle_notify, &toggle_data);
g_rw_lock_reader_unlock (&weak_locations_lock); g_rw_lock_reader_unlock (&weak_locations_lock);
return object_or_null; if (toggle_notify)
toggle_notify (toggle_data, object, FALSE);
return object;
} }
static void static void

View File

@ -731,11 +731,9 @@ weak_ref_in_toggle_notify_toggle_cb (gpointer data,
return; return;
/* We just got a second ref, while calling g_weak_ref_get(). /* We just got a second ref, while calling g_weak_ref_get().
* At this point, we hold a lock for the weak ref.
* *
* FIXME: currently we would dead lock with the lines below. * Test that taking another weak ref in this situation works.
*/ */
return;
g_weak_ref_init (&weak2, object); g_weak_ref_init (&weak2, object);
g_assert_true (object == g_weak_ref_get (&weak2)); g_assert_true (object == g_weak_ref_get (&weak2));