Fix close(-1) on failed open in btrfs rename fsync path (facebook#14443)

Summary:
Pull Request resolved: facebook#14443

In `PosixDirectory::FsyncWithDirOptions()`, when handling btrfs file rename
syncing, if `open()` fails and `fd` is -1, the code unconditionally calls
`close(fd)`. Calling `close(-1)` is undefined behavior per POSIX (returns
EBADF on Linux), and overwrites the original meaningful open error with a
misleading "While closing file after fsync" error message.

Fix: Guard the `close()` call with `fd >= 0`.

Reviewed By: xingbowang

Differential Revision: D95303407

fbshipit-source-id: 9b64b45a09e6ba41d87d164a1c094b4d22f7c186

Author: omkarhgawde

env/io_posix.cc

@@ -1940,7 +1940,10 @@ IOStatus PosixDirectory::FsyncWithDirOptions(
   assert(fd_ >= 0);  // Check use after close
   IOStatus s = IOStatus::OK();
 #ifndef OS_AIX
-  if (is_btrfs_) {
+  bool test_is_btrfs = is_btrfs_;
+  TEST_SYNC_POINT_CALLBACK("PosixDirectory::FsyncWithDirOptions:ForceBtrfs",
+                           &test_is_btrfs);
+  if (test_is_btrfs) {
     // skip dir fsync for new file creation, which is not needed for btrfs
     if (dir_fsync_options.reason == DirFsyncOptions::kNewFileSynced) {
       return s;
@@ -1959,7 +1962,7 @@ IOStatus PosixDirectory::FsyncWithDirOptions(
       } else if (fsync(fd) < 0) {
         s = IOError("While fsync renaming file", new_name, errno);
       }
-      if (close(fd) < 0) {
+      if (fd >= 0 && close(fd) < 0) {
         s = IOError("While closing file after fsync", new_name, errno);
       }
       return s;

env/io_posix_test.cc

@@ -3,11 +3,13 @@
 // COPYING file in the root directory) and Apache 2.0 License
 // (found in the LICENSE.Apache file in the root directory).
 
+#include "test_util/sync_point.h"
 #include "test_util/testharness.h"
 #include "util/random.h"
 
 #ifdef ROCKSDB_LIB_IO_POSIX
 #include "env/io_posix.h"
+#include "rocksdb/file_system.h"
 
 namespace ROCKSDB_NAMESPACE {
 
@@ -174,6 +176,47 @@ TEST_F(PosixWritableFileTest, SeekAfterExtend) {
   ASSERT_OK(fs->DeleteFile(path, IOOptions(), nullptr));
 }
 
+#ifdef OS_LINUX
+class PosixDirectoryTest : public testing::Test {};
+
+TEST_F(PosixDirectoryTest, BtrfsFsyncFailedOpenDoesNotCloseInvalidFd) {
+  // When FsyncWithDirOptions is called with kFileRenamed on a btrfs filesystem
+  // and open() fails, close(-1) should not be called. Without the fix,
+  // close(-1) is called which is POSIX undefined behavior and overwrites the
+  // meaningful open error with a misleading "While closing file after fsync".
+  std::shared_ptr<FileSystem> fs = FileSystem::Default();
+  std::string dir_path =
+      test::PerThreadDBPath("PosixDirectoryTest_BtrfsFsyncFailedOpen");
+  ASSERT_OK(fs->CreateDirIfMissing(dir_path, IOOptions(), nullptr));
+
+  std::unique_ptr<FSDirectory> dir;
+  ASSERT_OK(fs->NewDirectory(dir_path, IOOptions(), &dir, nullptr));
+
+  // Force the btrfs code path via sync point
+  SyncPoint::GetInstance()->SetCallBack(
+      "PosixDirectory::FsyncWithDirOptions:ForceBtrfs",
+      [](void* arg) { *static_cast<bool*>(arg) = true; });
+  SyncPoint::GetInstance()->EnableProcessing();
+
+  // Call FsyncWithDirOptions with a non-existent file for rename sync.
+  // open() will fail since the file doesn't exist.
+  DirFsyncOptions opts(std::string(dir_path + "/nonexistent_file"));
+  IOStatus s = dir->FsyncWithDirOptions(IOOptions(), nullptr, opts);
+
+  // Should get an error about open failing, NOT about closing
+  ASSERT_TRUE(s.IsIOError());
+  // The error message should mention "open", not "closing"
+  ASSERT_TRUE(s.ToString().find("open") != std::string::npos);
+  ASSERT_TRUE(s.ToString().find("closing") == std::string::npos);
+
+  SyncPoint::GetInstance()->DisableProcessing();
+  SyncPoint::GetInstance()->ClearAllCallBacks();
+
+  ASSERT_OK(dir->Close(IOOptions(), nullptr));
+  ASSERT_OK(fs->DeleteDir(dir_path, IOOptions(), nullptr));
+}
+#endif
+
 }  // namespace ROCKSDB_NAMESPACE
 #endif