Bug 90769

Summary:	user_elect_display() overwrites DISPLAY with TTY session when switching VT away from a graphical session
Product:	systemd	Reporter:	Philip Withnall <bugzilla>
Component:	general	Assignee:	systemd-bugs
Status:	RESOLVED FIXED	QA Contact:	systemd-bugs
Severity:	major
Priority:	medium	CC:	bugzilla
Version:	unspecified
Hardware:	All
OS:	All
Whiteboard:
i915 platform:		i915 features:
Attachments:	logind: Fix user_elect_display() to be more stable

Description Philip Withnall 2015-05-29 10:05:49 UTC

Patch coming to fix logind’s user_elect_display() to be more stable when switching VT from a graphical session to a TTY session, while still keeping the graphical session open.

See the commit message for full details.

This is important because it affects the session returned by sd_uid_get_display(), which will be used by PolicyKit to find the correct session context to make policy decisions in. Hence this may (depending on the particular udisks2 policy in force) break auto-mounting of USB disks if the user starts a remote session after their X11 session, for example.

See also: bug #67728, bug #76358.

---

The incorrect behaviour can be tested by:
 1. In an X11 terminal, `cat /run/systemd/users/$uid` gives SESSIONS=c1 DISPLAY=c1.
 2. Switch VT, log in, `cat /run/systemd/users/$uid` gives SESSIONS="c2 c1" DISPLAY=c2. Here, DISPLAY is incorrect and will result in the wrong return value from sd_uid_get_display().
 3. Switch back to X11 and cat again; details are the same.
 4. Log out of the VT and cat again; DISPLAY=c1 SESSIONS=c1 again (correct).

Comment 1 Philip Withnall 2015-05-29 10:07:20 UTC

Created attachment 116145 [details] [review]
logind: Fix user_elect_display() to be more stable

The previous implementation of user_elect_display() could easily end up
overwriting the user’s valid graphical session with a new TTY session.
For example, consider the situation where there is one session:
   c1, type = SESSION_X11, !stopping, class = SESSION_USER
it is initially elected as the user’s display (i.e. u->display = c1).

If another session is started, on a different VT, the sessions_by_user
list becomes:
   c1, type = SESSION_X11, !stopping, class = SESSION_USER
   c2, type = SESSION_TTY, !stopping, class = SESSION_USER

In the previous code, graphical = c1 and text = c2, as expected.
However, neither graphical nor text fulfil the conditions for setting
u->display = graphical (because neither is better than u->display), so
the code falls through to check the text variable. The conditions for
this match, as u->display->type != SESSION_TTY (it’s actually
SESSION_X11). Hence u->display is set to c2, which is incorrect, because
session c1 is still valid.

Refactor user_elect_display() to use a more explicit filter and
pre-order comparison over the sessions. This can be demonstrated to be
stable and only ever ‘upgrade’ the session to a more graphical one.

Comment 2 Philip Withnall 2015-06-04 15:26:20 UTC

Under review here: https://github.com/systemd/systemd/pull/56.

Comment 3 Philip Withnall 2015-06-05 12:08:32 UTC

Reviewed and committed via github: https://github.com/systemd/systemd/pull/56#issuecomment-109257063

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.