LightDM - May 2013 - Race condition in lightdm greeter setup

I came across a race condition in lightdm greeter setup phase before the login
screen is displayed (at boot time or after logout).

I reported this also on Launchpad with more details
(https://bugs.launchpad.net/lightdm/+bug/1172752), but to work on a proper fix,
ideas on how to fix this would be welcome.

During greeter setup "lightdm --session-child" is spawned twice. The
first call to session_start() that does fork+execlp() is done from
greeter_start() and right after that handle_login() calls session_stop() +
session_start(). session_stop() sends SIGTERM to the child process, but if the
child has not managed to call execlp() yet, it still has signal handler set for
SIGTERM, because fork copies the parent's signal handlers to the child. Now
when session_stop() sends SIGTERM to the child, it uses the signal handler set
by the parent which causes the signal go to signal_cb() that then signals the
main lightdm process to die.

I managed to get rid of the problem by switching fork() -> vfork() in
session_start() which blocks the parent before execlp() is run and signal
handlers are cleared. This ensures that the signal sent from session_stop()
always ends up only to the child. Using vfork() is probably not the best
solution, though, so I wonder if others have better ideas on how to fix this?


--- src/session.c.orig	2012-08-29 21:25:16.000000000 +0000
+++ src/session.c	2013-04-25 15:15:13.353450704 +0000
@@ -360,7 +360,7 @@
     session->priv->username = g_strdup (username);
 
     /* Run the child */
-    session->priv->pid = fork ();
+    session->priv->pid = vfork ();
     if (session->priv->pid < 0)
     {
         g_debug ("Failed to fork session child process: %s", strerror
(errno));


The signal handling in lightdm looks quite tricky, so I'm not really sure if
this is the best way to handle this. Maybe it would be possible to get rid of
the first "lightdm --session-child" spawning to make all this
unnecessary? I could try to work on fixing this if someone who knows the code
better has an opinion on this.

Happy hacking!

Veli-Matti

Hi Veli-Matti,

Thanks for finding this problem.

There's more information in the bug report (
https://bugs.launchpad.net/lightdm/+bug/1172752) but to summarise for
others reading the list:
- It is correct that two sessions are spawned - one is the greeter and one
is the user session being authenticated
- There is a bug here though, the signal handler and pipe is open before
the child processes are execed, which could cause the daemon to handle a
SIGTERM from the child process and exit

--Robert


On 4 May 2013 01:56, Veli-Matti Lintu <veli-matti.lintu at opinsys.fi>
wrote:
>
> I came across a race condition in lightdm greeter setup phase before the
> login screen is displayed (at boot time or after logout).
>
> I reported this also on Launchpad with more details (
> https://bugs.launchpad.net/lightdm/+bug/1172752), but to work on a proper
> fix, ideas on how to fix this would be welcome.
>
> During greeter setup "lightdm --session-child" is spawned twice.
The first
> call to session_start() that does fork+execlp() is done from
> greeter_start() and right after that handle_login() calls session_stop() +
> session_start(). session_stop() sends SIGTERM to the child process, but if
> the child has not managed to call execlp() yet, it still has signal handler
> set for SIGTERM, because fork copies the parent's signal handlers to
the
> child. Now when session_stop() sends SIGTERM to the child, it uses the
> signal handler set by the parent which causes the signal go to signal_cb()
> that then signals the main lightdm process to die.
>
> I managed to get rid of the problem by switching fork() -> vfork() in
> session_start() which blocks the parent before execlp() is run and signal
> handlers are cleared. This ensures that the signal sent from session_stop()
> always ends up only to the child. Using vfork() is probably not the best
> solution, though, so I wonder if others have better ideas on how to fix
> this?
>
>
> --- src/session.c.orig  2012-08-29 21:25:16.000000000 +0000
> +++ src/session.c       2013-04-25 15:15:13.353450704 +0000
> @@ -360,7 +360,7 @@
>      session->priv->username = g_strdup (username);
>
>      /* Run the child */
> -    session->priv->pid = fork ();
> +    session->priv->pid = vfork ();
>      if (session->priv->pid < 0)
>      {
>          g_debug ("Failed to fork session child process: %s",
strerror
> (errno));
>
>
> The signal handling in lightdm looks quite tricky, so I'm not really
sure
> if this is the best way to handle this. Maybe it would be possible to get
> rid of the first "lightdm --session-child" spawning to make all
this
> unnecessary? I could try to work on fixing this if someone who knows the
> code better has an opinion on this.
>
> Happy hacking!
>
> Veli-Matti
> _______________________________________________
> LightDM mailing list
> LightDM at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/lightdm
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.freedesktop.org/archives/lightdm/attachments/20130516/12bcedff/attachment.html>