From 3b8fbaeb192b9f447d60d691c5c8b45649d40b84 Mon Sep 17 00:00:00 2001 From: Finn Schiermer Andersen Date: Thu, 17 Jun 2021 11:54:04 +0200 Subject: [PATCH 1/2] fix for delete/open race --- CHANGELOG.md | 2 +- src/realm/db.cpp | 15 ++++++++--- src/realm/util/file.cpp | 56 +++++++++++++++++++++++++---------------- src/realm/util/file.hpp | 13 ++++++---- test/test_shared.cpp | 21 ++++++++++------ 5 files changed, 69 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc2111fefdc..a6eb7efbcc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) * Fixed a recursive loop which would eventually crash trying to refresh a user app token when it had been revoked by an admin. Now this situation logs the user out and reports an error. ([#4745](https://github.com/realm/realm-core/issues/4745), since v10.0.0). - +* Fixed a race between calling realm::delete_files and concurent opening of the realm file.([#4768](https://github.com/realm/realm-core/pull/4768)) ### Breaking changes * None. diff --git a/src/realm/db.cpp b/src/realm/db.cpp index fe1d94ae729..16801d34b49 100644 --- a/src/realm/db.cpp +++ b/src/realm/db.cpp @@ -766,7 +766,6 @@ void DB::do_open(const std::string& path, bool no_create_file, bool is_backend, auto core_files = DB::get_core_files(path); m_lockfile_path = core_files[DB::CoreFileType::Lock].first; m_coordination_dir = core_files[DB::CoreFileType::Management].first; - try_make_dir(m_coordination_dir); m_lockfile_prefix = m_coordination_dir + "/access_control"; m_alloc.set_read_only(false); @@ -809,7 +808,7 @@ void DB::do_open(const std::string& path, bool no_create_file, bool is_backend, m_file.open(m_lockfile_path, File::access_ReadWrite, File::create_Auto, 0); // Throws File::CloseGuard fcg(m_file); - m_file.set_fifo_path(m_coordination_dir + "/lock.fifo"); + m_file.set_fifo_path(m_coordination_dir, "lock.fifo"); if (m_file.try_lock_exclusive()) { // Throws File::UnlockGuard ulg(m_file); @@ -849,6 +848,16 @@ void DB::do_open(const std::string& path, bool no_create_file, bool is_backend, #else m_file.lock_shared(); // Throws #endif + // The coordination/management dir is created as a side effect of the lock + // operation above if needed for lock emulation. But it may also be needed + // for other purposes, so make sure it exists: + if (!util::File::is_dir(m_coordination_dir)) { + // in worst case there'll be a race on creating this directory. + // This should be safe but a waste of resources. + // Unfortunately it cannot be created at an earlier point, because + // it may then be deleted during the above lock_shared() operation. + try_make_dir(m_coordination_dir); + } // If the file is not completely initialized at this point in time, the // preceeding initialization attempt must have failed. We know that an // initialization process was in progress, because this thread (or @@ -2398,7 +2407,7 @@ bool DB::call_with_lock(const std::string& realm_path, CallbackWithLock callback File lockfile; lockfile.open(lockfile_path, File::access_ReadWrite, File::create_Auto, 0); // Throws File::CloseGuard fcg(lockfile); - lockfile.set_fifo_path(realm_path + ".management/lock.fifo"); + lockfile.set_fifo_path(realm_path + ".management", "lock.fifo"); if (lockfile.try_lock_exclusive()) { // Throws callback(realm_path); return true; diff --git a/src/realm/util/file.cpp b/src/realm/util/file.cpp index 83bfad5efa4..6a011cfec42 100644 --- a/src/realm/util/file.cpp +++ b/src/realm/util/file.cpp @@ -115,11 +115,11 @@ bool for_each_helper(const std::string& path, const std::string& dir, File::ForE { DirScanner ds{path}; // Throws std::string name; - while (ds.next(name)) { // Throws + while (ds.next(name)) { // Throws std::string subpath = File::resolve(name, path); // Throws bool go_on; - if (File::is_dir(subpath)) { // Throws - std::string subdir = File::resolve(name, dir); // Throws + if (File::is_dir(subpath)) { // Throws + std::string subdir = File::resolve(name, dir); // Throws go_on = for_each_helper(subpath, subdir, handler); // Throws } else { @@ -276,10 +276,10 @@ bool try_remove_dir_recursive(const std::string& path) bool allow_missing = true; DirScanner ds{path, allow_missing}; // Throws std::string name; - while (ds.next(name)) { // Throws + while (ds.next(name)) { // Throws std::string subpath = File::resolve(name, path); // Throws - if (File::is_dir(subpath)) { // Throws - try_remove_dir_recursive(subpath); // Throws + if (File::is_dir(subpath)) { // Throws + try_remove_dir_recursive(subpath); // Throws } else { File::remove(subpath); // Throws @@ -382,8 +382,7 @@ void File::open_internal(const std::string& path, AccessMode a, CreateMode c, in } DWORD flags_and_attributes = 0; std::wstring ws = string_to_wstring(path); - HANDLE handle = - CreateFile2(ws.c_str(), desired_access, share_mode, creation_disposition, nullptr); + HANDLE handle = CreateFile2(ws.c_str(), desired_access, share_mode, creation_disposition, nullptr); if (handle != INVALID_HANDLE_VALUE) { m_fd = handle; m_have_lock = false; @@ -613,7 +612,7 @@ void File::write_static(FileDesc fd, const char* data, size_t size) throw OutOfDiskSpace(msg); } throw std::system_error(err, std::system_category(), "write() failed"); -// LCOV_EXCL_STOP + // LCOV_EXCL_STOP #endif } @@ -920,13 +919,13 @@ bool File::prealloc_if_supported(SizeType offset, size_t size) } throw std::system_error(status, std::system_category(), "posix_fallocate() failed"); -// FIXME: OS X does not have any version of fallocate, but see -// http://stackoverflow.com/questions/11497567/fallocate-command-equivalent-in-os-x + // FIXME: OS X does not have any version of fallocate, but see + // http://stackoverflow.com/questions/11497567/fallocate-command-equivalent-in-os-x -// FIXME: On Windows one could use a call to CreateFileMapping() -// since it will grow the file if necessary, but never shrink it, -// just like posix_fallocate(). The advantage would be that it -// then becomes an atomic operation (probably). + // FIXME: On Windows one could use a call to CreateFileMapping() + // since it will grow the file if necessary, but never shrink it, + // just like posix_fallocate(). The advantage would be that it + // then becomes an atomic operation (probably). #else @@ -1089,11 +1088,28 @@ bool File::lock(bool exclusive, bool non_blocking) if (status) { int err = errno; REALM_ASSERT_EX(status == -1, status); - if (exclusive && err == ENOENT) { + if (err == ENOENT) { // The management directory doesn't exist, so there's clearly no - // readers. This can happen when calling DB::call_with_lock(). - return true; + // readers. This can happen when calling DB::call_with_lock() or + // if the management directory has been removed by DB::call_with_lock() + if (exclusive) { + return true; + } + else { + // open shared: + // We need the fifo in order to make a shared lock. If we have it + // in a management directory, we may need to create that first: + if (!m_fifo_dir_path.empty()) + try_make_dir(m_fifo_dir_path); + // now we can try creating the FIFO again + status = mkfifo(m_fifo_path.c_str(), 0666); + if (status) { + // If we fail it must be because it already exists, check further down + err = errno; + } + } } + // if failed to create the fifo, it must be because it already exists! REALM_ASSERT_EX(err == EEXIST, err); } if (exclusive) { @@ -1826,9 +1842,7 @@ DirScanner::DirScanner(const std::string&, bool) throw util::runtime_error("Not yet supported"); } -DirScanner::~DirScanner() noexcept -{ -} +DirScanner::~DirScanner() noexcept {} bool DirScanner::next(std::string&) { diff --git a/src/realm/util/file.hpp b/src/realm/util/file.hpp index 31c1708dd6d..a58507b51f9 100644 --- a/src/realm/util/file.hpp +++ b/src/realm/util/file.hpp @@ -372,7 +372,7 @@ class File { /// Set the path used for emulating file locks. If not set explicitly, /// the emulation will use the path of the file itself suffixed by ".fifo" - void set_fifo_path(const std::string& fifo_path); + void set_fifo_path(const std::string& fifo_dir_path, const std::string& fifo_file_name); enum { /// If possible, disable opportunistic flushing of dirted /// pages of a memory mapped file to physical medium. On some @@ -617,6 +617,7 @@ class File { #ifdef REALM_FILELOCK_EMULATION int m_pipe_fd = -1; // -1 if no pipe has been allocated for emulation bool m_has_exclusive_lock = false; + std::string m_fifo_dir_path; std::string m_fifo_path; #endif #endif @@ -1010,12 +1011,14 @@ inline File::~File() noexcept close(); } -inline void File::set_fifo_path(const std::string& fifo_path) +inline void File::set_fifo_path(const std::string& fifo_dir_path, const std::string& fifo_file_name) { #ifdef REALM_FILELOCK_EMULATION - m_fifo_path = fifo_path; + m_fifo_dir_path = fifo_dir_path; + m_fifo_path = fifo_dir_path + "/" + fifo_file_name; #else - static_cast(fifo_path); + static_cast(fifo_dir_path); + static_cast(fifo_file_name); #endif } @@ -1184,7 +1187,7 @@ inline void File::Map::unmap() noexcept template inline T* File::Map::remap(const File& f, AccessMode a, size_t size, int map_flags) { - //MapBase::remap(f, a, size, map_flags); + // MapBase::remap(f, a, size, map_flags); // missing sync() here? unmap(); map(f, a, size, map_flags); diff --git a/test/test_shared.cpp b/test/test_shared.cpp index ec5dc633c43..8255f2ec399 100644 --- a/test/test_shared.cpp +++ b/test/test_shared.cpp @@ -944,7 +944,9 @@ TEST(Shared_try_begin_write) // wait for the thread to start a write transaction std::unique_lock lock(cv_lock); - cv.wait(lock, [&] { return init_complete; }); + cv.wait(lock, [&] { + return init_complete; + }); // Try to also obtain a write lock. This should fail but not block. auto tr = sg->start_write(true); @@ -1535,7 +1537,9 @@ TEST(Shared_WriterThreads) // Create all threads for (int i = 0; i < thread_count; ++i) - threads[i].start([this, &sg, i] { writer_threads_thread(test_context, sg, ObjKey(i)); }); + threads[i].start([this, &sg, i] { + writer_threads_thread(test_context, sg, ObjKey(i)); + }); // Wait for all threads to complete for (int i = 0; i < thread_count; ++i) @@ -2099,8 +2103,9 @@ void multiprocess_threaded(TestContext& test_context, std::string path, int64_t // Start threads for (int64_t i = 0; i != num_threads; ++i) { - threads[i].start( - [&test_context, &path, base, i] { multiprocess_thread(test_context, path, ObjKey(base + i)); }); + threads[i].start([&test_context, &path, base, i] { + multiprocess_thread(test_context, path, ObjKey(base + i)); + }); } // Wait for threads to finish @@ -3504,7 +3509,7 @@ TEST(Shared_LockFileOfWrongSizeThrows) Thread t; auto do_async = [&]() { File f(path.get_lock_path(), File::mode_Write); - f.set_fifo_path(std::string(path) + ".management/lock.fifo"); + f.set_fifo_path(std::string(path) + ".management", "lock.fifo"); f.lock_shared(); File::UnlockGuard ug(f); @@ -3569,7 +3574,7 @@ TEST(Shared_LockFileOfWrongVersionThrows) File f; f.open(path.get_lock_path(), File::access_ReadWrite, File::create_Auto, 0); // Throws - f.set_fifo_path(std::string(path) + ".management/lock.fifo"); + f.set_fifo_path(std::string(path) + ".management", "lock.fifo"); f.lock_shared(); File::UnlockGuard ug(f); @@ -3622,7 +3627,7 @@ TEST(Shared_LockFileOfWrongMutexSizeThrows) auto do_async = [&]() { File f; f.open(path.get_lock_path(), File::access_ReadWrite, File::create_Auto, 0); // Throws - f.set_fifo_path(std::string(path) + ".management/lock.fifo"); + f.set_fifo_path(std::string(path) + ".management", "lock.fifo"); f.lock_shared(); File::UnlockGuard ug(f); @@ -3676,7 +3681,7 @@ TEST(Shared_LockFileOfWrongCondvarSizeThrows) auto do_async = [&]() { File f; f.open(path.get_lock_path(), File::access_ReadWrite, File::create_Auto, 0); // Throws - f.set_fifo_path(std::string(path) + ".management/lock.fifo"); + f.set_fifo_path(std::string(path) + ".management", "lock.fifo"); f.lock_shared(); File::UnlockGuard ug(f); From 6090c47b2b7646f5f4b0b9b0af0092a4915c4651 Mon Sep 17 00:00:00 2001 From: Finn Schiermer Andersen Date: Fri, 18 Jun 2021 08:48:36 +0200 Subject: [PATCH 2/2] post review fixes --- src/realm/db.cpp | 15 +++++++-------- src/realm/util/file.cpp | 30 ++++++++++++++++-------------- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/realm/db.cpp b/src/realm/db.cpp index 16801d34b49..3669e506f3b 100644 --- a/src/realm/db.cpp +++ b/src/realm/db.cpp @@ -850,14 +850,13 @@ void DB::do_open(const std::string& path, bool no_create_file, bool is_backend, #endif // The coordination/management dir is created as a side effect of the lock // operation above if needed for lock emulation. But it may also be needed - // for other purposes, so make sure it exists: - if (!util::File::is_dir(m_coordination_dir)) { - // in worst case there'll be a race on creating this directory. - // This should be safe but a waste of resources. - // Unfortunately it cannot be created at an earlier point, because - // it may then be deleted during the above lock_shared() operation. - try_make_dir(m_coordination_dir); - } + // for other purposes, so make sure it exists. + // in worst case there'll be a race on creating this directory. + // This should be safe but a waste of resources. + // Unfortunately it cannot be created at an earlier point, because + // it may then be deleted during the above lock_shared() operation. + try_make_dir(m_coordination_dir); + // If the file is not completely initialized at this point in time, the // preceeding initialization attempt must have failed. We know that an // initialization process was in progress, because this thread (or diff --git a/src/realm/util/file.cpp b/src/realm/util/file.cpp index 6a011cfec42..f8973dde675 100644 --- a/src/realm/util/file.cpp +++ b/src/realm/util/file.cpp @@ -1095,22 +1095,24 @@ bool File::lock(bool exclusive, bool non_blocking) if (exclusive) { return true; } - else { - // open shared: - // We need the fifo in order to make a shared lock. If we have it - // in a management directory, we may need to create that first: - if (!m_fifo_dir_path.empty()) - try_make_dir(m_fifo_dir_path); - // now we can try creating the FIFO again - status = mkfifo(m_fifo_path.c_str(), 0666); - if (status) { - // If we fail it must be because it already exists, check further down - err = errno; - } + // open shared: + // We need the fifo in order to make a shared lock. If we have it + // in a management directory, we may need to create that first: + if (!m_fifo_dir_path.empty()) + try_make_dir(m_fifo_dir_path); + // now we can try creating the FIFO again + status = mkfifo(m_fifo_path.c_str(), 0666); + if (status) { + // If we fail it must be because it already exists + err = errno; + REALM_ASSERT_EX(err == EEXIST, err); } } - // if failed to create the fifo, it must be because it already exists! - REALM_ASSERT_EX(err == EEXIST, err); + else { + // if we failed to create the fifo and not because dir is missing, + // it must be because the fifo already exists! + REALM_ASSERT_EX(err == EEXIST, err); + } } if (exclusive) { // check if any shared locks are already taken by trying to open the pipe for writing