Merge #12422: util: Make LockDirectory thread-safe, consistent, and fix OpenBSD 6.2 build

1d4cbd2 test: Add unit test for LockDirectory (Wladimir J. van der Laan)
fc888bf util: Fix multiple use of LockDirectory (Wladimir J. van der Laan)

Pull request description:

  Wrap the `boost::interprocess::file_lock` in a `std::unique_ptr` inside the map that keeps track of per-directory locks.

  This fixes a build issue with the clang 4.0.0+boost-1.58.0p8 version combo on OpenBSD 6.2, and should have no effect otherwise.

  Also add a unit test, make the function thread-safe, and fix Linux versus Windows behavior inconsistency.

  Meant to fix #12413.

Tree-SHA512: 1a94c714c932524a51212c46e8951c129337d57b00fd3da5a347c6bcf6a947706cd440f39df935591b2079995136917f71ca7435fb356f6e8a128c509a62ec32

src/test/util_tests.cpp

@@ -13,6 +13,10 @@
 
 #include <stdint.h>
 #include <vector>
+#ifndef WIN32
+#include <sys/types.h>
+#include <sys/wait.h>
+#endif
 
 #include <boost/test/unit_test.hpp>
 
@@ -603,4 +607,130 @@ BOOST_AUTO_TEST_CASE(test_ParseFixedPoint)
     BOOST_CHECK(!ParseFixedPoint("1.", 8, &amount));
 }
 
+static void TestOtherThread(fs::path dirname, std::string lockname, bool *result)
+{
+    *result = LockDirectory(dirname, lockname);
+}
+
+#ifndef WIN32 // Cannot do this test on WIN32 due to lack of fork()
+static constexpr char LockCommand = 'L';
+static constexpr char UnlockCommand = 'U';
+static constexpr char ExitCommand = 'X';
+
+static void TestOtherProcess(fs::path dirname, std::string lockname, int fd)
+{
+    char ch;
+    int rv;
+    while (true) {
+        rv = read(fd, &ch, 1); // Wait for command
+        assert(rv == 1);
+        switch(ch) {
+        case LockCommand:
+            ch = LockDirectory(dirname, lockname);
+            rv = write(fd, &ch, 1);
+            assert(rv == 1);
+            break;
+        case UnlockCommand:
+            ReleaseDirectoryLocks();
+            ch = true; // Always succeeds
+            rv = write(fd, &ch, 1);
+            break;
+        case ExitCommand:
+            close(fd);
+            exit(0);
+        default:
+            assert(0);
+        }
+    }
+}
+#endif
+
+BOOST_AUTO_TEST_CASE(test_LockDirectory)
+{
+    fs::path dirname = fs::temp_directory_path() / fs::unique_path();
+    const std::string lockname = ".lock";
+#ifndef WIN32
+    // Revert SIGCHLD to default, otherwise boost.test will catch and fail on
+    // it: there is BOOST_TEST_IGNORE_SIGCHLD but that only works when defined
+    // at build-time of the boost library
+    void (*old_handler)(int) = signal(SIGCHLD, SIG_DFL);
+
+    // Fork another process for testing before creating the lock, so that we
+    // won't fork while holding the lock (which might be undefined, and is not
+    // relevant as test case as that is avoided with -daemonize).
+    int fd[2];
+    BOOST_CHECK_EQUAL(socketpair(AF_UNIX, SOCK_STREAM, 0, fd), 0);
+    pid_t pid = fork();
+    if (!pid) {
+        BOOST_CHECK_EQUAL(close(fd[1]), 0); // Child: close parent end
+        TestOtherProcess(dirname, lockname, fd[0]);
+    }
+    BOOST_CHECK_EQUAL(close(fd[0]), 0); // Parent: close child end
+#endif
+    // Lock on non-existent directory should fail
+    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname), false);
+
+    fs::create_directories(dirname);
+
+    // Probing lock on new directory should succeed
+    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true);
+
+    // Persistent lock on new directory should succeed
+    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname), true);
+
+    // Another lock on the directory from the same thread should succeed
+    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname), true);
+
+    // Another lock on the directory from a different thread within the same process should succeed
+    bool threadresult;
+    std::thread thr(TestOtherThread, dirname, lockname, &threadresult);
+    thr.join();
+    BOOST_CHECK_EQUAL(threadresult, true);
+#ifndef WIN32
+    // Try to aquire lock in child process while we're holding it, this should fail.
+    char ch;
+    BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1);
+    BOOST_CHECK_EQUAL(read(fd[1], &ch, 1), 1);
+    BOOST_CHECK_EQUAL((bool)ch, false);
+
+    // Give up our lock
+    ReleaseDirectoryLocks();
+    // Probing lock from our side now should succeed, but not hold on to the lock.
+    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true);
+
+    // Try to acquire the lock in the child process, this should be succesful.
+    BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1);
+    BOOST_CHECK_EQUAL(read(fd[1], &ch, 1), 1);
+    BOOST_CHECK_EQUAL((bool)ch, true);
+
+    // When we try to probe the lock now, it should fail.
+    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), false);
+
+    // Unlock the lock in the child process
+    BOOST_CHECK_EQUAL(write(fd[1], &UnlockCommand, 1), 1);
+    BOOST_CHECK_EQUAL(read(fd[1], &ch, 1), 1);
+    BOOST_CHECK_EQUAL((bool)ch, true);
+
+    // When we try to probe the lock now, it should succeed.
+    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true);
+
+    // Re-lock the lock in the child process, then wait for it to exit, check
+    // successful return. After that, we check that exiting the process
+    // has released the lock as we would expect by probing it.
+    int processstatus;
+    BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1);
+    BOOST_CHECK_EQUAL(write(fd[1], &ExitCommand, 1), 1);
+    BOOST_CHECK_EQUAL(waitpid(pid, &processstatus, 0), pid);
+    BOOST_CHECK_EQUAL(processstatus, 0);
+    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true);
+
+    // Restore SIGCHLD
+    signal(SIGCHLD, old_handler);
+    BOOST_CHECK_EQUAL(close(fd[1]), 0); // Close our side of the socketpair
+#endif
+    // Clean up
+    ReleaseDirectoryLocks();
+    fs::remove_all(dirname);
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/util.cpp

@@ -373,20 +373,37 @@ int LogPrintStr(const std::string &str)
     return ret;
 }
 
+/** A map that contains all the currently held directory locks. After
+ * successful locking, these will be held here until the global destructor
+ * cleans them up and thus automatically unlocks them, or ReleaseDirectoryLocks
+ * is called.
+ */
+static std::map<std::string, std::unique_ptr<boost::interprocess::file_lock>> dir_locks;
+/** Mutex to protect dir_locks. */
+static std::mutex cs_dir_locks;
+
 bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only)
 {
+    std::lock_guard<std::mutex> ulock(cs_dir_locks);
     fs::path pathLockFile = directory / lockfile_name;
-    FILE* file = fsbridge::fopen(pathLockFile, "a"); // empty lock file; created if it doesn't exist.
+
+    // If a lock for this directory already exists in the map, don't try to re-lock it
+    if (dir_locks.count(pathLockFile.string())) {
+        return true;
+    }
+
+    // Create empty lock file if it doesn't exist.
+    FILE* file = fsbridge::fopen(pathLockFile, "a");
     if (file) fclose(file);
 
     try {
-        static std::map<std::string, boost::interprocess::file_lock> locks;
-        boost::interprocess::file_lock& lock = locks.emplace(pathLockFile.string(), pathLockFile.string().c_str()).first->second;
-        if (!lock.try_lock()) {
+        auto lock = MakeUnique<boost::interprocess::file_lock>(pathLockFile.string().c_str());
+        if (!lock->try_lock()) {
             return false;
         }
-        if (probe_only) {
-            lock.unlock();
+        if (!probe_only) {
+            // Lock successful and we're not just probing, put it into the map
+            dir_locks.emplace(pathLockFile.string(), std::move(lock));
         }
     } catch (const boost::interprocess::interprocess_exception& e) {
         return error("Error while attempting to lock directory %s: %s", directory.string(), e.what());
@@ -394,6 +411,12 @@ bool LockDirectory(const fs::path& directory, const std::string lockfile_name, b
     return true;
 }
 
+void ReleaseDirectoryLocks()
+{
+    std::lock_guard<std::mutex> ulock(cs_dir_locks);
+    dir_locks.clear();
+}
+
 /** Interpret string as boolean, for argument parsing */
 static bool InterpretBool(const std::string& strValue)
 {

src/util.h

@@ -174,6 +174,12 @@ int RaiseFileDescriptorLimit(int nMinFD);
 void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length);
 bool RenameOver(fs::path src, fs::path dest);
 bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only=false);
+
+/** Release all directory locks. This is used for unit testing only, at runtime
+ * the global destructor will take care of the locks.
+ */
+void ReleaseDirectoryLocks();
+
 bool TryCreateDirectories(const fs::path& p);
 fs::path GetDefaultDataDir();
 const fs::path &GetDataDir(bool fNetSpecific = true);