From 75aad3b101548151905d528269ffd2a388955193 Mon Sep 17 00:00:00 2001

From: Philip Withnall <philip.withnall@collabora.co.uk>

Date: Fri, 29 May 2015 10:49:21 +0100

Subject: [PATCH] logind: Fix user_elect_display() to be more stable

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

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.

https://bugs.freedesktop.org/show_bug.cgi?id=90769

(cherry picked from commit 7ffeb45cc63e1326690fd9461b7a4719a3d4f85c)

---

 src/login/logind-user.c | 90 +++++++++++++++++++++++++++++--------------------

 1 file changed, 54 insertions(+), 36 deletions(-)

diff --git a/src/login/logind-user.c b/src/login/logind-user.c

index 71bff96728..2f62e34f63 100644

--- a/src/login/logind-user.c

+++ b/src/login/logind-user.c

@@ -738,54 +738,72 @@ int user_kill(User *u, int signo) {

         return manager_kill_unit(u->manager, u->slice, KILL_ALL, signo, NULL);

 }

+static bool

+elect_display_filter(Session *s) {

+        /* Return true if the session is a candidate for the user’s ‘primary

+         * session’ or ‘display’. */

+        assert(s);

+

+        return (s->class == SESSION_USER && !s->stopping);

+}

+

+static int

+elect_display_compare(Session *s1, Session *s2) {

+        /* Indexed by SessionType. Lower numbers mean more preferred. */

+        const int type_ranks[_SESSION_TYPE_MAX] = {

+                [SESSION_UNSPECIFIED] = 0,

+                [SESSION_TTY] = -2,

+                [SESSION_X11] = -3,

+                [SESSION_WAYLAND] = -3,

+                [SESSION_MIR] = -3,

+                [SESSION_WEB] = -1,

+        };

+

+        /* Calculate the partial order relationship between s1 and s2,

+         * returning < 0 if s1 is preferred as the user’s ‘primary session’,

+         * 0 if s1 and s2 are equally preferred or incomparable, or > 0 if s2

+         * is preferred.

+         *

+         * s1 or s2 may be NULL. */

+        if ((s1 == NULL) != (s2 == NULL))

+                return (s1 == NULL) - (s2 == NULL);

+

+        if (s1->stopping != s2->stopping)

+                return s1->stopping - s2->stopping;

+

+        if ((s1->class != SESSION_USER) != (s2->class != SESSION_USER))

+                return (s1->class != SESSION_USER) - (s2->class != SESSION_USER);

+

+        if ((s1->type == _SESSION_TYPE_INVALID) != (s2->type == _SESSION_TYPE_INVALID))

+                return (s1->type == _SESSION_TYPE_INVALID) - (s2->type == _SESSION_TYPE_INVALID);

+

+        if (s1->type != s2->type)

+                return type_ranks[s1->type] - type_ranks[s2->type];

+

+        return 0;

+}

+

 void user_elect_display(User *u) {

-        Session *graphical = NULL, *text = NULL, *other = NULL, *s;

+        Session *s;

         assert(u);

         /* This elects a primary session for each user, which we call

          * the "display". We try to keep the assignment stable, but we

          * "upgrade" to better choices. */

+        log_debug("Electing new display for user %s", u->name);

         LIST_FOREACH(sessions_by_user, s, u->sessions) {

-

-                if (s->class != SESSION_USER)

-                        continue;

-

-                if (s->stopping)

+                if (!elect_display_filter(s)) {

+                        log_debug("Ignoring session %s", s->id);

                         continue;

+                }

-                if (SESSION_TYPE_IS_GRAPHICAL(s->type))

-                        graphical = s;

-                else if (s->type == SESSION_TTY)

-                        text = s;

-                else

-                        other = s;

-        }

-

-        if (graphical &&

-            (!u->display ||

-             u->display->class != SESSION_USER ||

-             u->display->stopping ||

-             !SESSION_TYPE_IS_GRAPHICAL(u->display->type))) {

-                u->display = graphical;

-                return;

-        }

-

-        if (text &&

-            (!u->display ||

-             u->display->class != SESSION_USER ||

-             u->display->stopping ||

-             u->display->type != SESSION_TTY)) {

-                u->display = text;

-                return;

+                if (elect_display_compare(s, u->display) < 0) {

+                        log_debug("Choosing session %s in preference to %s", s->id, u->display ? u->display->id : "-");

+                        u->display = s;

+                }

         }

-

-        if (other &&

-            (!u->display ||

-             u->display->class != SESSION_USER ||

-             u->display->stopping))

-                u->display = other;

 }

 static const char* const user_state_table[_USER_STATE_MAX] = {