From 1f86923766a3d1d319fe54ad24fcf6e2d75aca0d Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 12:40:49 +0000
Subject: [PATCH 1/3] gdbusinterfaceskeleton: Remove an unnecessary helper
 struct member
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The `GDBusInterfaceSkeleton` is already stored as the source object of
the `GTask` here, with a strong reference.

Storing it again in the task’s data struct is redundant, and makes it
look like the `GDBusInterfaceSkeleton` is being used without holding a
strong reference. (There’s not actually a bug there though: the strong
reference from the `GTask` outlives the data struct, so is sufficient.)

Remove the unnecessary helper struct member to clarify the code a bit.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: #2924
---
 gio/gdbusinterfaceskeleton.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/gio/gdbusinterfaceskeleton.c b/gio/gdbusinterfaceskeleton.c
index 3f07d4d0b2..d28282fea3 100644
--- a/gio/gdbusinterfaceskeleton.c
+++ b/gio/gdbusinterfaceskeleton.c
@@ -461,7 +461,6 @@ dbus_interface_interface_init (GDBusInterfaceIface *iface)
 typedef struct
 {
   gint ref_count;  /* (atomic) */
-  GDBusInterfaceSkeleton       *interface;
   GDBusInterfaceMethodCallFunc  method_call_func;
   GDBusMethodInvocation        *invocation;
 } DispatchData;
@@ -502,16 +501,17 @@ dispatch_in_thread_func (GTask        *task,
                          GCancellable *cancellable)
 {
   DispatchData *data = task_data;
+  GDBusInterfaceSkeleton *interface = g_task_get_source_object (task);
   GDBusInterfaceSkeletonFlags flags;
   GDBusObject *object;
   gboolean authorized;
 
-  g_mutex_lock (&data->interface->priv->lock);
-  flags = data->interface->priv->flags;
-  object = data->interface->priv->object;
+  g_mutex_lock (&interface->priv->lock);
+  flags = interface->priv->flags;
+  object = interface->priv->object;
   if (object != NULL)
     g_object_ref (object);
-  g_mutex_unlock (&data->interface->priv->lock);
+  g_mutex_unlock (&interface->priv->lock);
 
   /* first check on the enclosing object (if any), then the interface */
   authorized = TRUE;
@@ -519,13 +519,13 @@ dispatch_in_thread_func (GTask        *task,
     {
       g_signal_emit_by_name (object,
                              "authorize-method",
-                             data->interface,
+                             interface,
                              data->invocation,
                              &authorized);
     }
   if (authorized)
     {
-      g_signal_emit (data->interface,
+      g_signal_emit (interface,
                      signals[G_AUTHORIZE_METHOD_SIGNAL],
                      0,
                      data->invocation,
@@ -627,7 +627,6 @@ g_dbus_interface_method_dispatch_helper (GDBusInterfaceSkeleton       *interface
       DispatchData *data;
 
       data = g_slice_new0 (DispatchData);
-      data->interface = interface;
       data->method_call_func = method_call_func;
       data->invocation = invocation;
       data->ref_count = 1;
-- 
GitLab


From d5710deb9d621bcf0cec0ff2db0708f361490752 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 12:47:36 +0000
Subject: [PATCH 2/3] gdbusinterfaceskeleton: Fix a use-after-free of a
 GDBusMethodInvocation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This `GDBusMethodInvocation` may be shared across threads, with no
guarantee on the strong ref in one thread outlasting any refs in other
threads — so it needs a ref in this helper struct.

This should fix a use-after-free where the `GDBusMethodInvocation` is
freed from `g_value_unset()` after `g_signal_emit()` returns in
`dispatch_in_thread_func()` in one thread; but then dereferenced again
in `g_source_destroy_internal()` from another thread.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Fixes: #2924
---
 gio/gdbusinterfaceskeleton.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/gio/gdbusinterfaceskeleton.c b/gio/gdbusinterfaceskeleton.c
index d28282fea3..a2a79fe3d8 100644
--- a/gio/gdbusinterfaceskeleton.c
+++ b/gio/gdbusinterfaceskeleton.c
@@ -462,14 +462,17 @@ typedef struct
 {
   gint ref_count;  /* (atomic) */
   GDBusInterfaceMethodCallFunc  method_call_func;
-  GDBusMethodInvocation        *invocation;
+  GDBusMethodInvocation        *invocation;  /* (owned) */
 } DispatchData;
 
 static void
 dispatch_data_unref (DispatchData *data)
 {
   if (g_atomic_int_dec_and_test (&data->ref_count))
-    g_slice_free (DispatchData, data);
+    {
+      g_clear_object (&data->invocation);
+      g_slice_free (DispatchData, data);
+    }
 }
 
 static DispatchData *
@@ -628,7 +631,7 @@ g_dbus_interface_method_dispatch_helper (GDBusInterfaceSkeleton       *interface
 
       data = g_slice_new0 (DispatchData);
       data->method_call_func = method_call_func;
-      data->invocation = invocation;
+      data->invocation = g_object_ref (invocation);
       data->ref_count = 1;
 
       task = g_task_new (interface, NULL, NULL, NULL);
-- 
GitLab


From 7b101588e924f3783a0f5075f06b3e1d698be936 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 22 Feb 2023 12:50:10 +0000
Subject: [PATCH 3/3] gdbusconnection: Make GDBusMethodInvocation transfer a
 bit clearer

Add a missing steal call in `schedule_method_call()`. This introduces no
functional changes, but documents the ownership transfer more clearly.

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>

Helps: #2924
---
 gio/gdbusconnection.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index d938f71b99..da6b66f2ec 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -5043,7 +5043,7 @@ schedule_method_call (GDBusConnection            *connection,
   g_source_set_priority (idle_source, G_PRIORITY_DEFAULT);
   g_source_set_callback (idle_source,
                          call_in_idle_cb,
-                         invocation,
+                         g_steal_pointer (&invocation),
                          g_object_unref);
   g_source_set_static_name (idle_source, "[gio, " __FILE__ "] call_in_idle_cb");
   g_source_attach (idle_source, main_context);
-- 
GitLab