From e05623001b0a945cd58981b9a3921ffbf7c0255b Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Sat, 23 Dec 2023 20:06:03 +0100
Subject: [PATCH] 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.
---
 gobject/gobject.c         | 76 ++++++++++++++++++++++++++-------------
 gobject/tests/reference.c |  4 +--
 2 files changed, 53 insertions(+), 27 deletions(-)

diff --git a/gobject/gobject.c b/gobject/gobject.c
index aa3b851b0..10676c7fb 100644
--- a/gobject/gobject.c
+++ b/gobject/gobject.c
@@ -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_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)
+/* Internal implementation of g_object_ref() which doesn't call out to user code.
+ * @out_toggle_notify and @out_toggle_data *must* be provided, and if non-`NULL`
+ * 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
+ * only be called once all locks are released. */
+static gpointer
+object_ref (GObject *object,
+            GToggleNotify *out_toggle_notify,
+            gpointer *out_toggle_data)
 {
-  GObject *object = _object;
   GToggleNotify toggle_notify;
   gpointer toggle_data;
   gint old_ref;
 
-  g_return_val_if_fail (G_IS_OBJECT (object), NULL);
-
   old_ref = g_atomic_int_get (&object->ref_count);
 
 retry:
   toggle_notify = NULL;
+  toggle_data = NULL;
   if (old_ref > 1 && old_ref < G_MAXINT)
     {
       /* Fast-path. We have apparently more than 1 references already. No
@@ -3803,12 +3795,43 @@ retry:
     {
       gboolean object_already_finalized = TRUE;
 
+      *out_toggle_notify = NULL;
+      *out_toggle_data = NULL;
       g_return_val_if_fail (!object_already_finalized, NULL);
       return NULL;
     }
 
   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)
     toggle_notify (toggle_data, object, FALSE);
 
@@ -5113,20 +5136,25 @@ g_weak_ref_clear (GWeakRef *weak_ref)
 gpointer
 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);
 
-  object_or_null = weak_ref->priv.p;
+  object = weak_ref->priv.p;
 
-  if (object_or_null != NULL)
-    g_object_ref (object_or_null);
+  if (object)
+    object = object_ref (object, &toggle_notify, &toggle_data);
 
   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
diff --git a/gobject/tests/reference.c b/gobject/tests/reference.c
index 7b73eb725..1fbe7e7ea 100644
--- a/gobject/tests/reference.c
+++ b/gobject/tests/reference.c
@@ -731,11 +731,9 @@ weak_ref_in_toggle_notify_toggle_cb (gpointer data,
     return;
 
   /* 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_assert_true (object == g_weak_ref_get (&weak2));