Bug 90769 - user_elect_display() overwrites DISPLAY with TTY session when switching VT away from a graphical session
Summary: user_elect_display() overwrites DISPLAY with TTY session when switching VT aw...
Status: RESOLVED FIXED
Alias: None
Product: systemd
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: All All
: medium major
Assignee: systemd-bugs
QA Contact: systemd-bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-29 10:05 UTC by Philip Withnall
Modified: 2015-06-05 12:08 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:

Attachments
logind: Fix user_elect_display() to be more stable (5.61 KB, patch)
2015-05-29 10:07 UTC, Philip Withnall 		Details | Splinter Review
View All

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.