Fixed race conditions in file_utils::mkdir (#9192)

3rdParty/iresearch/core/utils/file_utils.cpp

@@ -732,7 +732,7 @@ handle_t open(FILE* file, const file_path_t mode) NOEXCEPT {
 // --SECTION--                                                        path utils
 // -----------------------------------------------------------------------------
 
-bool mkdir(const file_path_t path) NOEXCEPT {
+bool mkdir(const file_path_t path, bool createNew) NOEXCEPT {
   bool result;
 
   if (!exists_directory(result, path)) {
@@ -740,31 +740,18 @@ bool mkdir(const file_path_t path) NOEXCEPT {
   }
 
   if (result) {
-    return true; // already exists
+    return !createNew;  // directory already exists.
   }
 
-  if (!exists(result, path) || result) {
-    return false; // failure checking existence or something else exists with the same name
-  }
+  // we do not check existence of anything other than directory, as race conditions will arise on second check. Just rely on OS file manager to deal with all name conflicts.
 
   auto parts = path_parts(path);
 
   if (!parts.dirname.empty()) {
     // need a null terminated string for use with ::mkdir()/::CreateDirectoryW()
     std::basic_string<std::remove_pointer<file_path_t>::type> parent(parts.dirname);
-
-    if (!mkdir(parent.c_str())) {
-#ifdef _WIN32
-	  if (::GetLastError() != ERROR_ALREADY_EXISTS) {
-	    // failed to create parent
-	    return false;
-	  }
-#else
-      if (errno != EEXIST) {
-        // failed to create parent
-        return false;
-      }
-#endif
+    if (!mkdir(parent.c_str(), false)) { // intermediate path parts can exist, this is ok anyway
+      return false;
     }
   }
 
@@ -778,16 +765,18 @@ bool mkdir(const file_path_t path) NOEXCEPT {
     // '\\?\' cannot be used with relative paths
     if (!abs) {
       if (0 == ::CreateDirectoryW(path, nullptr)) {
-        typedef std::remove_pointer<file_path_t>::type char_t;
-        auto locale = irs::locale_utils::locale(irs::string_ref::NIL, "utf8", true); // utf8 internal and external
-        std::string utf8path;
+        if (::GetLastError() != ERROR_ALREADY_EXISTS || createNew) {
+          // failed to create directory  or directory exist, but we are asked to perform creation
+          typedef std::remove_pointer<file_path_t>::type char_t;
+          auto locale = irs::locale_utils::locale(irs::string_ref::NIL, "utf8", true);  // utf8 internal and external
+          std::string utf8path;
 
-        irs::locale_utils::append_external<char_t>(utf8path, path, locale);
-        IR_FRMT_ERROR("Failed to create path: '%s', error %d", utf8path.c_str(), GetLastError());
-
-        return false;
+          irs::locale_utils::append_external<char_t>(utf8path, path, locale);
+          IR_FRMT_ERROR("Failed to create path: '%s', error %d",
+                        utf8path.c_str(), GetLastError());
+          return false;
+        }
       }
-
       return true;
     }
 
@@ -800,20 +789,25 @@ bool mkdir(const file_path_t path) NOEXCEPT {
     );
 
     if (0 == ::CreateDirectoryW(dirname.c_str(), nullptr)) {
-      typedef std::remove_pointer<file_path_t>::type char_t;
-      auto locale = irs::locale_utils::locale(irs::string_ref::NIL, "utf8", true); // utf8 internal and external
-      std::string utf8path;
+      if (::GetLastError() != ERROR_ALREADY_EXISTS || createNew) {
+        // failed to create directory  or directory exist, but we are asked to perform creation
+        typedef std::remove_pointer<file_path_t>::type char_t;
+        auto locale = irs::locale_utils::locale(irs::string_ref::NIL, "utf8", true); // utf8 internal and external
+        std::string utf8path;
 
-      irs::locale_utils::append_external<char_t>(utf8path, path, locale);
-      IR_FRMT_ERROR("Failed to create path: '%s', error %d", utf8path.c_str(), GetLastError());
+        irs::locale_utils::append_external<char_t>(utf8path, path, locale);
+        IR_FRMT_ERROR("Failed to create path: '%s', error %d", utf8path.c_str(), GetLastError());
 
-      return false;
+        return false;
+      }
     }
   #else
     if (0 != ::mkdir(path, S_IRWXU|S_IRWXG|S_IRWXO)) {
-      IR_FRMT_ERROR("Failed to create path: '%s', error %d", path, errno);
-
-      return false;
+      if (errno != EEXIST || createNew) {
+        // failed to create directory  or directory exist, but we are asked to perform creation
+        IR_FRMT_ERROR("Failed to create path: '%s', error %d", path, errno);
+        return false;
+      }
     }
   #endif
 

3rdParty/iresearch/core/utils/file_utils.hpp

@@ -139,7 +139,7 @@ handle_t open(FILE* file, const file_path_t mode) NOEXCEPT;
 // --SECTION--                                                        path utils
 // -----------------------------------------------------------------------------
 
-bool mkdir(const file_path_t path) NOEXCEPT; // recursive directory creation
+bool mkdir(const file_path_t path, bool createNew) NOEXCEPT;  // recursive directory creation
 
 bool move(const file_path_t src_path, const file_path_t dst_path) NOEXCEPT;
 

3rdParty/iresearch/core/utils/utf8_path.cpp

@@ -338,8 +338,9 @@ bool utf8_path::mtime(time_t& result) const NOEXCEPT {
   return irs::file_utils::mtime(result, c_str());
 }
 
-bool utf8_path::mkdir() const NOEXCEPT {
-  return irs::file_utils::mkdir(c_str());
+
+bool utf8_path::mkdir(bool createNew /* = true*/) const NOEXCEPT {
+  return irs::file_utils::mkdir(c_str(), createNew); 
 }
 
 bool utf8_path::remove() const NOEXCEPT {

3rdParty/iresearch/core/utils/utf8_path.hpp

@@ -67,7 +67,9 @@ class IRESEARCH_API utf8_path {
   bool exists_directory(bool& result) const NOEXCEPT;
   bool exists_file(bool& result) const NOEXCEPT;
   bool file_size(uint64_t& result) const NOEXCEPT;
-  bool mkdir() const NOEXCEPT;
+  /// @brief creates path 
+  /// @param createNew requires at least last segment of path to be actually created or error will be detected
+  bool mkdir(bool createNew  = true) const NOEXCEPT;
   bool mtime(time_t& result) const NOEXCEPT;
   bool remove() const NOEXCEPT;
   bool rename(const utf8_path& destination) const NOEXCEPT;

3rdParty/iresearch/tests/store/directory_test_case.cpp

@@ -1106,7 +1106,7 @@ class fs_directory_test : public test_base {
     path_ = test_case_dir();
     path_ /= name_;
 
-    ASSERT_TRUE(path_.mkdir());
+    ASSERT_TRUE(path_.mkdir(false));
     dir_ = std::make_shared<fs_directory>(path_.utf8());
   }
 

3rdParty/iresearch/tests/tests_main.cpp

@@ -311,7 +311,7 @@ void test_base::SetUp() {
 
   test_case_dir_ /= ti->test_case_name();
   (test_dir_ = test_case_dir_) /= ti->name();
-  test_dir_.mkdir();
+  test_dir_.mkdir(false);
 }
 
 // -----------------------------------------------------------------------------

3rdParty/iresearch/tests/tests_param.cpp

@@ -43,7 +43,7 @@ std::pair<std::shared_ptr<irs::directory>, std::string> fs_directory(const test_
     auto dir = test->test_dir();
 
     dir /= "index";
-    dir.mkdir();
+    dir.mkdir(false);
 
     impl = std::shared_ptr<irs::fs_directory>(
       new irs::fs_directory(dir.utf8()),
@@ -63,7 +63,7 @@ std::pair<std::shared_ptr<irs::directory>, std::string> mmap_directory(const tes
     auto dir = test->test_dir();
 
     dir /= "index";
-    dir.mkdir();
+    dir.mkdir(false);
 
     impl = std::shared_ptr<irs::mmap_directory>(
       new irs::mmap_directory(dir.utf8()),

3rdParty/iresearch/tests/utils/utf8_path_tests.cpp

@@ -22,6 +22,8 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 #include <fstream>
+#include <thread>
+#include <condition_variable>
 
 #include "tests_shared.hpp"
 #include "utils/file_utils.hpp"
@@ -782,6 +784,105 @@ TEST_F(utf8_path_tests, directory) {
 
     ASSERT_FALSE(path2.remove()); // path already removed
   }
+
+  // recursive path creation with concurrency (full path exists)
+  {
+    std::string directory1("deleteme1/deleteme2/deleteme3");
+    irs::utf8_path path1;
+    irs::utf8_path path2;
+
+    path1 /= directory1;
+    path2 /= directory1;
+   
+    EXPECT_TRUE(path1.mkdir());
+    EXPECT_FALSE(path2.mkdir()); // directory already exists 
+    EXPECT_TRUE(path2.mkdir(false)); // directory exists, but creation is not mandatory
+
+    ASSERT_TRUE(path1.remove());
+    ASSERT_FALSE(path2.remove()); // path already removed
+  }
+  // recursive path creation with concurrency (only last sement added)
+  {
+    std::string directory1("deleteme1/deleteme2/deleteme3");
+    std::string directory2("deleteme4");
+    irs::utf8_path path1;
+    irs::utf8_path path2;
+
+    path1 /= directory1;
+    path2 /= directory1;
+    path2 /= directory2;
+
+    ASSERT_TRUE(path1.mkdir());
+    ASSERT_TRUE(path2.mkdir()); // last segment created
+
+    ASSERT_TRUE(path1.remove());
+    ASSERT_FALSE(path2.remove()); // path already removed
+  }
+  // race condition test inside path tree building
+  {
+    std::string directory1("deleteme1");
+    std::string directory2("deleteme2/deleteme3/deleteme_thread");
+    irs::utf8_path pathRoot;
+    pathRoot /= directory1;
+
+    // threads sync for start
+    std::mutex mutex;
+    std::condition_variable ready_cv;
+
+    for (size_t j = 0; j < 3; ++j) {
+      pathRoot.remove(); // make sure full path tree building always needed
+      
+      const auto thread_count = 20;
+      std::vector<int> results(thread_count, false);
+      std::vector<std::thread> pool;
+      // We need all threads to be in same position to maximize test validity (not just ready to run!). 
+      // So we count ready threads
+      size_t readyCount = 0; 
+      bool ready = false;  // flag to indicate all is ready (needed for spurious wakeup check)
+
+      for (size_t i = 0; i < thread_count; ++i) {
+        auto& result = results[i];
+        pool.emplace_back(std::thread([ &result, &directory1, &directory2, i, &mutex, &ready_cv, &readyCount, &ready]() {
+          irs::utf8_path path;
+          path /= directory1;
+          std::ostringstream ss;
+          ss << directory2 << i;
+          path /= ss.str();
+          
+          std::unique_lock<std::mutex> lk(mutex);
+          ++readyCount;
+          while (!ready) {
+            ready_cv.wait(lk);
+          }
+          lk.unlock();
+          result = path.mkdir(true) ? 1 : 0;
+        }));
+      }
+     
+      while(true) {
+        {
+          std::lock_guard<decltype(mutex)> lock(mutex);
+          if (readyCount >= thread_count) { 
+            // all threads on positions... go, go, go...
+            ready = true;
+            ready_cv.notify_all();
+            break;
+          }
+        }
+        std::this_thread::sleep_for(std::chrono::milliseconds(1000));
+      }
+      for (auto& thread : pool) {
+        thread.join();
+      }
+     
+      ASSERT_TRUE(std::all_of(
+        results.begin(), results.end(), [](bool res) { return res != 0; }
+      ));
+      pathRoot.remove(); // cleanup
+    }
+  }
+
+
 }
 
 void validate_move(bool src_abs, bool dst_abs) {