Race condition in lightdm greeter setup

Tested lightdm versions: 1.4.0-0ubuntu2 and 1.6.0-0ubuntu2 on Ubuntu 12.10

I come across a bug in lightdm that causes lightdm to die randomly when starting or after logout. It happens only on some machines and on some machines it never happens. My best guess here is a race condition.

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 of 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.

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?

Relevant pieces of code:

The fork() is called here and the calling process has signal handlers set.

-----------> src/session.c <--------------
gboolean
session_start (Session *session, const gchar *service, const gchar *username, gboolean do_authenticate, gboolean is_interactive, gboolean is_guest)
{
...
    /* Run the child */
    session->priv->pid = fork ();
    if (session->priv->pid < 0)
    {
        g_debug ("Failed to fork session child process: %s", strerror (errno));
        return FALSE;
    }

    if (session->priv->pid == 0)
    {
        /* Run us again in session child mode */
        execlp ("lightdm",
                "lightdm",
                "--session-child",
                g_strdup_printf ("%d", to_child_output),
                g_strdup_printf ("%d", from_child_input),
                NULL);
        _exit (EXIT_FAILURE);
    }
...
}

session_stop (Session *session)
{
    g_return_if_fail (session != NULL);

    if (session->priv->pid > 0)
    {
        g_debug ("Session %d: Sending SIGTERM", session->priv->pid);
        kill (session->priv->pid, SIGTERM);
        // FIXME: Handle timeout
    }
}
-----------> src/session.c <--------------

session_start() is called first time from greeter_start()

-----------> src/greeter.c <--------------
gboolean
greeter_start (Greeter *greeter, const gchar *service, const gchar *username)
{
...
    result = session_start (greeter->priv->session, service, username, FALSE, FALSE, FALSE);
...
}
-----------> src/greeter.c <--------------

Almost immediately after session_start() is called, reset_session() is callend which then calls session_stop(). After that session_start() is called again which spawns a new "lightdm --session-child" process.

-----------> src/greeter.c <--------------
static void
handle_login (Greeter *greeter, guint32 sequence_number, const gchar *username)
{
...
    reset_session (greeter);
...
    /* Run the session process */
    session_start (greeter->priv->authentication_session, service, username, TRUE, is_interactive, FALSE);
...
}
-----------> src/greeter.c <--------------

Because sometimes fork+execlp() has not finished before SIGTERM is sent, the SIGTERM ends up here instead of the callback in session-child.c:

-----------> src/process.c <--------------
signal_cb (int signum, siginfo_t *info, void *data)
{
    /* NOTE: Using g_printerr as can't call g_warning from a signal callback */
    if (write (signal_pipe[1], &info->si_signo, sizeof (int)) < 0 ||
        write (signal_pipe[1], &info->si_pid, sizeof (pid_t)) < 0)
        g_printerr ("Failed to write to signal pipe: %s", strerror (errno));
}

static gboolean
handle_signal (GIOChannel *source, GIOCondition condition, gpointer data)
{
    int signo;
    pid_t pid;
    Process *process;

    if (read (signal_pipe[0], &signo, sizeof (int)) < 0 ||
        read (signal_pipe[0], &pid, sizeof (pid_t)) < 0)
    {
        g_warning ("Error reading from signal pipe: %s", strerror (errno));
        return TRUE;
    }

    g_debug ("Got signal %d from process %d", signo, pid);

    process = g_hash_table_lookup (processes, GINT_TO_POINTER (pid));
    if (process == NULL)
        process = process_get_current ();
    if (process)
        g_signal_emit (process, signals[GOT_SIGNAL], 0, signo);

    return TRUE;
}
-----------> src/process.c <--------------