From 60e9deb2b0a42e63e02f4ebd819291a37103e882 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 10 Dec 2024 15:53:54 +0000 Subject: [PATCH 1/3] Revert "gdbus: Fix leak of method invocation when registering an object with closures" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 092fedd5f085a2f1966b5c34befe8b603c1a0f07. This was not the right change to make, and I shouldn’t have accepted the MR. The situation is laid out in this comment: https://gitlab.gnome.org/GNOME/glib/-/issues/2600#note_1385050 tl;dr: The reference on the `GDBusMethodInvocation` which is transferred in to the `GDBusInterfaceMethodCallFunc` is balanced by a reference transferred to `g_dbus_method_invocation_return_*()`. This is how the refcounting has always worked for these functions, and even if we’d probably arrange things differently if the code was written now, we can’t change those semantics without breaking API. In particular, bindings have various bits of custom code to account for these reference tranfers (since they can’t be represented using gobject-introspection annotations), so changing the semantics will break bindings. Fixes: #3559 --- gio/gdbusconnection.c | 2 +- gio/tests/gdbus-export.c | 19 +------------------ 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index c2107bcaff..3587a0928f 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -5950,7 +5950,7 @@ register_with_closures_on_method_call (GDBusConnection *connection, g_value_set_variant (¶ms[5], parameters); g_value_init (¶ms[6], G_TYPE_DBUS_METHOD_INVOCATION); - g_value_take_object (¶ms[6], g_steal_pointer (&invocation)); + g_value_set_object (¶ms[6], invocation); g_closure_invoke (data->method_call_closure, NULL, G_N_ELEMENTS (params), params, NULL); diff --git a/gio/tests/gdbus-export.c b/gio/tests/gdbus-export.c index 5be560013c..599df5bb56 100644 --- a/gio/tests/gdbus-export.c +++ b/gio/tests/gdbus-export.c @@ -161,23 +161,6 @@ foo_method_call (GDBusConnection *connection, } } -static void -foo_method_call_with_closure (GDBusConnection *connection, - const gchar *sender, - const gchar *object_path, - const gchar *interface_name, - const gchar *method_name, - GVariant *parameters, - GDBusMethodInvocation *invocation, - gpointer user_data) -{ - /* The call below takes ownership of the invocation but ownership is not - * passed into the callback so get an additional reference here */ - g_object_ref (invocation); - - foo_method_call (connection, sender, object_path, interface_name, method_name, parameters, invocation, user_data); -} - static GVariant * foo_get_property (GDBusConnection *connection, const gchar *sender, @@ -1457,7 +1440,7 @@ test_object_registration_with_closures (void) registration_id = g_dbus_connection_register_object_with_closures (c, "/foo/boss", (GDBusInterfaceInfo *) &foo_interface_info, - g_cclosure_new (G_CALLBACK (foo_method_call_with_closure), NULL, NULL), + g_cclosure_new (G_CALLBACK (foo_method_call), NULL, NULL), g_cclosure_new (G_CALLBACK (foo_get_property), NULL, NULL), g_cclosure_new (G_CALLBACK (foo_set_property), NULL, NULL), &error); -- GitLab From 1b66bf4122a1e5b881b2ed132e79beb5c54b75e4 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 10 Dec 2024 15:59:04 +0000 Subject: [PATCH 2/3] =?UTF-8?q?gdbusconnection:=20Add=20a=20comment=20expl?= =?UTF-8?q?aining=20why=20an=20invocation=20is=20=E2=80=98leaked=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s not leaked, it’s transferred forwards to the eventual `g_dbus_method_invocation_return_*()` call. Signed-off-by: Philip Withnall Helps: #3559 --- gio/gdbusconnection.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 3587a0928f..a4b57cfc6e 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -5950,6 +5950,11 @@ register_with_closures_on_method_call (GDBusConnection *connection, g_value_set_variant (¶ms[5], parameters); g_value_init (¶ms[6], G_TYPE_DBUS_METHOD_INVOCATION); + /* NOTE: This is deliberately *not* g_value_take_object(). A reference to + * `invocation` is transferred in to this function, and it needs to be + * transferred onwards to the `g_dbus_method_invocation_return_*()` method + * call which must eventually happen (either in the closure function, or in + * a delayed consequence from it). Changing this will break API. */ g_value_set_object (¶ms[6], invocation); g_closure_invoke (data->method_call_closure, NULL, G_N_ELEMENTS (params), params, NULL); -- GitLab From 557d2e498def230c40a8d785d4eaf2d86ec69137 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 10 Dec 2024 16:59:33 +0000 Subject: [PATCH 3/3] gdbusconnection: Document existing refcount semantics of closures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As per the previous few commits, explicitly document the established reference counting semantics of the method call closure for `g_dbus_connection_register_object_with_closures()`. This isn’t ideal, but `g_dbus_connection_register_object_with_closures()` has had these semantics for 10 years now, and it’s a bit late to change them to something neater. Signed-off-by: Philip Withnall Helps: #3559 --- gio/gdbusconnection.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index a4b57cfc6e..8e8e16afaa 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -6089,6 +6089,11 @@ register_with_closures_on_set_property (GDBusConnection *connection, * Version of g_dbus_connection_register_object() using closures instead of a * #GDBusInterfaceVTable for easier binding in other languages. * + * Note that the reference counting semantics of the function wrapped by + * @method_call_closure are the same as those of + * [callback@Gio.DBusInterfaceMethodCallFunc]: ownership of a reference to the + * [class@Gio.DBusMethodInvocation] is transferred to the function. + * * Returns: 0 if @error is set, otherwise a registration ID (never 0) * that can be used with g_dbus_connection_unregister_object() . * -- GitLab