Merge lp:~pitti/lightdm/cleanup-session into lp:lightdm

[Martin Pitt]

Fix bug 1637758 in a central/generic way instead of chasing every session (like unity-greeter) to clean up after itself.

This was not previously necessary with session D-Bus, as upstart killed that, and nm-applet/unity-settings-daemon went down with it. But now it's the other way around, nothing in e. g. unity-greeter stops nm-applet when the session closes, so that and the user bus and the whole lightdm logind session stay lingering.

[Martin Pitt (pitti)]

I uploaded this to zesty as debian/patch/, as too many people complain about the weird behaviour (duplicate settings-daemon etc.)

[Robert Ancell (robert-ancell)]

Oops, I overwrote you change with the 1.21 upload. Sorry. In the future please directly commit any packaging changes to lp:lightdm - these are more important to be in sync with what is in Ubuntu than blocking on a review (changes outside debian/ should still go through review).

[Robert Ancell (robert-ancell)]

I was under the impression that the logind PAM module would clean up the session but it appears that it only calls ReleaseSession which doesn't kill the processes like TerminateSession does. So if that's the case, then we do need to call TerminateSession at some point (for misbehaving greeter sessions).

However, we should do it after we send SIGTERM to the greeter - it should have a chance to shutdown cleanly. So I think the correct behaviour is to:

1. Send SIGTERM to the greeter
2. Wait for the greeter to stop (session_watch_cb)
3. Call TerminateSession to clean up anything the greeter missed.
4. Wait for the logind session to close (to avoid a race)

There should also be tests to confirm the TerminateSession call is made.

It's probably best to resurrect that distro patch and get that into zesty then fix it properly on this branch.

[Robert Ancell (robert-ancell)]

review: Needs Fixing

[Martin Pitt (pitti)]

lp:lightdm had a lot of commits and a newer upstream release than zesty, that's why I didn't just upload that directly.

> it appears that it only calls ReleaseSession which doesn't kill the processes like TerminateSession does.

Correct. logind *can* kill sessions on logout (KillUserProcesses=yes in logind.conf), but we don't do that by default as this would also kill e. g. "screen".

> we should do it after we send SIGTERM to the greeter - it should have a chance to shutdown cleanly.

We could use KillSession with SIGTERM first and wait -- but I don't see why we should bother. The "clean shutdown" part should already have happened from the greeter session itself (and e. g. unity-greeter actually does partially clean itself up), so this would just introduce another round of waiting and thus increase the time for login.

> 4. Wait for the logind session to close (to avoid a race)

I don't think that's necessary -- that can happen asynchronously in the background. The point is to make sure that things like settings-daemon disappear before starting the real user session; if the session itself is still "closing" (but without processes inside any more) for a few msecs that doesn't hurt at all.

[Robert Ancell (robert-ancell)]

> > 4. Wait for the logind session to close (to avoid a race)
>
> I don't think that's necessary -- that can happen asynchronously in the
> background. The point is to make sure that things like settings-daemon
> disappear before starting the real user session; if the session itself is
> still "closing" (but without processes inside any more) for a few msecs that
> doesn't hurt at all.

Can we assume all processes are gone by the time TerminateSession() is called?

[Robert Ancell (robert-ancell)]

"Perfect is the enemy of good". Works well enough so I've committed with a regression test.

[Martin Pitt (pitti)]

> Can we assume all processes are gone by the time TerminateSession() is called?

Not synchronously, it'll still take some time for the processes to react and shut down. But so far this seems to be "good enough" to fix the login issues.

But indeed this is a good point, this should be improved, like waiting for the session-XX.scope unit to actually disappear.

Thanks for adding a test!

=== modified file 'src/login1.c'
--- src/login1.c	2015-10-16 10:04:18 +0000
+++ src/login1.c	2016-11-14 11:05:19 +0000
@@ -437,6 +437,39 @@
     }
 }
 
+void
+login1_service_terminate_session (Login1Service *service, const gchar *session_id)
+{
+    GError *error = NULL;
+
+    g_return_if_fail (service != NULL);
+    g_return_if_fail (session_id != NULL);
+
+    g_debug ("Terminating login1 session %s", session_id);
+
+    if (session_id)
+    {
+        GVariant *result;
+
+        result = g_dbus_connection_call_sync (service->priv->connection,
+                                              LOGIN1_SERVICE_NAME,
+                                              LOGIN1_OBJECT_NAME,
+                                              LOGIN1_MANAGER_INTERFACE_NAME,
+                                              "TerminateSession",
+                                              g_variant_new ("(s)", session_id),
+                                              G_VARIANT_TYPE ("()"),
+                                              G_DBUS_CALL_FLAGS_NONE,
+                                              -1,
+                                              NULL,
+                                              &error);
+        if (error)
+            g_warning ("Error terminating login1 session: %s", error->message);
+        g_clear_error (&error);
+        if (result)
+            g_variant_unref (result);
+    }
+}
+
 static void
 login1_service_init (Login1Service *service)
 {

=== modified file 'src/login1.h'
--- src/login1.h	2014-12-16 22:48:40 +0000
+++ src/login1.h	2016-11-14 11:05:19 +0000
@@ -78,6 +78,8 @@
 
 void login1_service_activate_session (Login1Service *service, const gchar *session_id);
 
+void login1_service_terminate_session (Login1Service *service, const gchar *session_id);
+
 const gchar *login1_seat_get_id (Login1Seat *seat);
 
 gboolean login1_seat_get_can_graphical (Login1Seat *seat);

=== modified file 'src/session.c'
--- src/session.c	2016-07-11 21:51:14 +0000
+++ src/session.c	2016-11-14 11:05:19 +0000
@@ -913,6 +913,11 @@
 {
     g_return_if_fail (session != NULL);
 
+    /* Kill remaining processes in our logind session to avoid them leaking
+     * to the user session (they share the same $DISPLAY) */
+    if (getuid () == 0 && session->priv->login1_session_id)
+        login1_service_terminate_session (login1_service_get_instance (), session->priv->login1_session_id);
+
     /* If can cleanly stop then do that */
     if (session_get_is_authenticated (session) && !session->priv->command_run)
     {