Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make database files' permissions configurable #3709

Closed
wants to merge 1 commit into from
Closed

Make database files' permissions configurable #3709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions env/env_posix.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ ThreadStatusUpdater* CreateThreadStatusUpdater() {
return new ThreadStatusUpdater();
}

inline mode_t GetDBFileMode(bool allow_non_owner_access) {
return allow_non_owner_access ? 0644 : 0600;
}

// list of pathnames that are locked
static std::set<std::string> lockedFiles;
static port::Mutex mutex_lockedFiles;
Expand Down Expand Up @@ -167,7 +171,7 @@ class PosixEnv : public Env {

do {
IOSTATS_TIMER_GUARD(open_nanos);
fd = open(fname.c_str(), flags, 0644);
fd = open(fname.c_str(), flags, GetDBFileMode(allow_non_owner_access_));
} while (fd < 0 && errno == EINTR);
if (fd < 0) {
return IOError("While opening a file for sequentially reading", fname,
Expand Down Expand Up @@ -217,7 +221,7 @@ class PosixEnv : public Env {

do {
IOSTATS_TIMER_GUARD(open_nanos);
fd = open(fname.c_str(), flags, 0644);
fd = open(fname.c_str(), flags, GetDBFileMode(allow_non_owner_access_));
} while (fd < 0 && errno == EINTR);
if (fd < 0) {
return IOError("While open a file for random read", fname, errno);
Expand Down Expand Up @@ -288,7 +292,7 @@ class PosixEnv : public Env {

do {
IOSTATS_TIMER_GUARD(open_nanos);
fd = open(fname.c_str(), flags, 0644);
fd = open(fname.c_str(), flags, GetDBFileMode(allow_non_owner_access_));
} while (fd < 0 && errno == EINTR);

if (fd < 0) {
Expand Down Expand Up @@ -376,7 +380,8 @@ class PosixEnv : public Env {

do {
IOSTATS_TIMER_GUARD(open_nanos);
fd = open(old_fname.c_str(), flags, 0644);
fd = open(old_fname.c_str(), flags,
GetDBFileMode(allow_non_owner_access_));
} while (fd < 0 && errno == EINTR);
if (fd < 0) {
s = IOError("while reopen file for write", fname, errno);
Expand Down Expand Up @@ -436,7 +441,8 @@ class PosixEnv : public Env {
int fd = -1;
while (fd < 0) {
IOSTATS_TIMER_GUARD(open_nanos);
fd = open(fname.c_str(), O_CREAT | O_RDWR, 0644);
fd = open(fname.c_str(), O_CREAT | O_RDWR,
GetDBFileMode(allow_non_owner_access_));
if (fd < 0) {
// Error while opening the file
if (errno == EINTR) {
Expand Down Expand Up @@ -789,6 +795,11 @@ class PosixEnv : public Env {
return thread_pools_[pri].GetBackgroundThreads();
}

virtual Status SetAllowNonOwnerAccess(bool allow_non_owner_access) override {
allow_non_owner_access_ = allow_non_owner_access;
return Status::OK();
}

// Allow increasing the number of worker threads.
virtual void IncBackgroundThreadsIfNeeded(int num, Priority pri) override {
assert(pri >= Priority::BOTTOM && pri <= Priority::HIGH);
Expand Down Expand Up @@ -885,13 +896,17 @@ class PosixEnv : public Env {
std::vector<ThreadPoolImpl> thread_pools_;
pthread_mutex_t mu_;
std::vector<pthread_t> threads_to_join_;
// If true, allow non owner read access for db files. Otherwise, non-owner
// has no access to db files.
bool allow_non_owner_access_;
};

PosixEnv::PosixEnv()
: checkedDiskForMmap_(false),
forceMmapOff_(false),
page_size_(getpagesize()),
thread_pools_(Priority::TOTAL) {
thread_pools_(Priority::TOTAL),
allow_non_owner_access_(true) {
ThreadPoolImpl::PthreadCall("mutex_init", pthread_mutex_init(&mu_, nullptr));
for (int pool_id = 0; pool_id < Env::Priority::TOTAL; ++pool_id) {
thread_pools_[pool_id].SetThreadPriority(
Expand Down
34 changes: 34 additions & 0 deletions env/env_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,40 @@ TEST_F(EnvPosixTest, AreFilesSame) {
}
#endif

#ifdef OS_LINUX
TEST_F(EnvPosixTest, FilePermission) {
// Only works for Linux environment
if (env_ == Env::Default()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this conditional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I borrowed the conditional check from TEST_P(EnvPosixTestWithParam, RandomAccessUniqueID)
I think it's a good conditional check to have. Since Env::Default() is PosixEnv on Linux platform

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note this case is TEST_F, not TEST_P, so in this case the Env under test is not parameterized. So this will always evaluate to either false or true -- I am not sure which, but hope true otherwise the test will be a no-op.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Linux, that conditional check would always succeed. And the test would always run on Linux. It is also fine if we delete that conditional check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiaofeidu008 Thanks for checking :).

EnvOptions soptions;
std::vector<std::string> fileNames {
test::TmpDir(env_) + "/testfile", test::TmpDir(env_) + "/testfile1"};
unique_ptr<WritableFile> wfile;
ASSERT_OK(env_->NewWritableFile(fileNames[0], &wfile, soptions));
unique_ptr<RandomRWFile> rwfile;
ASSERT_OK(env_->NewRandomRWFile(fileNames[1], &rwfile, soptions));

struct stat sb;
for (const auto& filename : fileNames) {
if (::stat(filename.c_str(), &sb) == 0) {
ASSERT_EQ(sb.st_mode & 0777, 0644);
}
env_->DeleteFile(filename);
}

env_->SetAllowNonOwnerAccess(false);
ASSERT_OK(env_->NewWritableFile(fileNames[0], &wfile, soptions));
ASSERT_OK(env_->NewRandomRWFile(fileNames[1], &rwfile, soptions));

for (const auto& filename : fileNames) {
if (::stat(filename.c_str(), &sb) == 0) {
ASSERT_EQ(sb.st_mode & 0777, 0600);
}
env_->DeleteFile(filename);
}
}
}
#endif

TEST_P(EnvPosixTestWithParam, UnSchedule) {
std::atomic<bool> called(false);
env_->SetBackgroundThreads(1, Env::LOW);
Expand Down
8 changes: 8 additions & 0 deletions include/rocksdb/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,10 @@ class Env {
virtual void SetBackgroundThreads(int number, Priority pri = LOW) = 0;
virtual int GetBackgroundThreads(Priority pri = LOW) = 0;

virtual Status SetAllowNonOwnerAccess(bool /*allow_non_owner_access*/) {
return Status::NotSupported("Not supported.");
}

// Enlarge number of background worker threads of a specific thread pool
// for this environment if it is smaller than specified. 'LOW' is the default
// pool.
Expand Down Expand Up @@ -1074,6 +1078,10 @@ class EnvWrapper : public Env {
return target_->GetBackgroundThreads(pri);
}

Status SetAllowNonOwnerAccess(bool allow_non_owner_access) override {
return target_->SetAllowNonOwnerAccess(allow_non_owner_access);
}

void IncBackgroundThreadsIfNeeded(int num, Priority pri) override {
return target_->IncBackgroundThreadsIfNeeded(num, pri);
}
Expand Down