1From 1f86923766a3d1d319fe54ad24fcf6e2d75aca0d Mon Sep 17 00:00:00 2001 2From: Philip Withnall <pwithnall@endlessos.org> 3Date: Wed, 22 Feb 2023 12:40:49 +0000 4Subject: [PATCH 1/3] gdbusinterfaceskeleton: Remove an unnecessary helper 5 struct member 6MIME-Version: 1.0 7Content-Type: text/plain; charset=UTF-8 8Content-Transfer-Encoding: 8bit 9 10The `GDBusInterfaceSkeleton` is already stored as the source object of 11the `GTask` here, with a strong reference. 12 13Storing it again in the task’s data struct is redundant, and makes it 14look like the `GDBusInterfaceSkeleton` is being used without holding a 15strong reference. (There’s not actually a bug there though: the strong 16reference from the `GTask` outlives the data struct, so is sufficient.) 17 18Remove the unnecessary helper struct member to clarify the code a bit. 19 20Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 21 22Helps: #2924 23--- 24 gio/gdbusinterfaceskeleton.c | 15 +++++++-------- 25 1 file changed, 7 insertions(+), 8 deletions(-) 26 27diff --git a/gio/gdbusinterfaceskeleton.c b/gio/gdbusinterfaceskeleton.c 28index 3f07d4d0b2..d28282fea3 100644 29--- a/gio/gdbusinterfaceskeleton.c 30+++ b/gio/gdbusinterfaceskeleton.c 31@@ -461,7 +461,6 @@ dbus_interface_interface_init (GDBusInterfaceIface *iface) 32 typedef struct 33 { 34 gint ref_count; /* (atomic) */ 35- GDBusInterfaceSkeleton *interface; 36 GDBusInterfaceMethodCallFunc method_call_func; 37 GDBusMethodInvocation *invocation; 38 } DispatchData; 39@@ -502,16 +501,17 @@ dispatch_in_thread_func (GTask *task, 40 GCancellable *cancellable) 41 { 42 DispatchData *data = task_data; 43+ GDBusInterfaceSkeleton *interface = g_task_get_source_object (task); 44 GDBusInterfaceSkeletonFlags flags; 45 GDBusObject *object; 46 gboolean authorized; 47 48- g_mutex_lock (&data->interface->priv->lock); 49- flags = data->interface->priv->flags; 50- object = data->interface->priv->object; 51+ g_mutex_lock (&interface->priv->lock); 52+ flags = interface->priv->flags; 53+ object = interface->priv->object; 54 if (object != NULL) 55 g_object_ref (object); 56- g_mutex_unlock (&data->interface->priv->lock); 57+ g_mutex_unlock (&interface->priv->lock); 58 59 /* first check on the enclosing object (if any), then the interface */ 60 authorized = TRUE; 61@@ -519,13 +519,13 @@ dispatch_in_thread_func (GTask *task, 62 { 63 g_signal_emit_by_name (object, 64 "authorize-method", 65- data->interface, 66+ interface, 67 data->invocation, 68 &authorized); 69 } 70 if (authorized) 71 { 72- g_signal_emit (data->interface, 73+ g_signal_emit (interface, 74 signals[G_AUTHORIZE_METHOD_SIGNAL], 75 0, 76 data->invocation, 77@@ -627,7 +627,6 @@ g_dbus_interface_method_dispatch_helper (GDBusInterfaceSkeleton *interface 78 DispatchData *data; 79 80 data = g_slice_new0 (DispatchData); 81- data->interface = interface; 82 data->method_call_func = method_call_func; 83 data->invocation = invocation; 84 data->ref_count = 1; 85-- 86GitLab 87 88 89From d5710deb9d621bcf0cec0ff2db0708f361490752 Mon Sep 17 00:00:00 2001 90From: Philip Withnall <pwithnall@endlessos.org> 91Date: Wed, 22 Feb 2023 12:47:36 +0000 92Subject: [PATCH 2/3] gdbusinterfaceskeleton: Fix a use-after-free of a 93 GDBusMethodInvocation 94MIME-Version: 1.0 95Content-Type: text/plain; charset=UTF-8 96Content-Transfer-Encoding: 8bit 97 98This `GDBusMethodInvocation` may be shared across threads, with no 99guarantee on the strong ref in one thread outlasting any refs in other 100threads — so it needs a ref in this helper struct. 101 102This should fix a use-after-free where the `GDBusMethodInvocation` is 103freed from `g_value_unset()` after `g_signal_emit()` returns in 104`dispatch_in_thread_func()` in one thread; but then dereferenced again 105in `g_source_destroy_internal()` from another thread. 106 107Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 108 109Fixes: #2924 110--- 111 gio/gdbusinterfaceskeleton.c | 9 ++++++--- 112 1 file changed, 6 insertions(+), 3 deletions(-) 113 114diff --git a/gio/gdbusinterfaceskeleton.c b/gio/gdbusinterfaceskeleton.c 115index d28282fea3..a2a79fe3d8 100644 116--- a/gio/gdbusinterfaceskeleton.c 117+++ b/gio/gdbusinterfaceskeleton.c 118@@ -462,14 +462,17 @@ typedef struct 119 { 120 gint ref_count; /* (atomic) */ 121 GDBusInterfaceMethodCallFunc method_call_func; 122- GDBusMethodInvocation *invocation; 123+ GDBusMethodInvocation *invocation; /* (owned) */ 124 } DispatchData; 125 126 static void 127 dispatch_data_unref (DispatchData *data) 128 { 129 if (g_atomic_int_dec_and_test (&data->ref_count)) 130- g_slice_free (DispatchData, data); 131+ { 132+ g_clear_object (&data->invocation); 133+ g_slice_free (DispatchData, data); 134+ } 135 } 136 137 static DispatchData * 138@@ -628,7 +631,7 @@ g_dbus_interface_method_dispatch_helper (GDBusInterfaceSkeleton *interface 139 140 data = g_slice_new0 (DispatchData); 141 data->method_call_func = method_call_func; 142- data->invocation = invocation; 143+ data->invocation = g_object_ref (invocation); 144 data->ref_count = 1; 145 146 task = g_task_new (interface, NULL, NULL, NULL); 147-- 148GitLab 149 150 151From 7b101588e924f3783a0f5075f06b3e1d698be936 Mon Sep 17 00:00:00 2001 152From: Philip Withnall <pwithnall@endlessos.org> 153Date: Wed, 22 Feb 2023 12:50:10 +0000 154Subject: [PATCH 3/3] gdbusconnection: Make GDBusMethodInvocation transfer a 155 bit clearer 156 157Add a missing steal call in `schedule_method_call()`. This introduces no 158functional changes, but documents the ownership transfer more clearly. 159 160Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 161 162Helps: #2924 163--- 164 gio/gdbusconnection.c | 2 +- 165 1 file changed, 1 insertion(+), 1 deletion(-) 166 167diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c 168index d938f71b99..da6b66f2ec 100644 169--- a/gio/gdbusconnection.c 170+++ b/gio/gdbusconnection.c 171@@ -5043,7 +5043,7 @@ schedule_method_call (GDBusConnection *connection, 172 g_source_set_priority (idle_source, G_PRIORITY_DEFAULT); 173 g_source_set_callback (idle_source, 174 call_in_idle_cb, 175- invocation, 176+ g_steal_pointer (&invocation), 177 g_object_unref); 178 g_source_set_static_name (idle_source, "[gio, " __FILE__ "] call_in_idle_cb"); 179 g_source_attach (idle_source, main_context); 180-- 181GitLab 182 183