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 781825 - gdm3: autologin race makes wayland session impossible
gdm3: autologin race makes wayland session impossible
Status: RESOLVED FIXED
Product: gdm
Classification: Core
Component: general
3.22.x
Other Linux
: Normal normal
: ---
Assigned To: GDM maintainers
GDM maintainers
Depends on:
Blocks:
 
 
Reported: 2017-04-27 11:19 UTC by David Härdeman
Modified: 2017-05-04 20:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
session: update session type when session is read (6.70 KB, patch)
2017-04-27 14:26 UTC, Ray Strode [halfline]
none Details | Review
session: update session type when session is read (7.09 KB, patch)
2017-04-28 14:42 UTC, Ray Strode [halfline]
committed Details | Review

Description David Härdeman 2017-04-27 11:19:32 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.
Comment 1 Ray Strode [halfline] 2017-04-27 14:25:26 UTC
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?
Comment 2 Ray Strode [halfline] 2017-04-27 14:26:20 UTC
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.
Comment 3 Ray Strode [halfline] 2017-04-27 14:28:06 UTC
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.
Comment 4 David Härdeman 2017-04-27 19:30:15 UTC
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?
Comment 5 Ray Strode [halfline] 2017-04-28 14:41:46 UTC
you're right, of course.  Let me post an update that moves the call down a couple of lines.
Comment 6 Ray Strode [halfline] 2017-04-28 14:42:35 UTC
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.
Comment 7 David Härdeman 2017-04-28 17:14:04 UTC
I've tried the second version of the patch and it works for me (tm).
Comment 8 Ray Strode [halfline] 2017-05-04 20:10:15 UTC
Attachment 350650 [details] pushed as d7eda0d - session: update session type when session is read