After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 765406 - libsecret caching of GDBusProxy is incompatible with sync GMainContexts
libsecret caching of GDBusProxy is incompatible with sync GMainContexts
Status: RESOLVED OBSOLETE
Product: libsecret
Classification: Other
Component: General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: libsecret maintainer(s)
libsecret maintainer(s)
: 747669 765101 779786 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2016-04-22 01:34 UTC by Giovanni Campagna
Modified: 2018-09-21 16:25 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Giovanni Campagna 2016-04-22 01:34:32 UTC
All GDBus events are always delivered on the thread-default main context where a watch or proxy is created.

Unfortunately this breaks for sync invocation, because they push a temporary main context, iterate it for a bit, and never iterate it again.

In particular, this causes libsecret to fail to realize when gnome-keyring-daemon goes down for whatever reason (including a temporary g-k-d being replaced by the real g-k-d after login), because the NameOwnerChanged signal is never delivered.

One simple solution is to stop caching GDBusProxies altogether. This might add some overhead though.
Another more complex solution is a long-running worker thread that runs a dedicated loop.
Comment 1 Debarshi Ray 2017-05-29 15:12:40 UTC
*** Bug 779786 has been marked as a duplicate of this bug. ***
Comment 2 Debarshi Ray 2017-06-26 20:13:09 UTC
(In reply to Giovanni Campagna from comment #0)
> One simple solution is to stop caching GDBusProxies altogether. This might
> add some overhead though. Another more complex solution is a long-running
> worker thread that runs a dedicated loop.

A long-running worker thread would also add overhead, wouldn't it? A GIO-based program already starts at least 4 threads at start-up: (a) glib worker (b) gdbus worker (c) gtask's threadpool worker (d) dconf worker. It makes me cringe to add another to the mix. Maybe it isn't a problem in practice?

I guess one can workaround this by initializing the SecretService cache before the thread-default GMainContext is pushed. eg., at program start-up or at the entry-point to a library. Except, libsecret has this mechanism to clean up the cache when the D-Bus name vanishes. I wonder why is that. GDBusProxies should be able to transparently handle that, aren't they?

From an API design standpoint, it seems to me that we are better off letting the client code deal with caching. I like the convenience of having libsecret do it for me, but not at the cost of bugs like this.
Comment 3 Giovanni Campagna 2017-06-26 21:05:14 UTC
Well, if you want to reduce the number of threads, one option is to use the glib worker thread, after patching glib to make g_get_worker_context() public.

I'm not quite sure how you would initialize the cache at entry point to the library, and still have a working secret_service_get_sync(). Unless you plan to iterate the default main context inside get_sync(), which might surprise applications.
Comment 4 Debarshi Ray 2017-06-27 08:55:10 UTC
(In reply to Giovanni Campagna from comment #3)
> I'm not quite sure how you would initialize the cache at entry point to the
> library, and still have a working secret_service_get_sync(). Unless you plan
> to iterate the default main context inside get_sync(), which might surprise
> applications.

By "a library", I meant any library that might be using libsecret internally (eg., libgoa-backend-1.0.so), not the libsecret library itself. The easiest way to avoid bugs like these might be to not have any caching in libsecret at all and let its clients do whatever they want.

The signal delivery in GDBusProxy depends on the GMainContext used by g_dbus_connection_signal_subscribe during its construction, and since secret_service_get_sync only pushes a fresh thread-default context after that, it should be ok.
Comment 5 Giovanni Campagna 2017-06-27 16:28:05 UTC
Ha, you're right. I was going from memory and assuming secret_service_get_sync would call the async version of itself under a thread default main context, but actually it doesn't, it uses the sync initializer.

So the solution is to change secret_service_[lookup/search/...]_sync to call secret_service_get_sync() before pushing the main context.
(They are the one causing the problem)

Simple!
Comment 6 Debarshi Ray 2017-07-13 19:16:55 UTC
(In reply to Giovanni Campagna from comment #5)
> Ha, you're right. I was going from memory and assuming
> secret_service_get_sync would call the async version of itself under a
> thread default main context, but actually it doesn't, it uses the sync
> initializer.
> 
> So the solution is to change secret_service_[lookup/search/...]_sync to call
> secret_service_get_sync() before pushing the main context.
> (They are the one causing the problem)

Even though secret_service_get_sync is careful enough to use the global GMainContext for signal delivery, I doubt that it makes it simple to arrive at a proper solution.

The call chain is:
  secret_password_lookup_sync
  -> secret_password_lookupv_sync
     -> push new thread-default GMainContext
        secret_password_lookupv
        -> secret_service_lookup
           -> secret_service_get

libsecret could seed its cache by calling secret_service_get_sync before pushing the new GMainContext in secret_password_lookupv_sync. However, that seems very fragile. How would libsecret know that the thread-default GMainContext at that point is the correct one? The client code might have already pushed a short-lived GMainContext around it and we will be back to square one.

At the very least, it should be the client code's responsibility to seed the cache. However, the libsecret's cache clean-up might get in the way because the client won't know that the cache needs to be re-seeded. I don't understand why the cache needs to be cleaned up. GDBusProxies should be able to transparently restart the service, no?

It seems to me that non-trivial users of the API should stay away from the secret_password* convenience wrappers. Instead they should create their own SecretService instance, cache it if needed, and use the slightly lower level secret_service* APIs. That's what I am aiming for in gnome-online-accounts:wip/rishi/libsecret-workaround
Comment 7 Debarshi Ray 2017-07-17 17:13:05 UTC
I am playing around with the patches in bug 784944 (or gnome-online-accounts:wip/rishi/libsecret-workaround). I have a SecretService instance that's created under the global default GMainContext, and I manually killed and restarted gnome-keyring-daemon. However, secret_service_lookup_sync fails with:
    org.freedesktop.Secret.Error.NoSession: The session does not exist

I believe the org.freedesktop.secrets client needs to call OpenSession to re-establish the encrypted session. One possibility is to NULLify the old session by listening to SecretService::g-name-owner notifications. The application code can call secret_service_ensure_session everytime it does something that requires it.
Comment 8 Debarshi Ray 2017-07-17 17:23:19 UTC
*** Bug 765101 has been marked as a duplicate of this bug. ***
Comment 9 Debarshi Ray 2017-07-18 16:29:00 UTC
(In reply to Debarshi Ray from comment #2)
> I guess one can workaround this by initializing the SecretService cache
> before the thread-default GMainContext is pushed. eg., at program start-up
> or at the entry-point to a library. Except, libsecret has this mechanism to
> clean up the cache when the D-Bus name vanishes. I wonder why is that.
> GDBusProxies should be able to transparently handle that, aren't they?

I suppose this is because of the need to re-establish the session (comment 7).
Comment 10 Debarshi Ray 2017-10-24 18:32:46 UTC
*** Bug 747669 has been marked as a duplicate of this bug. ***
Comment 11 GNOME Infrastructure Team 2018-09-21 16:25:52 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libsecret/issues/9.