logind: drop the seat from the GC queue after draining its devices

manager_gc() pops a seat off m->seat_gc_queue and clears
seat->in_gc_queue before calling seat_free(). seat_free() then drains
the seat's devices, and device_detach() calls seat_add_to_gc_queue() for
a seat that just lost its last master device, re-prepending the dying
seat onto m->seat_gc_queue. seat_free() removed itself from the queue up
front, before that drain, so the re-queued seat is left on the queue as a
dangling pointer once mfree() runs.

Follow-up for 718d006a63f773c42106494e823250c48942cf08

diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c
index 1fe1eb41f45f32f58d82cb496c00fc99379d1aec..ab88eae8cc5b9ae4f9c76543fd6824d29f6c467b 100644 (file)
--- a/src/login/logind-seat.c
+++ b/src/login/logind-seat.c
@@ -73,9 +73,6 @@ Seat* seat_free(Seat *s) {
         if (!s)
                 return NULL;
 
-        if (s->in_gc_queue)
-                LIST_REMOVE(gc_queue, s->manager->seat_gc_queue, s);
-
         while (s->sessions)
                 session_free(s->sessions);
 
@@ -84,6 +81,13 @@ Seat* seat_free(Seat *s) {
         while (s->devices)
                 device_free(s->devices);
 
+        /* Draining sessions and devices above can put us back onto the GC queue: device_detach()
+         * re-queues the seat once it loses its last master device. Remove ourselves from the queue
+         * only here, after everything that could re-add us has run, otherwise we leave a dangling
+         * pointer behind for the next manager_gc() pass. */
+        if (s->in_gc_queue)
+                LIST_REMOVE(gc_queue, s->manager->seat_gc_queue, s);
+
         hashmap_remove(s->manager->seats, s->id);
 
         set_free(s->uevents);

diff --git a/src/login/test-login-tables.c b/src/login/test-login-tables.c
index 2c064eb9eb4ac4ab8322b2d0bd4c2631e66f347a..838d177c55cf80c272d50bcb2baab85a4615438d 100644 (file)
--- a/src/login/test-login-tables.c
+++ b/src/login/test-login-tables.c
@@ -8,6 +8,8 @@
 #include "logind.h"
 #include "logind-action.h"
 #include "logind-button.h"
+#include "logind-device.h"
+#include "logind-seat.h"
 #include "logind-session.h"
 #include "logind-user.h"
 #include "sleep-config.h"
@@ -69,6 +71,40 @@ static void test_button_free_cancels_long_press(void) {
         assert_se(!m->reboot_key_long_press_event_source);
 }
 
+/* Verify that seat_free() does not leave itself on the GC queue when draining a device re-queues it:
+ * device_detach() calls seat_add_to_gc_queue() once the seat loses its last master device, so a
+ * seat_free() that dropped itself from the queue before that drain would leave the freed seat dangling
+ * at the queue head for the next manager_gc() pass. */
+static void test_seat_free_dequeues_on_device_detach(void) {
+        _cleanup_(hashmap_freep) Hashmap *seats = NULL;
+        _cleanup_(hashmap_freep) Hashmap *devices = NULL;
+        _cleanup_free_ Manager *m = NULL;
+        Seat *s;
+        Device *d;
+
+        assert_se(m = new0(Manager, 1));
+        assert_se(seats = hashmap_new(&string_hash_ops));
+        assert_se(devices = hashmap_new(&string_hash_ops));
+        m->seats = seats;
+        m->devices = devices;
+
+        assert_se(seat_new(m, "seattest", &s) >= 0);
+
+        /* A non-master device: the seat has no master, so detaching it re-queues the seat for GC. */
+        assert_se(d = device_new(m, "/sys/devices/platform/test-seat-device", /* master= */ false));
+        device_attach(d, s);
+
+        /* Mimic manager_gc() having popped the seat off the queue right before freeing it. */
+        assert_se(!s->in_gc_queue);
+        assert_se(!m->seat_gc_queue);
+
+        seat_free(s);
+
+        /* Freeing the seat drained the device, which re-queued the (now freed) seat. seat_free() must
+         * have removed it again, otherwise the queue head is a dangling pointer. */
+        assert_se(!m->seat_gc_queue);
+}
+
 int main(int argc, char **argv) {
         test_setup_logging(LOG_DEBUG);
 
@@ -82,6 +118,7 @@ int main(int argc, char **argv) {
 
         test_sleep_handle_action();
         test_button_free_cancels_long_press();
+        test_seat_free_dequeues_on_device_detach();
 
         return EXIT_SUCCESS;
 }