sd-device: make sd_device_new_from_subsystem_sysname() stricter

As workarounded by fc0cbed2db860d163d59d04c32fa6ec30bd0606f, the pair of
subsystem and sysname is not unique. For examples,
- /sys/bus/gpio and /sys/class/gpio, both have gpiochip%N. However, these point to different devpaths.
- /sys/bus/mdio_bus and /sys/class/mdio_bus,
- /sys/bus/mei and /sys/class/mei,
- /sys/bus/typec and /sys/class/typec, and so on.

Let's refuse to provide sd_device object in such cases.

diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c
index 383445edc8f8829ae558297743960196ab3c8409..01fa90b1ff5cf23a82e94b3dd03d2d616b34736c 100644 (file)
--- a/src/libsystemd/sd-device/sd-device.c
+++ b/src/libsystemd/sd-device/sd-device.c
@@ -386,25 +386,80 @@ _public_ int sd_device_new_from_ifindex(sd_device **ret, int ifindex) {
         return 0;
 }
 
-static int device_strjoin_new(
+static int device_new_from_path_join(
+                sd_device **device,
+                const char *subsystem,
+                const char *driver_subsystem,
+                const char *sysname,
                 const char *a,
                 const char *b,
                 const char *c,
-                const char *d,
-                sd_device **ret) {
+                const char *d) {
 
-        const char *p;
+        _cleanup_(sd_device_unrefp) sd_device *new_device = NULL;
+        _cleanup_free_ char *p = NULL;
         int r;
 
-        p = strjoina(a, b, c, d);
-        if (access(p, F_OK) < 0)
-                return IN_SET(errno, ENOENT, ENAMETOOLONG) ? 0 : -errno; /* If this sysfs is too long then it doesn't exist either */
+        assert(device);
+        assert(subsystem);
+        assert(sysname);
+
+        p = path_join(a, b, c, d);
+        if (!p)
+                return -ENOMEM;
+
+        r = sd_device_new_from_syspath(&new_device, p);
+        if (r == -ENODEV)
+                return 0;
+        if (r < 0)
+                return r;
+
+        /* Check if the found device really has the expected subsystem and sysname, for safety. */
+        if (!device_in_subsystem(new_device, subsystem))
+                return 0;
+
+        const char *new_driver_subsystem = NULL;
+        (void) sd_device_get_driver_subsystem(new_device, &new_driver_subsystem);
 
-        r = sd_device_new_from_syspath(ret, p);
+        if (!streq_ptr(driver_subsystem, new_driver_subsystem))
+                return 0;
+
+        const char *new_sysname;
+        r = sd_device_get_sysname(new_device, &new_sysname);
         if (r < 0)
                 return r;
 
-        return 1;
+        if (!streq(sysname, new_sysname))
+                return 0;
+
+        /* If this is the first device we found, then take it. */
+        if (!*device) {
+                *device = TAKE_PTR(new_device);
+                return 1;
+        }
+
+        /* Unfortunately, (subsystem, sysname) pair is not unique. For examples,
+         *   - /sys/bus/gpio and /sys/class/gpio, both have gpiochip%N. However, these point to different devpaths.
+         *   - /sys/bus/mdio_bus and /sys/class/mdio_bus,
+         *   - /sys/bus/mei and /sys/class/mei,
+         *   - /sys/bus/typec and /sys/class/typec, and so on.
+         * Hence, if we already know a device, then we need to check if it is equivalent to the newly found one. */
+
+        const char *devpath, *new_devpath;
+        r = sd_device_get_devpath(*device, &devpath);
+        if (r < 0)
+                return r;
+
+        r = sd_device_get_devpath(new_device, &new_devpath);
+        if (r < 0)
+                return r;
+
+        if (!streq(devpath, new_devpath))
+                return log_debug_errno(SYNTHETIC_ERRNO(ETOOMANYREFS),
+                                       "sd-device: found multiple devices for subsystem=%s and sysname=%s, refusing: %s, %s",
+                                       subsystem, sysname, devpath, new_devpath);
+
+        return 1; /* Fortunately, they are consistent. */
 }
 
 _public_ int sd_device_new_from_subsystem_sysname(
@@ -412,6 +467,7 @@ _public_ int sd_device_new_from_subsystem_sysname(
                 const char *subsystem,
                 const char *sysname) {
 
+        _cleanup_(sd_device_unrefp) sd_device *device = NULL;
         char *name;
         int r;
 
@@ -430,19 +486,15 @@ _public_ int sd_device_new_from_subsystem_sysname(
 
         if (streq(subsystem, "subsystem")) {
                 FOREACH_STRING(s, "/sys/bus/", "/sys/class/") {
-                        r = device_strjoin_new(s, name, NULL, NULL, ret);
+                        r = device_new_from_path_join(&device, subsystem, NULL, sysname, s, name, NULL, NULL);
                         if (r < 0)
                                 return r;
-                        if (r > 0)
-                                return 0;
                 }
 
         } else if (streq(subsystem, "module")) {
-                r = device_strjoin_new("/sys/module/", name, NULL, NULL, ret);
+                r = device_new_from_path_join(&device, subsystem, NULL, sysname, "/sys/module/", name, NULL, NULL);
                 if (r < 0)
                         return r;
-                if (r > 0)
-                        return 0;
 
         } else if (streq(subsystem, "drivers")) {
                 const char *sep;
@@ -454,35 +506,31 @@ _public_ int sd_device_new_from_subsystem_sysname(
                         sep++;
 
                         if (streq(sep, "drivers")) /* If the sysname is "drivers", then it's the drivers directory itself that is meant. */
-                                r = device_strjoin_new("/sys/bus/", subsys, "/drivers", NULL, ret);
+                                r = device_new_from_path_join(&device, subsystem, subsys, "drivers", "/sys/bus/", subsys, "/drivers", NULL);
                         else
-                                r = device_strjoin_new("/sys/bus/", subsys, "/drivers/", sep, ret);
+                                r = device_new_from_path_join(&device, subsystem, subsys, sep, "/sys/bus/", subsys, "/drivers/", sep);
                         if (r < 0)
                                 return r;
-                        if (r > 0)
-                                return 0;
                 }
         }
 
-        r = device_strjoin_new("/sys/bus/", subsystem, "/devices/", name, ret);
+        r = device_new_from_path_join(&device, subsystem, NULL, sysname, "/sys/bus/", subsystem, "/devices/", name);
         if (r < 0)
                 return r;
-        if (r > 0)
-                return 0;
 
-        r = device_strjoin_new("/sys/class/", subsystem, "/", name, ret);
+        r = device_new_from_path_join(&device, subsystem, NULL, sysname, "/sys/class/", subsystem, name, NULL);
         if (r < 0)
                 return r;
-        if (r > 0)
-                return 0;
 
-        r = device_strjoin_new("/sys/firmware/", subsystem, "/", name, ret);
+        r = device_new_from_path_join(&device, subsystem, NULL, sysname, "/sys/firmware/", subsystem, name, NULL);
         if (r < 0)
                 return r;
-        if (r > 0)
-                return 0;
 
-        return -ENODEV;
+        if (!device)
+                return -ENODEV;
+
+        *ret = TAKE_PTR(device);
+        return 0;
 }
 
 _public_ int sd_device_new_from_stat_rdev(sd_device **ret, const struct stat *st) {

diff --git a/src/libsystemd/sd-device/test-sd-device.c b/src/libsystemd/sd-device/test-sd-device.c
index a048860dbe72c1d250b6e4ba07ccf21098df4494..620615b6bb79a9100d26469bf226cbe778687857 100644 (file)
--- a/src/libsystemd/sd-device/test-sd-device.c
+++ b/src/libsystemd/sd-device/test-sd-device.c
@@ -74,9 +74,7 @@ static void test_sd_device_one(sd_device *d) {
         r = sd_device_get_subsystem(d, &subsystem);
         if (r < 0)
                 assert_se(r == -ENOENT);
-        else if (!streq(subsystem, "gpio")) { /* Unfortunately, there exist /sys/class/gpio and /sys/bus/gpio.
-                                               * Hence, sd_device_new_from_subsystem_sysname() and
-                                               * sd_device_new_from_device_id() may not work as expected. */
+        else {
                 const char *name, *id;
 
                 if (streq(subsystem, "drivers")) {
@@ -85,10 +83,14 @@ static void test_sd_device_one(sd_device *d) {
                         name = strjoina(driver_subsystem, ":", sysname);
                 } else
                         name = sysname;
-                assert_se(sd_device_new_from_subsystem_sysname(&dev, subsystem, name) >= 0);
-                assert_se(sd_device_get_syspath(dev, &val) >= 0);
-                assert_se(streq(syspath, val));
-                dev = sd_device_unref(dev);
+
+                r = sd_device_new_from_subsystem_sysname(&dev, subsystem, name);
+                if (r >= 0) {
+                        assert_se(sd_device_get_syspath(dev, &val) >= 0);
+                        assert_se(streq(syspath, val));
+                        dev = sd_device_unref(dev);
+                } else
+                        ASSERT_ERROR(r, ETOOMANYREFS);
 
                 /* The device ID depends on subsystem. */
                 assert_se(sd_device_get_device_id(d, &id) >= 0);
@@ -97,12 +99,12 @@ static void test_sd_device_one(sd_device *d) {
                         log_device_warning_errno(d, r,
                                                  "Failed to create sd-device object from device ID \"%s\". "
                                                  "Maybe running on a non-host network namespace.", id);
-                else {
-                        assert_se(r >= 0);
+                else if (r >= 0) {
                         assert_se(sd_device_get_syspath(dev, &val) >= 0);
                         assert_se(streq(syspath, val));
                         dev = sd_device_unref(dev);
-                }
+                } else
+                        ASSERT_ERROR(r, ETOOMANYREFS);
 
                 /* These require udev database, and reading database requires device ID. */
                 r = sd_device_get_is_initialized(d);