Reapply "[libc] Return errno from OFD failure paths in fcntl." (#166658)

[jtstogel]

The previous implementation in #166252 (rolled back in #166658) caused buildbot failures due to a bug in an added test. The modified UseAfterClose did not pass a struct flock value to fcntl:

llvm-project/libc/test/src/fcntl/fcntl_test.cpp

Line 175 in 2d51705

ASSERT_EQ(-1, LIBC_NAMESPACE::fcntl(fd, GETLK_CMD));

Which ASAN caught and errored in the fcntl implementation when the unspecified argument was accessed:

llvm-project/libc/src/__support/OSUtil/linux/fcntl.cpp

Line 59 in c12cb28

flk64.l_type = flk->l_type;

[llvmbot]

@llvm/pr-subscribers-libc

Author: Jackson Stogel (jtstogel)

Changes

The previous implementation in #166252 (rolled back in #166658) caused buildbot failures due to a bug in an added test. The modified UseAfterClose did not pass a struct flock value to fcntl:

llvm-project/libc/test/src/fcntl/fcntl_test.cpp

Line 175 in 2d51705

ASSERT_EQ(-1, LIBC_NAMESPACE::fcntl(fd, GETLK_CMD));

Which ASAN caught and errored in the fcntl implementation when the unspecified argument was accessed:

llvm-project/libc/src/__support/OSUtil/linux/fcntl.cpp

Line 59 in c12cb28

flk64.l_type = flk->l_type;

---

Full diff: https://github.com/llvm/llvm-project/pull/166846.diff

2 Files Affected:

• (modified) libc/src/__support/OSUtil/linux/fcntl.cpp (+1-1)
• (modified) libc/test/src/fcntl/fcntl_test.cpp (+99-62)

diff --git a/libc/src/__support/OSUtil/linux/fcntl.cpp b/libc/src/__support/OSUtil/linux/fcntl.cpp
index bb76eee90efd2..08db4859c6417 100644
--- a/libc/src/__support/OSUtil/linux/fcntl.cpp
+++ b/libc/src/__support/OSUtil/linux/fcntl.cpp
@@ -66,7 +66,7 @@ ErrorOr<int> fcntl(int fd, int cmd, void *arg) {
         LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd, cmd, &flk64);
     // On failure, return
     if (ret < 0)
-      return Error(-1);
+      return Error(-ret);
     // Check for overflow, i.e. the offsets are not the same when cast
     // to off_t from off64_t.
     if (static_cast<off_t>(flk64.l_len) != flk64.l_len ||
diff --git a/libc/test/src/fcntl/fcntl_test.cpp b/libc/test/src/fcntl/fcntl_test.cpp
index 84feb34e537a0..41a9893d4d902 100644
--- a/libc/test/src/fcntl/fcntl_test.cpp
+++ b/libc/test/src/fcntl/fcntl_test.cpp
@@ -94,68 +94,105 @@ TEST_F(LlvmLibcFcntlTest, FcntlSetFl) {
   ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
 }
 
-TEST_F(LlvmLibcFcntlTest, FcntlGetLkRead) {
-  using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
-  constexpr const char *TEST_FILE_NAME = "testdata/fcntl_getlkread.test";
-  auto TEST_FILE = libc_make_test_file_path(TEST_FILE_NAME);
-
-  struct flock flk, svflk;
-  int retVal;
-  int fd =
-      LIBC_NAMESPACE::open(TEST_FILE, O_CREAT | O_TRUNC | O_RDONLY, S_IRWXU);
-  ASSERT_ERRNO_SUCCESS();
-  ASSERT_GT(fd, 0);
-
-  flk.l_type = F_RDLCK;
-  flk.l_start = 0;
-  flk.l_whence = SEEK_SET;
-  flk.l_len = 50;
-
-  // copy flk into svflk
-  svflk = flk;
-
-  retVal = LIBC_NAMESPACE::fcntl(fd, F_GETLK, &svflk);
-  ASSERT_ERRNO_SUCCESS();
-  ASSERT_GT(retVal, -1);
-  ASSERT_NE((int)flk.l_type, F_WRLCK); // File should not be write locked.
-
-  retVal = LIBC_NAMESPACE::fcntl(fd, F_SETLK, &svflk);
-  ASSERT_ERRNO_SUCCESS();
-  ASSERT_GT(retVal, -1);
-
-  ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
-}
-
-TEST_F(LlvmLibcFcntlTest, FcntlGetLkWrite) {
-  using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
-  constexpr const char *TEST_FILE_NAME = "testdata/fcntl_getlkwrite.test";
-  auto TEST_FILE = libc_make_test_file_path(TEST_FILE_NAME);
-
-  struct flock flk, svflk;
-  int retVal;
-  int fd = LIBC_NAMESPACE::open(TEST_FILE, O_CREAT | O_TRUNC | O_RDWR, S_IRWXU);
-  ASSERT_ERRNO_SUCCESS();
-  ASSERT_GT(fd, 0);
-
-  flk.l_type = F_WRLCK;
-  flk.l_start = 0;
-  flk.l_whence = SEEK_SET;
-  flk.l_len = 0;
-
-  // copy flk into svflk
-  svflk = flk;
-
-  retVal = LIBC_NAMESPACE::fcntl(fd, F_GETLK, &svflk);
-  ASSERT_ERRNO_SUCCESS();
-  ASSERT_GT(retVal, -1);
-  ASSERT_NE((int)flk.l_type, F_RDLCK); // File should not be read locked.
-
-  retVal = LIBC_NAMESPACE::fcntl(fd, F_SETLK, &svflk);
-  ASSERT_ERRNO_SUCCESS();
-  ASSERT_GT(retVal, -1);
-
-  ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
-}
+/* Tests that are common between OFD and traditional variants of fcntl locks. */
+template <int GETLK_CMD, int SETLK_CMD>
+class LibcFcntlCommonLockTests : public LlvmLibcFcntlTest {
+public:
+  void GetLkRead() {
+    using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+    constexpr const char *TEST_FILE_NAME = "testdata/fcntl_getlkread.test";
+    const auto TEST_FILE = libc_make_test_file_path(TEST_FILE_NAME);
+
+    struct flock flk = {};
+    struct flock svflk = {};
+    int retVal;
+    int fd =
+        LIBC_NAMESPACE::open(TEST_FILE, O_CREAT | O_TRUNC | O_RDONLY, S_IRWXU);
+    ASSERT_ERRNO_SUCCESS();
+    ASSERT_GT(fd, 0);
+
+    flk.l_type = F_RDLCK;
+    flk.l_start = 0;
+    flk.l_whence = SEEK_SET;
+    flk.l_len = 50;
+
+    // copy flk into svflk
+    svflk = flk;
+
+    retVal = LIBC_NAMESPACE::fcntl(fd, GETLK_CMD, &svflk);
+    ASSERT_ERRNO_SUCCESS();
+    ASSERT_GT(retVal, -1);
+    ASSERT_NE((int)svflk.l_type, F_WRLCK); // File should not be write locked.
+
+    retVal = LIBC_NAMESPACE::fcntl(fd, SETLK_CMD, &svflk);
+    ASSERT_ERRNO_SUCCESS();
+    ASSERT_GT(retVal, -1);
+
+    ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+  }
+
+  void GetLkWrite() {
+    using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+    constexpr const char *TEST_FILE_NAME = "testdata/fcntl_getlkwrite.test";
+    const auto TEST_FILE = libc_make_test_file_path(TEST_FILE_NAME);
+
+    struct flock flk = {};
+    struct flock svflk = {};
+    int retVal;
+    int fd =
+        LIBC_NAMESPACE::open(TEST_FILE, O_CREAT | O_TRUNC | O_RDWR, S_IRWXU);
+    ASSERT_ERRNO_SUCCESS();
+    ASSERT_GT(fd, 0);
+
+    flk.l_type = F_WRLCK;
+    flk.l_start = 0;
+    flk.l_whence = SEEK_SET;
+    flk.l_len = 0;
+
+    // copy flk into svflk
+    svflk = flk;
+
+    retVal = LIBC_NAMESPACE::fcntl(fd, GETLK_CMD, &svflk);
+    ASSERT_ERRNO_SUCCESS();
+    ASSERT_GT(retVal, -1);
+    ASSERT_NE((int)svflk.l_type, F_RDLCK); // File should not be read locked.
+
+    retVal = LIBC_NAMESPACE::fcntl(fd, SETLK_CMD, &svflk);
+    ASSERT_ERRNO_SUCCESS();
+    ASSERT_GT(retVal, -1);
+
+    ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+  }
+
+  void UseAfterClose() {
+    using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+    constexpr const char *TEST_FILE_NAME =
+        "testdata/fcntl_use_after_close.test";
+    const auto TEST_FILE = libc_make_test_file_path(TEST_FILE_NAME);
+    int fd =
+        LIBC_NAMESPACE::open(TEST_FILE, O_CREAT | O_TRUNC | O_RDWR, S_IRWXU);
+    ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+
+    struct flock flk = {};
+    flk.l_type = F_RDLCK;
+    flk.l_start = 0;
+    flk.l_whence = SEEK_SET;
+    flk.l_len = 50;
+    ASSERT_EQ(-1, LIBC_NAMESPACE::fcntl(fd, GETLK_CMD, &flk));
+    ASSERT_ERRNO_EQ(EBADF);
+  }
+};
+
+#define COMMON_LOCK_TESTS(NAME, GETLK_CMD, SETLK_CMD)                          \
+  using NAME = LibcFcntlCommonLockTests<GETLK_CMD, GETLK_CMD>;                 \
+  TEST_F(NAME, GetLkRead) { GetLkRead(); }                                     \
+  TEST_F(NAME, GetLkWrite) { GetLkWrite(); }                                   \
+  TEST_F(NAME, UseAfterClose) { UseAfterClose(); }                             \
+  static_assert(true, "Require semicolon.")
+
+COMMON_LOCK_TESTS(LlvmLibcFcntlProcessAssociatedLockTest, F_GETLK, F_SETLK);
+COMMON_LOCK_TESTS(LlvmLibcFcntlOpenFileDescriptionLockTest, F_OFD_GETLK,
+                  F_OFD_SETLK);
 
 TEST_F(LlvmLibcFcntlTest, UseAfterClose) {
   using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;