test: fix fd_is_mount_point() check

So the currentl and only fd_is_mount_point() check is actually entirely
bogus: it passes "/" as filename argument, but that's not actually a
a valid filename, but an absolute path.

fd_is_mount_point() is written in a way tha the fd refers to a directory
and the specified path is a file directly below it that shall be
checked. The test call actually violated that rule, but still expected
success.

Let's fix this, and check for this explicitly, and refuse it.

Let's extend the test and move it to test-mountpoint-util.c where the
rest of the tests for related calls are placed.

Replaces: #18004
Fixes: #17950

diff --git a/src/basic/mountpoint-util.c b/src/basic/mountpoint-util.c
index 8bed96069f74a9c4187e9a7e9f4805c35a901725..a6602ad53945f0a7ec517cac2d9be221389ddb3d 100644 (file)
--- a/src/basic/mountpoint-util.c
+++ b/src/basic/mountpoint-util.c
@@ -132,6 +132,29 @@ static int fd_fdinfo_mnt_id(int fd, const char *filename, int flags, int *ret_mn
         return safe_atoi(p, ret_mnt_id);
 }
 
+static bool filename_possibly_with_slash_suffix(const char *s) {
+        const char *slash, *copied;
+
+        /* Checks whether the specified string is either file name, or a filename with a suffix of
+         * slashes. But nothing else.
+         *
+         * this is OK: foo, bar, foo/, bar/, foo//, bar///
+         * this is not OK: "", "/", "/foo", "foo/bar", ".", ".." … */
+
+        slash = strchr(s, '/');
+        if (!slash)
+                return filename_is_valid(s);
+
+        if (slash - s > FILENAME_MAX) /* We want to allocate on the stack below, hence do a size check first */
+                return false;
+
+        if (slash[strspn(slash, "/")] != 0) /* Check that the suffix consist only of one or more slashes */
+                return false;
+
+        copied = strndupa(s, slash - s);
+        return filename_is_valid(copied);
+}
+
 int fd_is_mount_point(int fd, const char *filename, int flags) {
         _cleanup_free_ struct file_handle *h = NULL, *h_parent = NULL;
         int mount_id = -1, mount_id_parent = -1;
@@ -144,6 +167,11 @@ int fd_is_mount_point(int fd, const char *filename, int flags) {
         assert(filename);
         assert((flags & ~(AT_SYMLINK_FOLLOW|AT_EMPTY_PATH)) == 0);
 
+        /* Insist that the specified filename is actually a filename, and not a path, i.e. some inode further
+         * up or down the tree then immediately below the specified directory fd. */
+        if (!filename_possibly_with_slash_suffix(filename))
+                return -EINVAL;
+
         /* First we will try statx()' STATX_ATTR_MOUNT_ROOT attribute, which is our ideal API, available
          * since kernel 5.8.
          *

diff --git a/src/test/test-mountpoint-util.c b/src/test/test-mountpoint-util.c
index 287488b7c133e6682e9aed0ad9ce7403c7d7a815..47fde5cb2c6e45ded89f1e9a70f2796941819d23 100644 (file)
--- a/src/test/test-mountpoint-util.c
+++ b/src/test/test-mountpoint-util.c
@@ -256,6 +256,37 @@ static void test_path_is_mount_point(void) {
         assert_se(rm_rf(tmp_dir, REMOVE_ROOT|REMOVE_PHYSICAL) == 0);
 }
 
+static void test_fd_is_mount_point(void) {
+        _cleanup_close_ int fd = -1;
+
+        log_info("/* %s */", __func__);
+
+        fd = open("/", O_RDONLY|O_CLOEXEC|O_DIRECTORY|O_NOCTTY);
+        assert_se(fd >= 0);
+
+        /* Not allowed, since "/" is a path, not a plain filename */
+        assert_se(fd_is_mount_point(fd, "/", 0) == -EINVAL);
+        assert_se(fd_is_mount_point(fd, ".", 0) == -EINVAL);
+        assert_se(fd_is_mount_point(fd, "./", 0) == -EINVAL);
+        assert_se(fd_is_mount_point(fd, "..", 0) == -EINVAL);
+        assert_se(fd_is_mount_point(fd, "../", 0) == -EINVAL);
+        assert_se(fd_is_mount_point(fd, "", 0) == -EINVAL);
+        assert_se(fd_is_mount_point(fd, "/proc", 0) == -EINVAL);
+        assert_se(fd_is_mount_point(fd, "/proc/", 0) == -EINVAL);
+        assert_se(fd_is_mount_point(fd, "proc/sys", 0) == -EINVAL);
+        assert_se(fd_is_mount_point(fd, "proc/sys/", 0) == -EINVAL);
+
+        /* This one definitely is a mount point */
+        assert_se(fd_is_mount_point(fd, "proc", 0) > 0);
+        assert_se(fd_is_mount_point(fd, "proc/", 0) > 0);
+
+        /* /root's entire raison d'etre is to be on the root file system (i.e. not in /home/ which might be
+         * split off), so that the user can always log in, so it cannot be a mount point unless the system is
+         * borked. Let's allow for it to be missing though. */
+        assert_se(IN_SET(fd_is_mount_point(fd, "root", 0), -ENOENT, 0));
+        assert_se(IN_SET(fd_is_mount_point(fd, "root/", 0), -ENOENT, 0));
+}
+
 int main(int argc, char *argv[]) {
         test_setup_logging(LOG_DEBUG);
 
@@ -279,6 +310,7 @@ int main(int argc, char *argv[]) {
 
         test_mnt_id();
         test_path_is_mount_point();
+        test_fd_is_mount_point();
 
         return 0;
 }

diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c
index 874bab8f9405c6c21c882c91528711cfbd9f040f..fcaffa4539107d4cb623f2735a1d307780c08b8c 100644 (file)
--- a/src/test/test-path-util.c
+++ b/src/test/test-path-util.c
@@ -7,7 +7,6 @@
 #include "exec-util.h"
 #include "fd-util.h"
 #include "macro.h"
-#include "mountpoint-util.h"
 #include "path-util.h"
 #include "process-util.h"
 #include "rm-rf.h"
@@ -42,8 +41,6 @@ static void test_path_simplify(const char *in, const char *out, const char *out_
 }
 
 static void test_path(void) {
-        _cleanup_close_ int fd = -1;
-
         log_info("/* %s */", __func__);
 
         test_path_compare("/goo", "/goo", 0);
@@ -82,10 +79,6 @@ static void test_path(void) {
         assert_se(streq(basename("/aa///file..."), "file..."));
         assert_se(streq(basename("file.../"), ""));
 
-        fd = open("/", O_RDONLY|O_CLOEXEC|O_DIRECTORY|O_NOCTTY);
-        assert_se(fd >= 0);
-        assert_se(fd_is_mount_point(fd, "/", 0) > 0);
-
         test_path_simplify("aaa/bbb////ccc", "aaa/bbb/ccc", "aaa/bbb/ccc");
         test_path_simplify("//aaa/.////ccc", "/aaa/./ccc", "/aaa/ccc");
         test_path_simplify("///", "/", "/");