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 756708 - fix deadlocks/stalls when using autologin
fix deadlocks/stalls when using autologin
Status: RESOLVED DUPLICATE of bug 756059
Product: gnome-keyring
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: GNOME keyring maintainer(s)
GNOME keyring maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-10-16 17:24 UTC by Ray Strode [halfline]
Modified: 2015-10-16 17:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
daemon: fork before threads are spawned (13.05 KB, patch)
2015-10-16 17:24 UTC, Ray Strode [halfline]
none Details | Review
daemon: kill off foreground proceses when session dies (2.67 KB, patch)
2015-10-16 17:25 UTC, Ray Strode [halfline]
none Details | Review

Description Ray Strode [halfline] 2015-10-16 17:24:52 UTC
Since the gdbus port gnome-keyring sometimes deadlocks at start up
when using autologin.

This is because it forks after starting the gdbus worker thread and,
of course, glib doesn't use pthread_atfork or anything like that to
clean up its locks.

Anyway, fork() should never be too far away from exec (either nearly
right before or nearly right after [just after the start of a
program]).  Anything else is risky.

It also stalls reboots, because gnome-keyring know longer opens a
connection to the display, and there are cases where it doesn't get
on the bus, so there's nothing to scope it the session.  I rectify
that by getting on the bus.  We'll probably have to tied it to
logind state down the road when we drop session buses in favor of
user buses.

Downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=1269581
Comment 1 Ray Strode [halfline] 2015-10-16 17:24:59 UTC
Created attachment 313492 [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.
Comment 2 Ray Strode [halfline] 2015-10-16 17:25:03 UTC
Created attachment 313493 [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.
Comment 3 Ray Strode [halfline] 2015-10-16 17:35:46 UTC
Review of attachment 313492 [details] [review]:

this is a bit of a first draft, that I threw together quickly to get in in time for a fedora test compose.  A few things i've noticed...

::: daemon/gkd-main.c
@@ +517,3 @@
+
+	output = g_malloc0 (output_size);
+	bytes_read = read (fd, output, output_size);

probably should either use recvmsg here or at least handle EINTR / short reads

@@ +541,3 @@
+	output = g_strjoinv ("\n", (gchar **) gkd_util_get_environment ());
+	output_size = strlen (output) + 1;
+	bytes_written = write (fd, &output_size, sizeof (output_size));

worrying about short writes is less of a concern but EINTR is still a problem. probably should just use sendmsg here.

@@ +665,1 @@
+	g_unix_open_pipe (wakeup_fds, FD_CLOEXEC, NULL);

need to check return value here

@@ +976,3 @@
+		sane_dup2 (fd, i);
+		close (fd);
+	}

for clarity sake this loop should probably get moved out of fork_and_print_environment in an earlier commit, maybe to a helper function of its own.
Comment 4 Ray Strode [halfline] 2015-10-16 17:36:45 UTC
Review of attachment 313493 [details] [review]:

::: daemon/gkd-main.c
@@ +934,3 @@
+				connection = g_bus_get_sync (G_BUS_TYPE_SESSION,
+							     NULL,
+							     NULL);

should probably pass an error and print it here.
Comment 5 Ray Strode [halfline] 2015-10-16 17:57:26 UTC
<halfline> cosimoc: stefw: can i get one or more of you to review https://bugzilla.gnome.org/show_bug.cgi?id=756708
<bugbot> Bug 756708: gnome-keyring, normal, Normal, ---, gnome-keyring-maint, NEW , fix deadlocks/stalls when using autologin
<cosimoc> halfline, thanks for looking into that - let me take a look
<halfline> cool thanks
<cosimoc> halfline, the "do not fork" patch is very similar to another one I wrote for another bug so that's good :)
<halfline> oh yea?
<halfline> if you already have another patch we can totally use it instead
<halfline> OR WE CAN COMBINE FORCES FOR A SUPER PATCH
<cosimoc> my patch is not as good as yours
<cosimoc> because it doesn
<cosimoc> doesn't print the environment in the parent 
<cosimoc> and there are things that rely on that apparently :(
<halfline> unfortunate
<halfline> means
<halfline> 1) things relying on environment variables
<halfline> 2) things relying on screen scraping
<halfline> we need to modernize our ecosystem
<halfline> or something
<cosimoc> yeah, not sure what those things are exactly, but see https://bugzilla.gnome.org/show_bug.cgi?id=756059
<bugbot> Bug 756059: gnome-keyring, normal, Normal, ---, gnome-keyring-maint, NEW , No secrets provided by gnome-keyring after upgrade to 3.18
<halfline> oh interesting
<cosimoc> halfline, why was the sleep() part not an issue before?
<halfline> i don't know.  at one point gnome-keyring used gtk
<halfline> but i think the gtk dependency was dropped before 3.16
<halfline> so not sure
<halfline> when it used gtk there was a display connection to force it to exit
<halfline> maybe it was broken before but no one knew to blame it on gnome-keyring
<cosimoc> fair enough :)
<halfline> hmm gtk dependency has been gone since 3.8
<halfline> i wonder when it got dropped from gcr
* halfline checks
<halfline> heh it's funny
<halfline> you made the exact same mistake i made on your first iteration of the patch
<halfline> (closing stdio too early)
<halfline> we're like peas in a pod
<cosimoc> hehe
<cosimoc> halfline, I think your patches looks good - perhaps you can push them in a branch and we can iterate there on them?
<halfline> sure
<halfline> i'll push to wip/fork-fixes
<cosimoc> thanks
<halfline> hmm gcr still has a gtk dependency
<halfline> so i wonder if at some point in the recent past gcr initialization moved around
<halfline> if something called gtk_init then the sleep would be fine
<cosimoc> halfline, could it have something to do with the gpg agent removal?
<cosimoc> not really sure tho
<halfline> dunno
<halfline> i'll just dupe my bug to your bug
<halfline> sound good ?
<cosimoc> halfline, yeah sounds good

*** This bug has been marked as a duplicate of bug 756059 ***