GNOME Bugzilla – Bug 781825
gdm3: autologin race makes wayland session impossible
Last modified: 2017-05-04 20:10:19 UTC
This is from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=851754 In daemon/gdm-session.c, gdm_session_setup() and gdm_session_setup_for_user() both do: send_setup[_for_user](self, service_name); gdm_session_defaults_changed(self); send_setup[_for_user]() does asynchronous dbus calls and return immediately, meaning that gdm_session_defaults_changed() is called *before* the dbus call is finished and before the user's saved session has been set. gdm_session_defaults_changed() calls: --> update_session_type() --> gdm_session_is_wayland_session() --> get_session_filename() --> get_session_name() /* no session selected */ --> get_default_session_name() /* no saved_session */ --> get_fallback_session_name() The default session returned by get_fallback_session_name() is "default" on Debian systems (that's a Debian specific patch, upstream uses "gnome" as the default) which gets converted to "default.desktop" before returning to gdm_session_is_wayland_session(). That difference shouldn't matter. gdm_session_is_wayland_session() then calls load_key_file_for_file(self, "default.desktop"...) which looks for "default.desktop" in the directories returned by get_system_session_dirs() in the following order: "/etc/X11/sessions/", DMCONFDIR "/Sessions/", DATADIR "/gdm/BuiltInSessions/", DATADIR "/xsessions/", DATADIR "/wayland-sessions/" "default.desktop" is found in DATADIR "/gdm/BuiltInSessions/", i.e. /usr/share/gdm/BuiltInSessions/default.desktop and the full path is returned to gdm_session_is_wayland_session() which checks the path to determine if the session is a wayland session: if (full_path != NULL && strstr (full_path, "/wayland-sessions/") != NULL) { is_wayland_session = TRUE; } This means that gdm_session_is_wayland_session() will always return false for the default session. update_session_type() uses the return value (is_wayland_session) as follows: if (is_wayland_session) { set_session_type (self, "wayland"); } else { set_session_type (self, NULL); } gdm_session_start_session() is later called, after the real user session has been set but update_session_type() is *not* called when that happens. gdm_session_start_session() does: gboolean is_x11 = TRUE; ... #ifdef ENABLE_WAYLAND_SUPPORT is_x11 = g_strcmp0 (self->priv->session_type, "wayland") != 0; #endif if (display_mode == GDM_SESSION_DISPLAY_MODE_LOGIND_MANAGED || display_mode == GDM_SESSION_DISPLAY_MODE_NEW_VT) { run_launcher = TRUE; } ... command = get_session_command (self); ... if (run_launcher) { if (is_x11) { program = g_strdup_printf (LIBEXECDIR "/gdm-x-session %s %s\"%s\"", run_xsession_script? "--run-script " : "", allow_remote_connections? "--allow-remote-connections " : "", command); } else { program = g_strdup_printf (LIBEXECDIR "/gdm-wayland-session \"%s\"", command); } } Therefore, is_x11 will always be true here. I couldn't find the right point in time to call update_session_type() because of all the async dbus calls and callbacks, so I stopped here. I checked this on Debian unstable (i.e. gdm3 3.22.3) but I visually inspected the git tree of gdm3, and I'm pretty certain the bug is still there.
Hey thanks for digging into this... (In reply to David Härdeman from comment #0) > The default session returned by get_fallback_session_name() is "default" > on Debian systems (that's a Debian specific patch, upstream uses "gnome" > as the default) which gets converted to "default.desktop" before > returning to gdm_session_is_wayland_session(). That difference shouldn't > matter. Well, in theory it shouldn't matter, but in practice it does. The reason it matters is because there is a gnome.desktop in $datadir/wayland-sessions/ but there's not a default.desktop. So this is an upstream bug that only downstream will see. I'll attach a patch that I think will fix this bug. Can you give it a try?
Created attachment 350556 [details] [review] session: update session type when session is read We currently update the session type (to either wayland or x11) when we first start the PAM conversation and later when the username is set (if the user is not set at the same time as the PAM conversation). There's a race that means the session won't necessarly have been read from accountsservice at these points. This commit changes the code to instead update the session type in result to the session actually getting read.
Review of attachment 350556 [details] [review]: ::: daemon/gdm-session.c @@ +935,3 @@ + update_session_type (self); + This is the crux of the fix. @@ +2298,3 @@ g_return_if_fail (GDM_IS_SESSION (self)); + update_session_type (self); not 100% sure this is needed (or the next call). I need to investigate if I can get rid of them without ill effect.
Haven't tried it yet but at least the call to update_session_type() in worker_on_saved_session_name_read() seems too early? There's code later on in worker_on_saved_session_name_read() which updates self->priv->saved_session, which is used by the update_session_type() logic?
you're right, of course. Let me post an update that moves the call down a couple of lines.
Created attachment 350650 [details] [review] session: update session type when session is read We currently update the session type (to either wayland or x11) when we first start the PAM conversation and later when the username is set (if the user is not set at the same time as the PAM conversation). There's a race that means the session won't necessarly have been read from accountsservice at these points. This commit changes the code to instead update the session type in result to the session actually getting read.
I've tried the second version of the patch and it works for me (tm).
Attachment 350650 [details] pushed as d7eda0d - session: update session type when session is read