fd-util: use F_DUPFD_QUERY for same_fd()

Catch up with the nice little toys the kernel fs developers have added
for us. Preferably, let's make use of the new F_DUPFD_QUERY fcntl() call
that checks whether two fds are just duplicates of each other
(duplicates as in dup(), not as in open() of the same inode, i.e.
whether they share a single file offset and so on).

This API is much nicer, since it is a core kernel feature, unlike the
kcmp() call we so far used, which is part of the (optional)
checkpoint/restore stuff.

F_DUPFD_QUERY is available since kernel 6.10.

diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c
index 3f8c5b92d32e90359a86df14220b98e4e19df04f..c112f8dbad552f274c5d20e8a17f81a1506b132f 100644 (file)
--- a/src/basic/fd-util.c
+++ b/src/basic/fd-util.c
@@ -530,25 +530,57 @@ int same_fd(int a, int b) {
         assert(b >= 0);
 
         /* Compares two file descriptors. Note that semantics are quite different depending on whether we
-         * have kcmp() or we don't. If we have kcmp() this will only return true for dup()ed file
-         * descriptors, but not otherwise. If we don't have kcmp() this will also return true for two fds of
-         * the same file, created by separate open() calls. Since we use this call mostly for filtering out
-         * duplicates in the fd store this difference hopefully doesn't matter too much. */
+         * have F_DUPFD_QUERY/kcmp() or we don't. If we have F_DUPFD_QUERY/kcmp() this will only return true
+         * for dup()ed file descriptors, but not otherwise. If we don't have F_DUPFD_QUERY/kcmp() this will
+         * also return true for two fds of the same file, created by separate open() calls. Since we use this
+         * call mostly for filtering out duplicates in the fd store this difference hopefully doesn't matter
+         * too much.
+         *
+         * Guarantees that if either of the passed fds is not allocated we'll return -EBADF. */
+
+        if (a == b) {
+                /* Let's validate that the fd is valid */
+                r = fd_validate(a);
+                if (r < 0)
+                        return r;
 
-        if (a == b)
                 return true;
+        }
+
+        /* Try to use F_DUPFD_QUERY if we have it first, as it is the nicest API */
+        r = fcntl(a, F_DUPFD_QUERY, b);
+        if (r > 0)
+                return true;
+        if (r == 0) {
+                /* The kernel will return 0 in case the first fd is allocated, but the 2nd is not. (Which is different in the kcmp() case) Explicitly validate it hence. */
+                r = fd_validate(b);
+                if (r < 0)
+                        return r;
+
+                return false;
+        }
+        /* On old kernels (< 6.10) that do not support F_DUPFD_QUERY this will return EINVAL for regular fds, and EBADF on O_PATH fds. Confusing. */
+        if (errno == EBADF) {
+                /* EBADF could mean two things: the first fd is not valid, or it is valid and is O_PATH and
+                 * F_DUPFD_QUERY is not supported. Let's validate the fd explicitly, to distinguish this
+                 * case. */
+                r = fd_validate(a);
+                if (r < 0)
+                        return r;
+
+                /* If the fd is valid, but we got EBADF, then let's try kcmp(). */
+        } else if (!ERRNO_IS_NOT_SUPPORTED(errno) && !ERRNO_IS_PRIVILEGE(errno) && errno != EINVAL)
+                return -errno;
 
         /* Try to use kcmp() if we have it. */
         pid = getpid_cached();
         r = kcmp(pid, pid, KCMP_FILE, a, b);
-        if (r == 0)
-                return true;
-        if (r > 0)
-                return false;
+        if (r >= 0)
+                return !r;
         if (!ERRNO_IS_NOT_SUPPORTED(errno) && !ERRNO_IS_PRIVILEGE(errno))
                 return -errno;
 
-        /* We don't have kcmp(), use fstat() instead. */
+        /* We have neither F_DUPFD_QUERY nor kcmp(), use fstat() instead. */
         if (fstat(a, &sta) < 0)
                 return -errno;
 

diff --git a/src/basic/missing_fcntl.h b/src/basic/missing_fcntl.h
index 1248eb7e4df469e2f552cd5b804b5671822ee499..a6188879c1f352b85b4a83e1392d055997966d9a 100644 (file)
--- a/src/basic/missing_fcntl.h
+++ b/src/basic/missing_fcntl.h
@@ -7,6 +7,10 @@
 #define F_LINUX_SPECIFIC_BASE 1024
 #endif
 
+#ifndef F_DUPFD_QUERY
+#define F_DUPFD_QUERY (F_LINUX_SPECIFIC_BASE + 3)
+#endif
+
 #ifndef F_SETPIPE_SZ
 #define F_SETPIPE_SZ (F_LINUX_SPECIFIC_BASE + 7)
 #endif

diff --git a/src/test/test-fd-util.c b/src/test/test-fd-util.c
index e49a5dde457bf09a8466c3d03f7bd9732941a7e0..20cf7b7627b395fceb4c4a841142f8471055b95b 100644 (file)
--- a/src/test/test-fd-util.c
+++ b/src/test/test-fd-util.c
@@ -72,12 +72,14 @@ TEST(fd_validate) {
 
 TEST(same_fd) {
         _cleanup_close_pair_ int p[2];
-        _cleanup_close_ int a, b, c;
+        _cleanup_close_ int a, b, c, d, e;
 
         assert_se(pipe2(p, O_CLOEXEC) >= 0);
         assert_se((a = fcntl(p[0], F_DUPFD, 3)) >= 0);
         assert_se((b = open("/dev/null", O_RDONLY|O_CLOEXEC)) >= 0);
         assert_se((c = fcntl(a, F_DUPFD, 3)) >= 0);
+        assert_se((d = open("/dev/null", O_RDONLY|O_CLOEXEC|O_PATH)) >= 0); /* O_PATH changes error returns in F_DUPFD_QUERY, let's test explicitly */
+        assert_se((e = fcntl(d, F_DUPFD, 3)) >= 0);
 
         assert_se(same_fd(p[0], p[0]) > 0);
         assert_se(same_fd(p[1], p[1]) > 0);
@@ -102,6 +104,20 @@ TEST(same_fd) {
 
         assert_se(same_fd(a, b) == 0);
         assert_se(same_fd(b, a) == 0);
+
+        assert_se(same_fd(a, d) == 0);
+        assert_se(same_fd(d, a) == 0);
+        assert_se(same_fd(d, d) > 0);
+        assert_se(same_fd(d, e) > 0);
+        assert_se(same_fd(e, d) > 0);
+
+        /* Let's now compare with a valid fd nr, that is definitely closed, and verify it returns the right error code */
+        safe_close(d);
+        assert_se(same_fd(d, d) == -EBADF);
+        assert_se(same_fd(e, d) == -EBADF);
+        assert_se(same_fd(d, e) == -EBADF);
+        assert_se(same_fd(e, e) > 0);
+        TAKE_FD(d);
 }
 
 TEST(open_serialization_fd) {