From 75fceae7003e217e16b04433831da7528ae56881 Mon Sep 17 00:00:00 2001 From: Adam Azarchs Date: Wed, 19 Sep 2018 16:29:00 -0700 Subject: [PATCH] Add O_CLOEXEC to open calls. This prevents file descriptors from leaking to child processes. When compiled for older (pre-2.6.23) kernels which lack support for O_CLOEXEC there is no change in behavior. With newer kernels, child processes will no longer inherit leveldb's file handles, which reduces the changes of accidentally corrupting the database. Fixes https://github.com/google/leveldb/issues/623 --- CMakeLists.txt | 1 + port/port_config.h.in | 5 +++ util/env_posix.cc | 27 +++++++++---- util/env_posix_test.cc | 102 ++++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 126 insertions(+), 9 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f6a7c0a..9b0042b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -38,6 +38,7 @@ include(CheckCXXSymbolExists) # (-std=c11), but do expose the function in standard C++ mode (-std=c++11). check_cxx_symbol_exists(fdatasync "unistd.h" HAVE_FDATASYNC) check_cxx_symbol_exists(F_FULLFSYNC "fcntl.h" HAVE_FULLFSYNC) +check_cxx_symbol_exists(O_CLOEXEC "fcntl.h" HAVE_O_CLOEXEC) include(CheckCXXSourceCompiles) diff --git a/port/port_config.h.in b/port/port_config.h.in index d6a6d01..2127315 100644 --- a/port/port_config.h.in +++ b/port/port_config.h.in @@ -15,6 +15,11 @@ #cmakedefine01 HAVE_FULLFSYNC #endif // !defined(HAVE_FULLFSYNC) +// Define to 1 if you have a definition for O_CLOEXEC in . +#if !defined(HAVE_O_CLOEXEC) +#cmakedefine01 HAVE_O_CLOEXEC +#endif // !defined(HAVE_O_CLOEXEC) + // Define to 1 if you have Google CRC32C. #if !defined(HAVE_CRC32C) #cmakedefine01 HAVE_CRC32C diff --git a/util/env_posix.cc b/util/env_posix.cc index 362adb3..7a0f04d 100644 --- a/util/env_posix.cc +++ b/util/env_posix.cc @@ -48,6 +48,13 @@ constexpr const int kDefaultMmapLimit = (sizeof(void*) >= 8) ? 1000 : 0; // Can be set using EnvPosixTestHelper::SetReadOnlyMMapLimit. int g_mmap_limit = kDefaultMmapLimit; +// Common flags defined for all posix open operations +#if defined(HAVE_O_CLOEXEC) +constexpr const int O_FLAGS = O_CLOEXEC; +#else +constexpr const int O_FLAGS = 0; +#endif // defined(HAVE_O_CLOEXEC) + constexpr const size_t kWritableFileBufferSize = 65536; Status PosixError(const std::string& context, int error_number) { @@ -168,7 +175,7 @@ class PosixRandomAccessFile final : public RandomAccessFile { char* scratch) const override { int fd = fd_; if (!has_permanent_fd_) { - fd = ::open(filename_.c_str(), O_RDONLY); + fd = ::open(filename_.c_str(), O_RDONLY | O_FLAGS); if (fd < 0) { return PosixError(filename_, errno); } @@ -343,7 +350,7 @@ class PosixWritableFile final : public WritableFile { return status; } - int fd = ::open(dirname_.c_str(), O_RDONLY); + int fd = ::open(dirname_.c_str(), O_RDONLY | O_FLAGS); if (fd < 0) { status = PosixError(dirname_, errno); } else { @@ -491,7 +498,7 @@ class PosixEnv : public Env { Status NewSequentialFile(const std::string& filename, SequentialFile** result) override { - int fd = ::open(filename.c_str(), O_RDONLY); + int fd = ::open(filename.c_str(), O_RDONLY | O_FLAGS); if (fd < 0) { *result = nullptr; return PosixError(filename, errno); @@ -504,7 +511,7 @@ class PosixEnv : public Env { Status NewRandomAccessFile(const std::string& filename, RandomAccessFile** result) override { *result = nullptr; - int fd = ::open(filename.c_str(), O_RDONLY); + int fd = ::open(filename.c_str(), O_RDONLY | O_FLAGS); if (fd < 0) { return PosixError(filename, errno); } @@ -536,7 +543,9 @@ class PosixEnv : public Env { Status NewWritableFile(const std::string& filename, WritableFile** result) override { - int fd = ::open(filename.c_str(), O_TRUNC | O_WRONLY | O_CREAT, 0644); + int fd = ::open(filename.c_str(), + O_TRUNC | O_WRONLY | O_CREAT | O_FLAGS, + 0644); if (fd < 0) { *result = nullptr; return PosixError(filename, errno); @@ -548,7 +557,9 @@ class PosixEnv : public Env { Status NewAppendableFile(const std::string& filename, WritableFile** result) override { - int fd = ::open(filename.c_str(), O_APPEND | O_WRONLY | O_CREAT, 0644); + int fd = ::open(filename.c_str(), + O_APPEND | O_WRONLY | O_CREAT | O_FLAGS, + 0644); if (fd < 0) { *result = nullptr; return PosixError(filename, errno); @@ -618,7 +629,7 @@ class PosixEnv : public Env { Status LockFile(const std::string& filename, FileLock** lock) override { *lock = nullptr; - int fd = ::open(filename.c_str(), O_RDWR | O_CREAT, 0644); + int fd = ::open(filename.c_str(), O_RDWR | O_CREAT | O_FLAGS, 0644); if (fd < 0) { return PosixError(filename, errno); } @@ -674,7 +685,7 @@ class PosixEnv : public Env { } Status NewLogger(const std::string& filename, Logger** result) override { - std::FILE* fp = std::fopen(filename.c_str(), "w"); + std::FILE* fp = std::fopen(filename.c_str(), "we"); if (fp == nullptr) { *result = nullptr; return PosixError(filename, errno); diff --git a/util/env_posix_test.cc b/util/env_posix_test.cc index e28df9a..5519d8d 100644 --- a/util/env_posix_test.cc +++ b/util/env_posix_test.cc @@ -2,6 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. See the AUTHORS file for names of contributors. +#include +#include + #include "leveldb/env.h" #include "port/port.h" @@ -31,7 +34,7 @@ TEST(EnvPosixTest, TestOpenOnRead) { ASSERT_OK(env_->GetTestDirectory(&test_dir)); std::string test_file = test_dir + "/open_on_read.txt"; - FILE* f = fopen(test_file.c_str(), "w"); + FILE* f = fopen(test_file.c_str(), "we"); ASSERT_TRUE(f != nullptr); const char kFileData[] = "abcdefghijklmnopqrstuvwxyz"; fputs(kFileData, f); @@ -56,9 +59,106 @@ TEST(EnvPosixTest, TestOpenOnRead) { ASSERT_OK(env_->DeleteFile(test_file)); } +#if defined(HAVE_O_CLOEXEC) + +TEST(EnvPosixTest, TestCloseOnExec) { + // Test that file handles are not inherited by child processes. + + // Open file handles with each of the open methods. + std::string test_dir; + ASSERT_OK(env_->GetTestDirectory(&test_dir)); + std::vector test_files = { + test_dir + "/close_on_exec_seq.txt", + test_dir + "/close_on_exec_rand.txt", + test_dir + "/close_on_exec_write.txt", + test_dir + "/close_on_exec_append.txt", + test_dir + "/close_on_exec_lock.txt", + test_dir + "/close_on_exec_log.txt", + }; + for (const std::string& test_file : test_files) { + const char kFileData[] = "0123456789"; + ASSERT_OK(WriteStringToFile(env_, kFileData, test_file)); + } + leveldb::SequentialFile* seqFile = nullptr; + leveldb::RandomAccessFile* randFile = nullptr; + leveldb::WritableFile* writeFile = nullptr; + leveldb::WritableFile* appendFile = nullptr; + leveldb::FileLock* lockFile = nullptr; + leveldb::Logger* logFile = nullptr; + ASSERT_OK(env_->NewSequentialFile(test_files[0], &seqFile)); + ASSERT_OK(env_->NewRandomAccessFile(test_files[1], &randFile)); + ASSERT_OK(env_->NewWritableFile(test_files[2], &writeFile)); + ASSERT_OK(env_->NewAppendableFile(test_files[3], &appendFile)); + ASSERT_OK(env_->LockFile(test_files[4], &lockFile)); + ASSERT_OK(env_->NewLogger(test_files[5], &logFile)); + + // Fork a child process and wait for it to complete. + int pid = fork(); + if (pid == 0) { + const char* const child[] = {"/proc/self/exe", "-cloexec-child", nullptr}; + execv(child[0], const_cast(child)); + printf("Error spawning child process: %s\n", strerror(errno)); + exit(6); + } + int status; + waitpid(pid, &status, 0); + ASSERT_EQ(0, WEXITSTATUS(status)); + + // cleanup + ASSERT_OK(env_->UnlockFile(lockFile)); + delete seqFile; + delete randFile; + delete writeFile; + delete appendFile; + delete logFile; + for (const std::string& test_file : test_files) { + ASSERT_OK(env_->DeleteFile(test_file)); + } +} + +#endif // defined(HAVE_O_CLOEXEC) + +int cloexecChild() { + // Checks for open file descriptors in the range 3..FD_SETSIZE. + for (int i = 3; i < FD_SETSIZE; i++) { + int dup_result = dup2(i, i); + if (dup_result != -1) { + printf("Unexpected open file %d\n", i); + char nbuf[28]; + snprintf(nbuf, 28, "/proc/self/fd/%d", i); + char dbuf[1024]; + int result = readlink(nbuf, dbuf, 1024); + if (0 < result && result < 1024) { + dbuf[result] = 0; + printf("File descriptor %d is %s\n", i, dbuf); + if (strstr(dbuf, "close_on_exec_") == nullptr) { + continue; + } + } else if (result >= 1024) { + printf("(file name length is too long)\n"); + } else { + printf("Couldn't get file name: %s\n", strerror(errno)); + } + return 3; + } else { + int e = errno; + if (e != EBADF) { + printf("Unexpected result reading file handle %d: %s\n", i, + strerror(errno)); + return 4; + } + } + } + return 0; +} + } // namespace leveldb int main(int argc, char** argv) { + // Check if this is the child process for TestCloseOnExec + if (argc > 1 && strcmp(argv[1], "-cloexec-child") == 0) { + return leveldb::cloexecChild(); + } // All tests currently run with the same read-only file limits. leveldb::EnvPosixTest::SetFileLimits(leveldb::kReadOnlyFileLimit, leveldb::kMMapLimit);