Several users in Debian complained too about gnome-keyring 3.18.x being broken.

Some of the comments on https://bugs.debian.org/800617 suggest that there may be a conflict (or a race condition) between gnome-keyring process launched from autostart file and the process launched via d-bus service activation.

Please read the linked bug for the full users' stories.

Cosimo, this seems related to the GDBus port? Would you have a chance to investigate? Thanks ... I'm swamped at present.

It can be that the patch from bug 756006 (committed yesterday) fixes the issue. I have applied it in our Debian packages, but haven't yet got any feedback.

There is some feedback now, and the user says that this bug is *not* fixed. He also gives some information that may be helpful.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=800617#49

Created attachment 313008 [details] [review]
main: be careful to avoid spinning a main loop before fork()

gnome-keyring-daemon supports many modes of operation; some of those
modes require the program to call fork(), in order to daemonize or run
in the background.

GMainContext/GMainLoop need careful handling when they're used from
programs that fork() themselves. In particular, they're not compatible
with being run in the parent process and later being run in the child.

Since our port to GDBus, discovering whether there is another running
daemon involve a synchronous call using GDBusConnection, which is
implemented in GIO by creating a GMainLoop and waiting for the method to
return.

Currently gnome-keyring-daemon will trigger the discovery early in its
initialization, while still in the parent process; unfortunately, due to
the reasons above, this will cause any later GDBus calls in the child
process to fail.

This commit fixes the issue by moving the fork() logic earlier in the
initialization process.

Attached patch should fix this issue. Testing would be appreciated.

(In reply to Cosimo Cecchi from comment #6)
> Attached patch should fix this issue. Testing would be appreciated.

Hi Cosimo. This bug is present on OpenBSD as well and I can confirm your patch fixes it :-)
Thank you.

Cosimo, thanks for the patch. I uploaded the new version to Debian with it applied, let's wait for the feedback.

Cosimo, after applying your patch the /daemon/startup/control/valid test fails (and also it does not clean up its children so the testsuite hangs when ran during Debian package build).

dmitry@lenovo:/tmp/gnome-keyring-3.18.0$ ps aux | grep startup | grep -v grep
dmitry@lenovo:/tmp/gnome-keyring-3.18.0$ ./test-startup 2>error.log
/daemon/startup/replace: OK
/daemon/startup/control/valid: Aborted
dmitry@lenovo:/tmp/gnome-keyring-3.18.0$ ps aux | grep startup | grep -v grep
dmitry    5247  0.0  0.0  35468   344 pts/0    S    21:42   0:00 ./test-startup
dmitry    5259  0.0  0.1 267684  7032 pts/0    Sl   21:42   0:00 /tmp/gnome-keyring-3.18.0/gnome-keyring-daemon --foreground --control-directory /tmp/scratch-test-startup.US505X/xxxx --components=
dmitry@lenovo:/tmp/gnome-keyring-3.18.0$ cat error.log
gnome-keyring-daemon: insufficient process capabilities, unsecure memory might get used
gnome-keyring-daemon: insufficient process capabilities, unsecure memory might get used
** Message: Replacing daemon, using directory: /tmp/keyring-test-two/keyring
gnome-keyring-daemon: insufficient process capabilities, unsecure memory might get used
**
ERROR:daemon/test-startup.c:92:test_control_valid: assertion failed (g_environ_getenv (output, "GNOME_KEYRING_CONTROL") == fixed): (NULL == "/tmp/scratch-test-startup.US505X/xxxx")

The warnings about insufficient capabilities can be ignored as they were present before applying this patch too.

Created attachment 313140 [details] [review]
main: be careful to avoid spinning a main loop before fork()

--

Thanks for testing - this new patch should address that.

Review of attachment 313140 [details] [review]:

This breaks the use cases of non GNOME users. People expect the following commands to work:

$ eval $(gnome-keyring-daemon)
$ eval $(gnome-keyring-daemon --daemonize)

And people expect this command to print out environment variables:

$ gnome-keyring-daemon --foreground

Even if we do at some point to decide to move away from supporting environment variables, we would still not be able to make such a change in a released point 3.18.1 version of gnome-keyring.

According to https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=800617#86 and https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=800617#91 Cosimo's patch still does not fix the issue.

*** Bug 756708 has been marked as a duplicate of this bug. ***

There are similar patches in bug 756708 and on the wip/fork-fixes branch that cosimoc and I are iterating on.

more discussion:

<stefw> cosimoc, halfline i have 45 minutes or so
<stefw> is there anything you want me to review?
<cosimoc> stefw, hey sort of
<cosimoc> we've been putting some fixes in a branch with halfline, but it does not seem to work properly :(
<stefw> is it wip/fork-fixes?
<halfline> yea
<halfline> cosimoc: doesn't work properly ? uh oh
<cosimoc> yeah - I have some other things locally
<cosimoc> halfline, the testsuite fails
<cosimoc> test-dbus-search gets stuck
<halfline> oh let me try
<halfline> did you get this follow up fix? https://git.gnome.org/browse/gnome-keyring/commit/?h=wip/fork-fixes&id=59b86fd6f1e2afb6674e9e0933a29cebeea13264 ?
<cosimoc> halfline, I pushed my commits on fork-fixes
<flo> hergertme: is there any vala project you know where vala support works good enough to check it out?
<halfline> oh okay
<cosimoc> oh!
<cosimoc> no I did not
<cosimoc> hold on
<halfline> cosimoc: eek, not sure ginputstream/goutputstream is a good idea
<cosimoc> why not?
<halfline> cosimoc: actually maybe it's okay
<halfline> cosimoc: okay i changed my mind
<cosimoc> halfline, the follow up fix works great
<cosimoc> :)
<halfline> cosimoc: i was worried about complexity before fork, but they're actually going to run after fork
<halfline> so it's fine
<halfline> cosimoc: oh good
<cosimoc> halfline, pushed the fix again on top of the branch. I also think there is another problem where read_login_password() should run before fork()
<cosimoc> do you agree?
<stefw> it should, yes
<halfline> why ?
* halfline boggles
<stefw> the whole reason --login reads the password from stdin is selinux
<stefw> mclasen did the work long ago
<halfline> sure, but why does it need to run before fork?
<stefw> but basically connecting to the DBus socket (was unix socket back then) fell over with selinux
<stefw> halfline, are we leaving stdin open across the fork?
* stefw looks
<cosimoc> stefw, halfline, running to lunch now, but I pushed all I had on the branch
<cosimoc> it's ready for a review for what I'm concerned
<halfline> stefw: yea, the patch i did closes stdout after fork
<halfline> err stdin
<halfline> well all 3
<halfline> cosimoc: thanks
<stefw> halfline, so reading from STDIN won't work after fork
<stefw> needs to happen before
<cosimoc> make check also passes \o/
<halfline> thats what you guys keep saying, but i don't understand why!
<stefw> in theory we could just move that code befor ethe fork
<stefw> halfline, because the resulting daemon doesn't have the STDIN
<halfline> why wouldn't it?
<halfline> the fd's don't get closed in the child
<halfline> until after its read
<stefw> aha
<stefw> so why the whole print_environment_from_fds dance?
<stefw> when could just write to stdout?
<halfline> heh
<halfline> god you're asking a great question
* stefw tries to remove it
<stefw> lets see
<stefw> sorry just catching up with this last batch of changes
<halfline> i wasn't thinking when i wrote it i guess
<halfline> my thought process was "keep parity with what's there, but move fork earlier"
<halfline> but there's no reason the print has to come from the parent indeed
<halfline> so we can dramatically simplify the patch
<halfline> maybe just use a GWakeup instead of a pipe
<stefw> i don't see the syncronization point
<halfline> stefw: read is a blocking call
<stefw> halfline, ah right makes sense
<halfline> stefw: so it wakes up when the read finishes
<stefw> ok, lets just close it
<stefw> close(parent_wakeup_fd)
<stefw> should i add a patch
<stefw> or have you already done it?
<halfline> haven't done it yet
<halfline> oh weird gwakeup doesn't have a sync wait function
<halfline> so we'd have to introduce g_poll() or something
<halfline> yea closing the fd is better
<stefw> what is this gwakeup you're talking about?
<stefw> not in the docs
<stefw> internal to glib?
<halfline> i don't think it's internal
<halfline> it's a wrapper around a pipe/ evenfd depending on platform
<halfline> but it's not a good fit anyway
<halfline> and if it's not in the docs it's either internal or too new to depend on
<halfline> so forget i brought it up
<halfline> it's what GCancellable uses 
<stefw> ok testing

Further work on wip/fork-fixes.

Fixes autologin according to my testing.
Fixes secret retrieval when gnome-keyring started by PAM.

$ secret-tool store --label="Hello" hello value
Password: 
$ secret-tool lookup hello value
test

Created attachment 313497 [details] [review]
daemon: fork before threads are spawned

It's not really a good idea to fork after glib has initialized,
since it has helper threads that may have taken locks etc.

This commit forks really early to prevent locks from leaking
and causing deadlock.

Signed-off-by: Cosimo Cecchi <cosimoc@gnome.org>
 * Split out separate function
 * Check return value of g_unix_open_pipe()
 * Don't fork when running foreground
 * Read login password before fork()

Signed-off-by: Stef Walter <stefw@gnome.org>
 * Since stdout is open, just print evironment directly

Created attachment 313498 [details] [review]
daemon: adjust CFLAGS for daemon

Created attachment 313499 [details] [review]
daemon: Kill off foreground proceses when session dies

Right now gnome-keyring will keep processes around forever
in some cases.  They need to die when the session goes away,
at least.

Signed-off-by: Cosimo Cecchi <cosimoc@gnome.org>
 * Don't leak a DBus connection

Signed-off-by: Stef Walter <stefw@gnome.org>
 * Print error message if we cannot connect to bus
 * Wait for gnome-keyring-daemon when --foreground

Attachment 313497 [details] pushed as 6040bdb - daemon: fork before threads are spawned
Attachment 313498 [details] pushed as ac9492a - daemon: adjust CFLAGS for daemon
Attachment 313499 [details] pushed as bf560bd - daemon: Kill off foreground proceses when session dies

These patches will be included in gnome-keyring 3.18.1

*** Bug 756398 has been marked as a duplicate of this bug. ***

Works for me. Thanks!

Not sure if it belongs here or new ticket should be created but I still experience one issue when using keyring integration extension for firefox (https://github.com/swick/mozilla-gnome-keyring). With gnome-keyring 3.18.0/3.18.1 firefox hangs when trying to access keyring and following messages are logged:

paź 20 14:55:57 cukinia gcr-prompter[18553]: Gcr: calling the PromptReady method on /org/gnome/keyring/Prompt/p5@:1.210
paź 20 14:55:57 cukinia gcr-prompter[18553]: acquired name: org.gnome.keyring.SystemPrompter
paź 20 14:55:57 cukinia gcr-prompter[18553]: acquired name: org.gnome.keyring.PrivatePrompter
paź 20 14:55:57 cukinia gcr-prompter[18553]: Gcr: returned from the PromptReady method on /org/gnome/keyring/Prompt/p5@:1.210
paź 20 14:55:57 cukinia gnome-keyring-daemon[17481]: could not register secret unlock prompt on session bus: An object is already exported for the interface org.freedesktop.Secret.Prompt at /org/freedesktop/secrets/prompt/p1
paź 20 14:55:57 cukinia gnome-keyring-daemon[17481]: GLib-GIO: g_dbus_interface_skeleton_unexport: assertion 'interface_->priv->connections != NULL' failed
paź 20 14:55:57 cukinia org.freedesktop.secrets[2004]: ** (gnome-keyring-daemon:17481): WARNING **: could not register secret unlock prompt on session bus: An object is already exported for the interface org.freedesktop.Secret.Prompt at /org/freedesktop/secrets/prompt/p1
paź 20 14:55:57 cukinia org.freedesktop.secrets[2004]: (gnome-keyring-daemon:17481): GLib-GIO-CRITICAL **: g_dbus_interface_skeleton_unexport: assertion 'interface_->priv->connections != NULL' failed

If keyring deamon is killed firefox "unhangs".

> paź 20 14:55:57 cukinia org.freedesktop.secrets[2004]: ** (gnome-keyring-daemon:17481): WARNING **: could not register secret unlock prompt on session bus: An object is already exported for the interface org.freedesktop.Secret.Prompt at /org/freedesktop/secrets/prompt/p1

That's a different bug #756032