fd-util: cover some corner cases with fd_reopen() on symlinks

The /proc/self/fd/ interface cannot be used to follow symlinks pinned
via O_PATH. Add a comment + test for that. Moreover, using fd_reopen()
with O_NOFOLLOW cannot work. Add an explicit check and test for that, to
make behaviour uniform.

diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c
index 5eb43e1f816cb09554c87237e089f3242b3dfd10..b968bf948c4644c0d1da600123425dca01944eee 100644 (file)
--- a/src/basic/fd-util.c
+++ b/src/basic/fd-util.c
@@ -753,7 +753,20 @@ int fd_reopen(int fd, int flags) {
          *
          * This implicitly resets the file read index to 0.
          *
-         * If AT_FDCWD is specified as file descriptor gets an fd to the current cwd */
+         * If AT_FDCWD is specified as file descriptor gets an fd to the current cwd.
+         *
+         * If the specified file descriptor refers to a symlink via O_PATH, then this function cannot be used
+         * to follow that symlink. Because we cannot have non-O_PATH fds to symlinks reopening it without
+         * O_PATH will always result in -ELOOP. Or in other words: if you have an O_PATH fd to a symlink you
+         * can reopen it only if you pass O_PATH again. */
+
+        if (FLAGS_SET(flags, O_NOFOLLOW))
+                /* O_NOFOLLOW is not allowed in fd_reopen(), because after all this is primarily implemented
+                 * via a symlink-based interface in /proc/self/fd. Let's refuse this here early. Note that
+                 * the kernel would generate ELOOP here too, hence this manual check is mostly redundant –
+                 * the only reason we add it here is so that the O_DIRECTORY special case (see below) behaves
+                 * the same way as the non-O_DIRECTORY case. */
+                return -ELOOP;
 
         if (FLAGS_SET(flags, O_DIRECTORY) || fd == AT_FDCWD) {
                 /* If we shall reopen the fd as directory we can just go via "." and thus bypass the whole

diff --git a/src/test/test-fd-util.c b/src/test/test-fd-util.c
index 148694e0295454ee740939e5cec329cf7bbe6fad..fb9d4a33cb925f6cf689e69e480dd2dbe96079cf 100644 (file)
--- a/src/test/test-fd-util.c
+++ b/src/test/test-fd-util.c
@@ -22,6 +22,7 @@
 #include "rm-rf.h"
 #include "seccomp-util.h"
 #include "serialize.h"
+#include "stat-util.h"
 #include "string-util.h"
 #include "tests.h"
 #include "tmpfile-util.h"
@@ -410,6 +411,10 @@ TEST(fd_reopen) {
         assert_se(FLAGS_SET(fl, O_DIRECTORY));
         assert_se(FLAGS_SET(fl, O_PATH));
 
+        /* fd_reopen() with O_NOFOLLOW will systematically fail, since it is implemented via a symlink in /proc/self/fd/ */
+        assert_se(fd_reopen(fd1, O_RDONLY|O_CLOEXEC|O_NOFOLLOW) == -ELOOP);
+        assert_se(fd_reopen(fd1, O_RDONLY|O_CLOEXEC|O_DIRECTORY|O_NOFOLLOW) == -ELOOP);
+
         fd2 = fd_reopen(fd1, O_RDONLY|O_DIRECTORY|O_CLOEXEC);  /* drop the O_PATH */
         assert_se(fd2 >= 0);
 
@@ -486,6 +491,23 @@ TEST(fd_reopen) {
         safe_close(fd1);
         assert_se(fd_reopen(fd1, O_RDONLY|O_CLOEXEC) == -EBADF);
         fd1 = -EBADF;
+
+        /* Validate what happens if we reopen a symlink */
+        fd1 = open("/proc/self", O_PATH|O_CLOEXEC|O_NOFOLLOW);
+        assert_se(fd1 >= 0);
+        assert_se(fstat(fd1, &st1) >= 0);
+        assert_se(S_ISLNK(st1.st_mode));
+
+        fd2 = fd_reopen(fd1, O_PATH|O_CLOEXEC);
+        assert_se(fd2 >= 0);
+        assert_se(fstat(fd2, &st2) >= 0);
+        assert_se(S_ISLNK(st2.st_mode));
+        assert_se(stat_inode_same(&st1, &st2));
+        fd2 = safe_close(fd2);
+
+        /* So here's the thing: if we have an O_PATH fd to a symlink, we *cannot* convert it to a regular fd
+         * with that. i.e. you cannot have the VFS follow a symlink pinned via an O_PATH fd. */
+        assert_se(fd_reopen(fd1, O_RDONLY|O_CLOEXEC) == -ELOOP);
 }
 
 TEST(fd_reopen_condition) {