GNOME Bugzilla – Bug 622905
Migrate to glib's GDBus
Last modified: 2015-11-21 22:11:57 UTC
For GLib 2.25.5 GDBus D-Bus support was merged, providing an API to replace dbus-glib. See http://library.gnome.org/devel/gio/unstable/gdbus.html and http://library.gnome.org/devel/gio/unstable/ch28.html . According to a quick grep this module seems to use code copied from dbus-glib: ./gnome-keyring/egg/egg-dbus.c: * Copied from dbus-gmain.c due to API instabilities in dbus-glib bindings. :(
This also applies for libgnome-keyring.
We don't use dbus-glib and just copied a few functions that provide integration into the glib mainloop. Other than that we just use dbus directly. Porting the whole thing over to gdbus is an interesting exercise, but unless someone volunteers to do this, it'll have to go at the bottom of my list of things to do. It's a ton of work, and there's simply too much other stuff to do before rewriting stuff.
Anyone willing to invest time into this? If not I'm going to close this bug. libgnome-keyring is going away (replaced by libsecret), so this only applies to gnome-keyring-daemon.
Created attachment 290247 [details] [review] Port gnome-keyring-daemon to GDBus This commit ports gnome-keyring-daemon from libdbus1 to GDBus. Unfortunately it's not really possible to split this commit into multiple while keeping the code building. Noteworthy is the different approach that GDBus allows us to use; instead of filtering every message and writing our own internal message routing, we can more naturally use generated skeletons and register/unregister them on the bus as needed.
Created attachment 290248 [details] [review] egg: remove egg-dbus.[ch] They're not used anymore.
Created attachment 290249 [details] [review] build: remove dbus checks We don't depend on libdbus anymore.
Created attachment 290250 [details] [review] test: adapt for gdbus-codegen PropertiesChanged behavior Before using gdbus-codegen for the secret objects, property signals would be emitted synchronously before returning the method call. Generated GDBus skeletons will queue signalling of property changes in an idle, so we can't rely on the same timing behavior as before. See code comment for more details.
Created attachment 290251 [details] [review] Update .gitignore
Here's my attempt at this. Note that the code is still only lightly tested, but at least make check passes.
Created attachment 290253 [details] [review] Port gnome-keyring-daemon to GDBus -- Fix some warnings at login I've seen in testing
Awesome. Do you have a branch? I can't apply these patches to gnome-keyring git master, so I'm assuming I'm getting them in the wrong order?
Review of attachment 290253 [details] [review]: A bit of review to start ... will do more later. ::: daemon/dbus/gkd-dbus-environment.c @@ +48,1 @@ + g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), result, &error); Memory leak here of the return value when error is not set. ::: daemon/dbus/gkd-dbus-session.c @@ +71,3 @@ + if (client_session_signal_id) { + g_dbus_connection_signal_unsubscribe (conn, client_session_signal_id); + client_session_signal_id = 0; client_session_signal_id is not set to non-zero anywhere that I can see. ::: daemon/dbus/gkd-dbus.c @@ +144,3 @@ /* First register the object */ if (!object_registered) { + GError *error = NULL; Defining error twice in the same function with the same name is confusing.
Thanks, Stef. I fixed your comments and pushed another additional fix in this branch: https://git.gnome.org/browse/gnome-keyring/log/?h=wip/gdbus
Thanks. Needs a minor rebase onto git master. Some more review: This test fails on the wip/gdbus branch: $ ./test-dbus-search /secret-search/service-search-items-unlocked-separate: ** Message: couldn't set environment variable in session: GDBus.Error:org.gnome.SessionManager.NotInInitialization: Setenv interface is only available during the DisplayServer and Initialization phase ** Message: open session ** (gnome-keyring-daemon:3878): CRITICAL **: gkd_secret_service_get_pkcs11_session: assertion 'client' failed ** ERROR:../daemon/dbus/test-service.c:121:test_service_setup: assertion failed (error == NULL): GDBus.Error:org.freedesktop.DBus.Error.NoReply: Message did not receive a reply (timeout by message bus) (g-dbus-error-quark, 4) Aborted (core dumped) + --generate-c-code $(srcdir)/daemon/dbus/gkd-daemon-generated \ Code should not be generated in $(srcdir) by any of the usual 'make' targets. $(builddir) is the right place for that. + g_dbus_connection_call_sync (conn, + SERVICE_SESSION_MANAGER, + client_session_path, + IFACE_SESSION_PRIVATE, + "EndSessionResponse", + g_variant_new ("(bs)", + is_ok, + reason), + NULL, + G_DBUS_CALL_FLAGS_NONE, 1000, + NULL, &error); Leak of return value. + g_dbus_connection_call_sync (conn, + SERVICE_SESSION_MANAGER, + PATH_SESSION_MANAGER, + IFACE_SESSION_MANAGER, + "UnregisterClient", + g_variant_new ("(o)", client_session_path), + NULL, G_DBUS_CALL_FLAGS_NONE, + -1, NULL, NULL); This is a leak of the return value. +gkd_secret_propagate_error (GDBusMethodInvocation *invocation, ... - g_return_val_if_fail (error != NULL, NULL); We should keep this precondition check. + g_dbus_method_invocation_return_error_literal (invocation, + G_DBUS_ERROR, + G_DBUS_ERROR_FAILED, + "Couldn't create new collection"); Hmmm, this was broken before, but I think this should actually be set to @description as the error message. + g_set_error (error_out, G_DBUS_ERROR, + G_DBUS_ERROR_FAILED, + "Object does not have the '%s' property", + prop_name); + return FALSE; + } Hmmm, while we're rewriting this ... I think this should be G_DBUS_ERROR_UNKNOWN_PROPERTY. - if (!gkd_secret_util_parse_path (path, collection, item)) + /* Retrieve the actual attribute value */ + if (!gkd_secret_property_parse_variant (value, prop_name, &builder)) { + gck_builder_clear (&builder); + g_set_error (error_out, G_DBUS_ERROR, + G_DBUS_ERROR_FAILED, + "The property type or value was invalid: %s", + prop_name); I think these should be G_DBUS_INVALID_ARGS + if (!gkd_secret_property_get_type (prop_name, &attr.type)) { + g_set_error (error_out, G_DBUS_ERROR, + G_DBUS_ERROR_FAILED, + "Object does not have the '%s' property", + prop_name); + return NULL; + } Once again, this should be G_DBUS_ERROR_UNKNOWN_PROPERTY. + while (g_variant_iter_loop (&iter, "{sv}", &name, &variant)) { /* Property interface.name */ - g_return_val_if_fail (dbus_message_iter_get_arg_type (&dict) == DBUS_TYPE_STRING, FALSE); - dbus_message_iter_get_basic (&dict, &name); - dbus_message_iter_next (&dict); - if (!property_to_attribute (name, interface, &attr_type, &data_type)) return FALSE; /* Property value */ - g_return_val_if_fail (dbus_message_iter_get_arg_type (&dict) == DBUS_TYPE_VARIANT, FALSE); - if (!iter_get_variant (&dict, data_type, attr_type, builder)) + if (!iter_get_variant (variant, data_type, attr_type, builder)) return FALSE; - This is a leak, on return from a g_variant_iter_loop().
Thanks, Stef. I think I fixed all of your comments in the branch now.
Stef, FYI I pushed a rebased and cleaned up version of the branch.
Review of attachment 290253 [details] [review]: Thanks! This looks nearly good to merge. Go ahead and merge into git master once the following issues are solved. * The generated code shouldn't be written to $(srcdir). Please write generated code to $(builddir). * There's a merge conflict against git master. ::: daemon/dbus/gkd-dbus-environment.c @@ +48,1 @@ + g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), result, &error); There is still a memory leak here of the return value. ::: daemon/dbus/gkd-dbus-session.c @@ +71,3 @@ + if (client_session_signal_id) { + g_dbus_connection_signal_unsubscribe (conn, client_session_signal_id); + client_session_signal_id = 0; client_session_signal_id is never set. ::: daemon/dbus/gkd-dbus.c @@ +144,3 @@ /* First register the object */ if (!object_registered) { + GError *error = NULL; The error variable has the same name as another variable in the same funcition. Could you change it?
Stef, I think all your comments above were already fixed in the branch I mentioned above. I did not bother reattaching all the commits here, but I can do that if you want to take another look at them. Sorry for the confusion!
Ah no worries. Will do a last check on the branch, and merge. Awesome work by the way. You're a machine.
Merged from branch. Made some changes to fix build and merge issues, but nothing significant. Attachment 290248 [details] pushed as 1d26d1a - egg: remove egg-dbus.[ch] Attachment 290249 [details] pushed as 0805192 - build: remove dbus checks Attachment 290250 [details] pushed as a590a9f - test: adapt for gdbus-codegen PropertiesChanged behavior Attachment 290251 [details] pushed as ed390c9 - Update .gitignore Attachment 290253 [details] pushed as c459da1 - Port gnome-keyring-daemon to GDBus
Looks like there are srcdir != builddir from git issues at least: http://build.gnome.org/continuous/buildmaster/builds/2015/06/29/58/build/log-gnome-keyring.txt
So after running this for a while, I feel like I merged this prematurely. I've fixed several bugs, but some major regressions remain. * The way that objects are dispatched is now global. Previously many objects were per caller. * Objects are exported when created rather than when explicitly published. I guess I'll need to find some time to back out some of the changes, particularly those to do with dispatching code. I was hopeing to use g_dbus_connection_register_subtree() as a way to fix this up, but despite its name it just falls over with nested objects.
Sorry about this; I am not sure I entirely follow your two points here and what consequences they imply. What do you mean that objects were "dispatched per caller"? What does it mean to "explicitly publish" an object? I would be willing to work on fixing these problems given some more detail.
> What do you mean that objects were "dispatched per caller"? For better or worse, certain objects, like sessions, prompts have only been exposed to certain callers. > What does it mean to "explicitly publish" an object? Previously we used to call gkd_secret_service_publish_dispatch() to export an object. Now it's done during the constructor. You can see how this was used in the unlock code daemon/dbus/gkd-secret-unlock.c(), where we replace one prompt object with another implementation. My reaction was to do some work to keep the old dispatch logic, and just move to GVariant. But if you're going to help ... then I wouldn't be stuck on that.
Created attachment 308895 [details] [review] dbus: do not handle methods when they don't match initial caller Match the previous behavior in GkdSecretPrompt, GkdSecretSession and GkdSecretUnlock, where we do not reply to dbus methods that are being invoked for a caller different than the one initially requesting the object.
(In reply to Stef Walter from comment #24) > > What do you mean that objects were "dispatched per caller"? > > For better or worse, certain objects, like sessions, prompts have only been > exposed to certain callers. OK; the attached patch above should restore the previous behavior. > > What does it mean to "explicitly publish" an object? > > Previously we used to call gkd_secret_service_publish_dispatch() to export > an object. Now it's done during the constructor. You can see how this was > used in the unlock code daemon/dbus/gkd-secret-unlock.c(), where we replace > one prompt object with another implementation. > > My reaction was to do some work to keep the old dispatch logic, and just > move to GVariant. But if you're going to help ... then I wouldn't be stuck > on that. I am not sure I completely understand the problem with this, and I did not change at all the implementation of gkd_secret_service_publish_dispatch() as far as I can see with git blame. What's wrong with exporting the objects during construction? Does something rely on a different behavior and breaks?
With the new version, I cannot create a new secret item with the encrypted session (via D-Bus API). I get "The secret was transferred or encrypted in an invalid way." message, while everything was OK with the gnome-keyring-daemon 3.16. Quick debugging shows that the warning comes from handling of CKR_WRAPPED_KEY_INVALID in daemon/dbus/gkd-secret-session.c. Is that a known issue? (FWIW I tested with https://github.com/mitya57/secretstorage, the error is also caught by the testsuite.)
(In reply to Dmitry Shachnev from comment #27) > With the new version, I cannot create a new secret item with the encrypted > session (via D-Bus API). I get "The secret was transferred or encrypted in > an invalid way." message, while everything was OK with the > gnome-keyring-daemon 3.16. > > Quick debugging shows that the warning comes from handling of > CKR_WRAPPED_KEY_INVALID in daemon/dbus/gkd-secret-session.c. > > Is that a known issue? > > (FWIW I tested with https://github.com/mitya57/secretstorage, the error is > also caught by the testsuite.) Dmitry, the issue is not known to me; thanks for linking to the testsuite - I will try and reproduce against it.
The issue does reproduce for me, but I haven't been able to track it down yet. Stef, I pushed the patch above and a few other minor changes into this branch: https://git.gnome.org/browse/gnome-keyring/log/?h=wip/cosimoc/gdbus-fixes Also any thoughts about my question above?
Thanks. Merging this one after testing it for a bit. Attachment 308895 [details] pushed as 5b59762 - dbus: do not handle methods when they don't match initial caller
Stef, good news on the regressions front - I spent some time working on this yesterday night and finally found a few problems. Expect more patches later.
This branch now contains a set of fixes that should address all the issues mentioned in this report. The secretstorage testsuite mentioned above by Dmitry also now passes. https://git.gnome.org/browse/gnome-keyring/log/?h=wip/cosimoc/gdbus-fixes Would be great to get these in for 3.18.
Review of attachment 308895 [details] [review]: This was pushed
Fixes in the branch were pushed to master after IRC review by Stef. Keeping this open as Stef is chasing another last bug.
Thanks Cosimo and Stefan, 3.17.91 works fine for me.
I think this can be closed now.