logind: don't free live SessionDevice on duplicate TakeDevice

method_take_device() declares its local with
_cleanup_(session_device_freep), but the "device already taken" check
assigns a borrowed hashmap pointer to it and returns immediately:

    sd = hashmap_get(s->devices, &dev);
    if (sd)
            return sd_bus_error_set(error, BUS_ERROR_DEVICE_IS_TAKEN, ...);

On that return the cleanup attribute runs session_device_free() on the
still-live entry that s->devices continues to own and use.
A subsequent ReleaseDevice then fails with BUS_ERROR_DEVICE_NOT_TAKEN
and the caller is left holding a revoked fd.

Follow-up for 360179ea4621a65e9993782bfe28ac4168a7e254

diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c
index 2c8207f054e10dd50f819f84a96b2623462aae9d..c0fad444b5a8100f4e9341f67d6b488e60784639 100644 (file)
--- a/src/login/logind-session-dbus.c
+++ b/src/login/logind-session-dbus.c
@@ -583,8 +583,7 @@ static int method_take_device(sd_bus_message *message, void *userdata, sd_bus_er
                 return sd_bus_error_set(error, BUS_ERROR_NOT_IN_CONTROL, "You are not in control of this session");
 
         dev = makedev(major, minor);
-        sd = hashmap_get(s->devices, &dev);
-        if (sd)
+        if (hashmap_contains(s->devices, &dev))
                 /* We don't allow retrieving a device multiple times.
                  * The related ReleaseDevice call is not ref-counted.
                  * The caller should use dup() if it requires more

diff --git a/src/login/test-session-properties.c b/src/login/test-session-properties.c
index acb2d8022958ec8a02025eb92930ba9fa93d9611..ee872c93363c5cc31d1c5b331b39998b985a7dd7 100644 (file)
--- a/src/login/test-session-properties.c
+++ b/src/login/test-session-properties.c
@@ -9,11 +9,13 @@
 #include <fcntl.h>
 
 #include "sd-bus.h"
+#include "sd-device.h"
 
 #include "alloc-util.h"
 #include "argv-util.h"
 #include "bus-common-errors.h"
 #include "bus-locator.h"
+#include "device-util.h"
 #include "tests.h"
 #include "time-util.h"
 
@@ -173,6 +175,69 @@ TEST(set_idle_hint) {
         assert_se(!idle_hint);
 }
 
+/* Takes the first device matching subsystem/sysname that this session is allowed to take,
+ * returning its devnum in *ret. */
+static bool take_first_available_device(sd_bus *bus, const char *subsystem, const char *sysname, dev_t *ret) {
+        _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL;
+
+        assert(bus);
+        assert(subsystem);
+        assert(sysname);
+        assert(ret);
+
+        assert_se(sd_device_enumerator_new(&e) >= 0);
+        assert_se(sd_device_enumerator_add_match_subsystem(e, subsystem, /* match= */ true) >= 0);
+        assert_se(sd_device_enumerator_add_match_sysname(e, sysname) >= 0);
+
+        FOREACH_DEVICE(e, d) {
+                dev_t devnum;
+
+                if (sd_device_get_devnum(d, &devnum) < 0)
+                        continue;
+
+                /* Take it once; skip devices that aren't takeable for this session. */
+                if (bus_call_method(bus, &session, "TakeDevice", NULL, NULL, "uu",
+                                    (uint32_t) major(devnum), (uint32_t) minor(devnum)) < 0)
+                        continue;
+
+                *ret = devnum;
+                return true;
+        }
+
+        return false;
+}
+
+/* Tests org.freedesktop.logind.Session TakeDevice/ReleaseDevice: a duplicate TakeDevice for a
+ * device the caller already holds must be rejected without tearing down the live device. */
+TEST(take_device) {
+        _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL;
+        _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
+        dev_t found_dev = 0;
+
+        assert_se(sd_bus_open_system(&bus) >= 0);
+        assert_se(bus_call_method(bus, &session, "TakeControl", NULL, NULL, "b", true) >= 0);
+
+        /* Take a device on the session's seat. Input event devices (e.g. the ACPI power button and
+         * the keyboard) and DRM cards belong to the seat and can be taken by its controller. */
+        if (!take_first_available_device(bus, "input", "event*", &found_dev) &&
+            !take_first_available_device(bus, "drm", "card*", &found_dev)) {
+                log_notice("No takeable seat device found, skipping %s.", __func__);
+                assert_se(bus_call_method(bus, &session, "ReleaseControl", NULL, NULL, NULL) >= 0);
+                return;
+        }
+
+        /* A duplicate TakeDevice must be rejected with 'device already taken' ... */
+        assert_se(bus_call_method(bus, &session, "TakeDevice", &error, NULL, "uu",
+                                  (uint32_t) major(found_dev), (uint32_t) minor(found_dev)) < 0);
+        assert_se(sd_bus_error_has_name(&error, BUS_ERROR_DEVICE_IS_TAKEN));
+
+        /* ... and must NOT tear down the original entry. */
+        assert_se(bus_call_method(bus, &session, "ReleaseDevice", NULL, NULL, "uu",
+                                  (uint32_t) major(found_dev), (uint32_t) minor(found_dev)) >= 0);
+
+        assert_se(bus_call_method(bus, &session, "ReleaseControl", NULL, NULL, NULL) >= 0);
+}
+
 static int intro(void) {
         if (saved_argc <= 1)
                 return EXIT_FAILURE;