GNOME Bugzilla – Bug 756708
fix deadlocks/stalls when using autologin
Last modified: 2015-10-16 17:57:26 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
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.
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.
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.
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.
<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 ***