From de844c79d4df7a1349d7122494ae5e1de765c0d8 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Thu, 13 Feb 2025 20:44:35 +0100 Subject: [PATCH 1/3] util: Prevent multiple LockDirectory calls within the same process Previously LockDirectory only prevented concurrent locks across different processes, but allowed the same process to re-lock on the same directory. This change is not immediately relevant for its current use, where the lock is only supposed to protect against a different process writing on the same resources. This change is relevant for future use by the kernel library, where users of the library might mistakenly create multiple instances of an object that seek to write to a common resource. --- src/test/util_tests.cpp | 4 ++-- src/util/fs_helpers.cpp | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 4cacbd1151f..0b3a2a2f418 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -1244,13 +1244,13 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory) BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::Success); // Another lock on the directory from the same thread should succeed - BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::Success); + BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::ErrorLock); // Another lock on the directory from a different thread within the same process should succeed util::LockResult threadresult; std::thread thr([&] { threadresult = util::LockDirectory(dirname, lockname); }); thr.join(); - BOOST_CHECK_EQUAL(threadresult, util::LockResult::Success); + BOOST_CHECK_EQUAL(threadresult, util::LockResult::ErrorLock); #ifndef WIN32 // Try to acquire lock in child process while we're holding it, this should fail. BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1); diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp index 4d06afe1442..eb1c500830e 100644 --- a/src/util/fs_helpers.cpp +++ b/src/util/fs_helpers.cpp @@ -57,7 +57,8 @@ LockResult LockDirectory(const fs::path& directory, const fs::path& lockfile_nam // If a lock for this directory already exists in the map, don't try to re-lock it if (dir_locks.count(fs::PathToString(pathLockFile))) { - return LockResult::Success; + LogError("Error while attempting to lock directory %s: Lock already taken", fs::PathToString(directory)); + return LockResult::ErrorLock; } // Create empty lock file if it doesn't exist. From 21300478d93f754cdc5fc48facc19022d124775f Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 14 Feb 2025 22:20:05 +0100 Subject: [PATCH 2/3] util: Add RAII directory lock This makes it easier for a class or a struct to own a lock on a directory for the duration of its lifetime. It is used in the next commit. --- src/test/util_tests.cpp | 24 ++++++++++++++++++++++ src/util/fs_helpers.cpp | 45 +++++++++++++++++++++++++++++++++++++++++ src/util/fs_helpers.h | 17 ++++++++++++++++ 3 files changed, 86 insertions(+) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 0b3a2a2f418..7cc3452d936 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -1291,6 +1291,30 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory) BOOST_CHECK_EQUAL(processstatus, 0); BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::Success); + { + auto lock{DirectoryLock(dirname, "test")}; + BOOST_CHECK_THROW(DirectoryLock(dirname, "test"), std::runtime_error); + } + { + BOOST_CHECK_NO_THROW(DirectoryLock(dirname, "test")); + } + + { + DirectoryLock lock1(dirname, "test"); + DirectoryLock lock2(std::move(lock1)); + BOOST_CHECK_THROW(DirectoryLock(dirname, "test"), std::runtime_error); + } + + { + auto dirname_move = dirname / "move"; + fs::create_directories(dirname_move); + DirectoryLock lock1(dirname, "test"); + DirectoryLock lock2(dirname_move, "test"); + lock2 = std::move(lock1); + BOOST_CHECK_THROW(DirectoryLock(dirname, "test"), std::runtime_error); + BOOST_CHECK_NO_THROW(DirectoryLock(dirname_move, "test")); + } + // Restore SIGCHLD signal(SIGCHLD, old_handler); BOOST_CHECK_EQUAL(close(fd[1]), 0); // Close our side of the socketpair diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp index eb1c500830e..776c456548a 100644 --- a/src/util/fs_helpers.cpp +++ b/src/util/fs_helpers.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -91,6 +92,50 @@ void ReleaseDirectoryLocks() dir_locks.clear(); } +DirectoryLock::DirectoryLock(fs::path dir_path, std::string name) + : m_path{dir_path}, + m_name{name} +{ + // Ensures only a single lock is taken on the provided directory. + switch (util::LockDirectory(m_path, ".lock", false)) { + case util::LockResult::ErrorWrite: + throw std::runtime_error(strprintf(_("Cannot write to %s directory '%s'; check permissions."), m_name, fs::PathToString(m_path)).original); + case util::LockResult::ErrorLock: + throw std::runtime_error(strprintf(_("Cannot obtain a lock on %s directory %s. %s is probably already running."), m_name, fs::PathToString(m_path), CLIENT_NAME).original); + case util::LockResult::Success: + return; + } // no default case, so the compiler can warn about missing cases + assert(false); +} + +DirectoryLock::DirectoryLock(DirectoryLock&& other) noexcept + : m_path{std::move(other.m_path)}, + m_name{std::move(other.m_name)} +{ + other.m_path.clear(); + other.m_name.clear(); +} + +DirectoryLock& DirectoryLock::operator=(DirectoryLock&& other) noexcept +{ + if (this != &other) { + if (!m_path.empty()) { + UnlockDirectory(m_path, ".lock"); + } + m_path = std::move(other.m_path); + other.m_path.clear(); + m_name = std::move(other.m_name); + other.m_name.clear(); + } + return *this; +} + + +DirectoryLock::~DirectoryLock() +{ + if (!m_path.empty()) UnlockDirectory(m_path, ".lock"); +} + bool CheckDiskSpace(const fs::path& dir, uint64_t additional_bytes) { constexpr uint64_t min_disk_space = 52428800; // 50 MiB diff --git a/src/util/fs_helpers.h b/src/util/fs_helpers.h index 28dd6d979d5..89b4078fd50 100644 --- a/src/util/fs_helpers.h +++ b/src/util/fs_helpers.h @@ -45,6 +45,23 @@ enum class LockResult { [[nodiscard]] LockResult LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only = false); } // namespace util void UnlockDirectory(const fs::path& directory, const fs::path& lockfile_name); + +class DirectoryLock +{ + fs::path m_path; + std::string m_name; + +public: + explicit DirectoryLock(fs::path dir_path, std::string name); + ~DirectoryLock(); + + DirectoryLock(const DirectoryLock&) = delete; + DirectoryLock& operator=(const DirectoryLock&) = delete; + + DirectoryLock(DirectoryLock&& other) noexcept; + DirectoryLock& operator=(DirectoryLock&& other) noexcept; +}; + bool CheckDiskSpace(const fs::path& dir, uint64_t additional_bytes = 0); /** Get the size of a file by scanning it. From 51436f85a2bfcb7874a0c3b12add2df19ff18a3b Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 14 Feb 2025 22:20:54 +0100 Subject: [PATCH 3/3] init: Take lock on blocks directory in BlockManager ctor This moves the responsibility of taking the lock for the blocks directory into the BlockManager. Use the DirectoryLock wrapper to ensure it is the first resource to be acquired and is released again after use. This is relevant for the kernel library where the lock should be taken even if the user fails to explicitly do so. --- src/init.cpp | 14 +++----------- src/init/common.cpp | 7 ++++--- src/node/blockstorage.cpp | 4 +++- src/node/blockstorage.h | 3 ++- 4 files changed, 12 insertions(+), 16 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index d46318fd45e..9346e9e2048 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1107,11 +1107,6 @@ static bool LockDirectory(const fs::path& dir, bool probeOnly) } // no default case, so the compiler can warn about missing cases assert(false); } -static bool LockDirectories(bool probeOnly) -{ - return LockDirectory(gArgs.GetDataDirNet(), probeOnly) && \ - LockDirectory(gArgs.GetBlocksDirPath(), probeOnly); -} bool AppInitSanityChecks(const kernel::Context& kernel) { @@ -1129,7 +1124,8 @@ bool AppInitSanityChecks(const kernel::Context& kernel) // Probe the directory locks to give an early error message, if possible // We cannot hold the directory locks here, as the forking for daemon() hasn't yet happened, // and a fork will cause weird behavior to them. - return LockDirectories(true); + return LockDirectory(gArgs.GetDataDirNet(), /*probeOnly=*/true) + && LockDirectory(gArgs.GetBlocksDirPath(), /*probeOnly=*/true); } bool AppInitLockDirectories() @@ -1137,11 +1133,7 @@ bool AppInitLockDirectories() // After daemonization get the directory locks again and hold on to them until exit // This creates a slight window for a race condition to happen, however this condition is harmless: it // will at most make us exit without printing a message to console. - if (!LockDirectories(false)) { - // Detailed error printed inside LockDirectory - return false; - } - return true; + return LockDirectory(gArgs.GetDataDirNet(), /*probeOnly=*/false); } bool AppInitInterfaces(NodeContext& node) diff --git a/src/init/common.cpp b/src/init/common.cpp index bc0a2f65080..e90d34afb05 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -119,9 +119,10 @@ bool StartLogging(const ArgsManager& args) } if (!LogInstance().m_log_timestamps) - LogPrintf("Startup time: %s\n", FormatISO8601DateTime(GetTime())); - LogPrintf("Default data directory %s\n", fs::PathToString(GetDefaultDataDir())); - LogPrintf("Using data directory %s\n", fs::PathToString(gArgs.GetDataDirNet())); + LogInfo("Startup time: %s", FormatISO8601DateTime(GetTime())); + LogInfo("Default data directory %s", fs::PathToString(GetDefaultDataDir())); + LogInfo("Using data directory %s", fs::PathToString(gArgs.GetDataDirNet())); + LogInfo("Using blocks directory %s", fs::PathToString(gArgs.GetBlocksDirPath())); // Only log conf file usage message if conf file actually exists. fs::path config_file_path = args.GetConfigFilePath(); diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 372395dd24d..e3209847556 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -1165,7 +1166,8 @@ static auto InitBlocksdirXorKey(const BlockManager::Options& opts) } BlockManager::BlockManager(const util::SignalInterrupt& interrupt, Options opts) - : m_prune_mode{opts.prune_target > 0}, + : m_blocks_dir_lock{DirectoryLock(opts.blocks_dir, "blocks")}, + m_prune_mode{opts.prune_target > 0}, m_xor_key{InitBlocksdirXorKey(opts)}, m_opts{std::move(opts)}, m_block_file_seq{FlatFileSeq{m_opts.blocks_dir, "blk", m_opts.fast_prune ? 0x4000 /* 16kB */ : BLOCKFILE_CHUNK_SIZE}}, diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 8a34efadfea..5db9aa82d67 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -127,7 +128,6 @@ struct BlockfileCursor { std::ostream& operator<<(std::ostream& os, const BlockfileCursor& cursor); - /** * Maintains a tree of blocks (stored in `m_block_index`) which is consulted * to determine where the most-work tip is. @@ -141,6 +141,7 @@ class BlockManager friend ChainstateManager; private: + DirectoryLock m_blocks_dir_lock; const CChainParams& GetParams() const { return m_opts.chainparams; } const Consensus::Params& GetConsensus() const { return m_opts.chainparams.GetConsensus(); } /**