Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logind: Fix user_elect_display() to be more stable #56

Closed

Conversation

pwithnall
Copy link
Contributor

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

@zonque zonque added the login label Jun 3, 2015
@dvdhrm
Copy link
Contributor

dvdhrm commented Jun 4, 2015

Patch looks mostly good. Please fix the issues I mentioned and it should be good to go.
Thanks!

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
@pwithnall pwithnall force-pushed the wip/pwithnall/elect-display branch from d6721c5 to c00ee6f Compare June 4, 2015 15:18
@riking
Copy link

riking commented Jun 5, 2015

LGTM.

@dvdhrm
Copy link
Contributor

dvdhrm commented Jun 5, 2015

Applied, thanks.

@dvdhrm dvdhrm closed this Jun 5, 2015
@pwithnall pwithnall deleted the wip/pwithnall/elect-display branch June 5, 2015 12:07
@poettering
Copy link
Member

Guys, the line breaks are all fucked up on this one, please read up on CODING_STYLE on this!

@poettering
Copy link
Member

Fixed the coding style issues now in cde40ac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants