fix for delete/open race (#4768)

CHANGELOG.md

@@ -6,7 +6,7 @@
 ### Fixed
 * <How to hit and notice issue? what was the impact?> ([#????](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.
 

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,15 @@ 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.
+ // 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 +2406,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;

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,12 +1088,31 @@ 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;
+ }
+ // 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);
+ }
+ }
+ 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);
  }
- REALM_ASSERT_EX(err == EEXIST, err);
  }
  if (exclusive) {
  // check if any shared locks are already taken by trying to open the pipe for writing
@@ -1826,9 +1844,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&)
 {

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<void>(fifo_path);
+ static_cast<void>(fifo_dir_path);
+ static_cast<void>(fifo_file_name);
 #endif
 }
 
@@ -1184,7 +1187,7 @@ inline void File::Map<T>::unmap() noexcept
 template <class T>
 inline T* File::Map<T>::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);

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<std::mutex> 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);