Skip to content

Commit

Permalink
Add O_CLOEXEC to open calls.
Browse files Browse the repository at this point in the history 
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 #623
  • Loading branch information
@adam-azarchs
adam-azarchs committed Sep 20, 2018
1 parent 73d5834 commit 18136e9
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 9 deletions.
22 changes: 14 additions & 8 deletions util/env_posix.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ namespace leveldb {

namespace {

#ifndef O_CLOEXEC
#define O_CLOEXEC 0
#endif

static int open_read_only_file_limit = -1;
static int mmap_limit = -1;

Expand Down Expand Up @@ -159,7 +163,7 @@ class PosixRandomAccessFile: public RandomAccessFile {
char* scratch) const {
int fd = fd_;
if (temporary_fd_) {
fd = open(filename_.c_str(), O_RDONLY);
fd = open(filename_.c_str(), O_RDONLY | O_CLOEXEC);
if (fd < 0) {
return PosixError(filename_, errno);
}
Expand Down Expand Up @@ -316,7 +320,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_CLOEXEC);
if (fd < 0) {
status = PosixError(dirname_, errno);
} else {
Expand Down Expand Up @@ -421,7 +425,7 @@ class PosixEnv : public Env {

virtual Status NewSequentialFile(const std::string& fname,
SequentialFile** result) {
int fd = open(fname.c_str(), O_RDONLY);
int fd = open(fname.c_str(), O_RDONLY | O_CLOEXEC);
if (fd < 0) {
*result = nullptr;
return PosixError(fname, errno);
Expand All @@ -435,7 +439,7 @@ class PosixEnv : public Env {
RandomAccessFile** result) {
*result = nullptr;
Status s;
int fd = open(fname.c_str(), O_RDONLY);
int fd = open(fname.c_str(), O_RDONLY | O_CLOEXEC);
if (fd < 0) {
s = PosixError(fname, errno);
} else if (mmap_limit_.Acquire()) {
Expand All @@ -462,7 +466,8 @@ class PosixEnv : public Env {
virtual Status NewWritableFile(const std::string& fname,
WritableFile** result) {
Status s;
int fd = open(fname.c_str(), O_TRUNC | O_WRONLY | O_CREAT, 0644);
int fd = open(fname.c_str(), O_TRUNC | O_WRONLY | O_CREAT | O_CLOEXEC,
0644);
if (fd < 0) {
*result = nullptr;
s = PosixError(fname, errno);
Expand All @@ -475,7 +480,8 @@ class PosixEnv : public Env {
virtual Status NewAppendableFile(const std::string& fname,
WritableFile** result) {
Status s;
int fd = open(fname.c_str(), O_APPEND | O_WRONLY | O_CREAT, 0644);
int fd = open(fname.c_str(), O_APPEND | O_WRONLY | O_CREAT | O_CLOEXEC,
0644);
if (fd < 0) {
*result = nullptr;
s = PosixError(fname, errno);
Expand Down Expand Up @@ -551,7 +557,7 @@ class PosixEnv : public Env {
virtual Status LockFile(const std::string& fname, FileLock** lock) {
*lock = nullptr;
Status result;
int fd = open(fname.c_str(), O_RDWR | O_CREAT, 0644);
int fd = open(fname.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0644);
if (fd < 0) {
result = PosixError(fname, errno);
} else if (!locks_.Insert(fname)) {
Expand Down Expand Up @@ -601,7 +607,7 @@ class PosixEnv : public Env {
}

virtual Status NewLogger(const std::string& fname, Logger** result) {
FILE* f = fopen(fname.c_str(), "w");
FILE* f = fopen(fname.c_str(), "we");
if (f == nullptr) {
*result = nullptr;
return PosixError(fname, errno);
Expand Down
95 changes: 94 additions & 1 deletion util/env_posix_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <sys/wait.h>
#include <unistd.h>

#include "leveldb/env.h"

#include "port/port.h"
Expand Down Expand Up @@ -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);
Expand All @@ -56,9 +59,99 @@ TEST(EnvPosixTest, TestOpenOnRead) {
ASSERT_OK(env_->DeleteFile(test_file));
}

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<std::string> 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";
FILE* f = fopen(test_file.c_str(), "we");
ASSERT_TRUE(f != nullptr);
ASSERT_LE(0, fputs(kFileData, f));
ASSERT_EQ(0, fclose(f));
}
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"};
execv(child[0], const_cast<char* const*>(child));
}
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));
}
}

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) {
printf("File descriptor %d is %s\n", i, dbuf);
} else if (result >= 1024) {
printf("(file name length is too long)\n");
} else {
printf("Couldn't get file name: %s\n", strerror(errno));
}
return 1;
} else {
int e = errno;
if (e != EBADF) {
printf("Unexpected result reading file handle %d: %s\n", i,
strerror(errno));
return 2;
}
}
}
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);
Expand Down

0 comments on commit 18136e9

Please sign in to comment.