From e60cb99c580a602a83856769ad2ac882d3cdfcb5 Mon Sep 17 00:00:00 2001 From: MeshCollider Date: Fri, 15 Dec 2017 11:06:22 +1300 Subject: [PATCH 1/5] Add a lock to the wallet directory --- src/wallet/db.cpp | 66 +++++++++++++++++++++++++++++++++-------------- src/wallet/db.h | 2 +- 2 files changed, 47 insertions(+), 21 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 35ff0e1eec..c0c24ae98b 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -18,6 +18,7 @@ #include #endif +#include #include namespace { @@ -52,6 +53,24 @@ void CheckUniqueFileid(const CDBEnv& env, const std::string& filename, Db& db) } } } + +bool LockEnvDirectory(const fs::path& env_path) +{ + // Make sure only a single Bitcoin process is using the wallet directory. + fs::path lock_file_path = env_path / ".lock"; + FILE* file = fsbridge::fopen(lock_file_path, "a"); // empty lock file; created if it doesn't exist. + if (file) fclose(file); + + try { + static boost::interprocess::file_lock lock(lock_file_path.string().c_str()); + if (!lock.try_lock()) { + return false; + } + } catch (const boost::interprocess::interprocess_exception& e) { + return error("Error obtaining lock on wallet directory %s: %s.", env_path.string(), e.what()); + } + return true; +} } // namespace // @@ -95,13 +114,17 @@ void CDBEnv::Close() EnvShutdown(); } -bool CDBEnv::Open(const fs::path& pathIn) +bool CDBEnv::Open(const fs::path& pathIn, bool retry) { if (fDbEnvInit) return true; boost::this_thread::interruption_point(); + if (!LockEnvDirectory(pathIn)) { + return false; + } + strPath = pathIn.string(); fs::path pathLogDir = pathIn / "database"; TryCreateDirectories(pathLogDir); @@ -134,7 +157,24 @@ bool CDBEnv::Open(const fs::path& pathIn) S_IRUSR | S_IWUSR); if (ret != 0) { dbenv->close(0); - return error("CDBEnv::Open: Error %d opening database environment: %s\n", ret, DbEnv::strerror(ret)); + LogPrintf("CDBEnv::Open: Error %d opening database environment: %s\n", ret, DbEnv::strerror(ret)); + if (retry) { + // try moving the database env out of the way + fs::path pathDatabaseBak = pathIn / strprintf("database.%d.bak", GetTime()); + try { + fs::rename(pathLogDir, pathDatabaseBak); + LogPrintf("Moved old %s to %s. Retrying.\n", pathLogDir.string(), pathDatabaseBak.string()); + } catch (const fs::filesystem_error&) { + // failure is ok (well, not really, but it's not worse than what we started with) + } + // try opening it again one more time + if (!Open(pathIn, false)) { + // if it still fails, it probably means we can't even create the database env + return false; + } + } else { + return false; + } } fDbEnvInit = true; @@ -269,25 +309,11 @@ bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& walle return false; } - if (!bitdb.Open(walletDir)) - { - // try moving the database env out of the way - fs::path pathDatabase = walletDir / "database"; - fs::path pathDatabaseBak = walletDir / strprintf("database.%d.bak", GetTime()); - try { - fs::rename(pathDatabase, pathDatabaseBak); - LogPrintf("Moved old %s to %s. Retrying.\n", pathDatabase.string(), pathDatabaseBak.string()); - } catch (const fs::filesystem_error&) { - // failure is ok (well, not really, but it's not worse than what we started with) - } - - // try again - if (!bitdb.Open(walletDir)) { - // if it still fails, it probably means we can't even create the database env - errorStr = strprintf(_("Error initializing wallet database environment %s!"), walletDir); - return false; - } + if (!bitdb.Open(walletDir, true)) { + errorStr = strprintf(_("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it."), walletDir); + return false; } + return true; } diff --git a/src/wallet/db.h b/src/wallet/db.h index c6f317927f..787135e400 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -68,7 +68,7 @@ public: typedef std::pair, std::vector > KeyValPair; bool Salvage(const std::string& strFile, bool fAggressive, std::vector& vResult); - bool Open(const fs::path& path); + bool Open(const fs::path& path, bool retry = 0); void Close(); void Flush(bool fShutdown); void CheckpointLSN(const std::string& strFile); From c9ed4bd58cac05b355ab103476ff29ecd10ce263 Mon Sep 17 00:00:00 2001 From: MeshCollider Date: Fri, 15 Dec 2017 11:15:18 +1300 Subject: [PATCH 2/5] Add a test for wallet directory locking --- test/functional/multiwallet.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/test/functional/multiwallet.py b/test/functional/multiwallet.py index 12d9e9f48d..38e2a8bfec 100755 --- a/test/functional/multiwallet.py +++ b/test/functional/multiwallet.py @@ -15,8 +15,8 @@ from test_framework.util import assert_equal, assert_raises_rpc_error class MultiWalletTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True - self.num_nodes = 1 - self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3', '-wallet=w']] + self.num_nodes = 2 + self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3', '-wallet=w'], []] self.supports_cli = True def run_test(self): @@ -28,7 +28,7 @@ class MultiWalletTest(BitcoinTestFramework): assert_equal(set(node.listwallets()), {"w1", "w2", "w3", "w"}) - self.stop_node(0) + self.stop_nodes() # should not initialize if there are duplicate wallets self.assert_start_raises_init_error(0, ['-wallet=w1', '-wallet=w1'], 'Error loading wallet w1. Duplicate -wallet filename specified.') @@ -59,19 +59,21 @@ class MultiWalletTest(BitcoinTestFramework): assert_equal(set(node.listwallets()), {"w4", "w5"}) w5 = wallet("w5") w5.generate(1) - self.stop_node(0) # now if wallets/ exists again, but the rootdir is specified as the walletdir, w4 and w5 should still be loaded os.rename(wallet_dir2, wallet_dir()) - self.start_node(0, ['-wallet=w4', '-wallet=w5', '-walletdir=' + data_dir()]) + self.restart_node(0, ['-wallet=w4', '-wallet=w5', '-walletdir=' + data_dir()]) assert_equal(set(node.listwallets()), {"w4", "w5"}) w5 = wallet("w5") w5_info = w5.getwalletinfo() assert_equal(w5_info['immature_balance'], 50) - self.stop_node(0) + competing_wallet_dir = os.path.join(self.options.tmpdir, 'competing_walletdir') + os.mkdir(competing_wallet_dir) + self.restart_node(0, ['-walletdir='+competing_wallet_dir]) + self.assert_start_raises_init_error(1, ['-walletdir='+competing_wallet_dir], 'Cannot obtain a lock on wallet directory') - self.start_node(0, self.extra_args[0]) + self.restart_node(0, self.extra_args[0]) w1 = wallet("w1") w2 = wallet("w2") From 64226de908c76997fadf147342c494ad0662fa43 Mon Sep 17 00:00:00 2001 From: MeshCollider Date: Sun, 24 Dec 2017 12:45:33 +1300 Subject: [PATCH 3/5] Generalise walletdir lock error message for correctness --- src/wallet/db.cpp | 5 +++-- test/functional/multiwallet.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index c0c24ae98b..a647a5b025 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -121,11 +121,12 @@ bool CDBEnv::Open(const fs::path& pathIn, bool retry) boost::this_thread::interruption_point(); + strPath = pathIn.string(); if (!LockEnvDirectory(pathIn)) { + LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it.", strPath); return false; } - strPath = pathIn.string(); fs::path pathLogDir = pathIn / "database"; TryCreateDirectories(pathLogDir); fs::path pathErrorFile = pathIn / "db.log"; @@ -310,7 +311,7 @@ bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& walle } if (!bitdb.Open(walletDir, true)) { - errorStr = strprintf(_("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it."), walletDir); + errorStr = strprintf(_("Error initializing wallet database environment %s!"), walletDir); return false; } diff --git a/test/functional/multiwallet.py b/test/functional/multiwallet.py index 38e2a8bfec..0891829127 100755 --- a/test/functional/multiwallet.py +++ b/test/functional/multiwallet.py @@ -71,7 +71,7 @@ class MultiWalletTest(BitcoinTestFramework): competing_wallet_dir = os.path.join(self.options.tmpdir, 'competing_walletdir') os.mkdir(competing_wallet_dir) self.restart_node(0, ['-walletdir='+competing_wallet_dir]) - self.assert_start_raises_init_error(1, ['-walletdir='+competing_wallet_dir], 'Cannot obtain a lock on wallet directory') + self.assert_start_raises_init_error(1, ['-walletdir='+competing_wallet_dir], 'Error initializing wallet database environment') self.restart_node(0, self.extra_args[0]) From 5260a4aca1e6c11e1dbca9d3390c0dc739e81204 Mon Sep 17 00:00:00 2001 From: MeshCollider Date: Tue, 26 Dec 2017 19:18:39 +1300 Subject: [PATCH 4/5] Make .walletlock distinct from .lock --- src/wallet/db.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index a647a5b025..e3366e1060 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -57,7 +57,7 @@ void CheckUniqueFileid(const CDBEnv& env, const std::string& filename, Db& db) bool LockEnvDirectory(const fs::path& env_path) { // Make sure only a single Bitcoin process is using the wallet directory. - fs::path lock_file_path = env_path / ".lock"; + fs::path lock_file_path = env_path / ".walletlock"; FILE* file = fsbridge::fopen(lock_file_path, "a"); // empty lock file; created if it doesn't exist. if (file) fclose(file); @@ -123,7 +123,7 @@ bool CDBEnv::Open(const fs::path& pathIn, bool retry) strPath = pathIn.string(); if (!LockEnvDirectory(pathIn)) { - LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it.", strPath); + LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it.\n", strPath); return false; } From 2f3bd47d44634cfc0a4261e64af178407ce2869c Mon Sep 17 00:00:00 2001 From: MeshCollider Date: Tue, 26 Dec 2017 19:41:55 +1300 Subject: [PATCH 5/5] Abstract directory locking into util.cpp --- src/init.cpp | 19 +++---------------- src/util.cpp | 22 ++++++++++++++++++++++ src/util.h | 1 + src/wallet/db.cpp | 21 +-------------------- 4 files changed, 27 insertions(+), 36 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 7215e87359..b48802637b 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1143,23 +1143,10 @@ bool AppInitParameterInteraction() static bool LockDataDirectory(bool probeOnly) { - std::string strDataDir = GetDataDir().string(); - // Make sure only a single Bitcoin process is using the data directory. - fs::path pathLockFile = GetDataDir() / ".lock"; - FILE* file = fsbridge::fopen(pathLockFile, "a"); // empty lock file; created if it doesn't exist. - if (file) fclose(file); - - try { - static boost::interprocess::file_lock lock(pathLockFile.string().c_str()); - if (!lock.try_lock()) { - return InitError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running."), strDataDir, _(PACKAGE_NAME))); - } - if (probeOnly) { - lock.unlock(); - } - } catch(const boost::interprocess::interprocess_exception& e) { - return InitError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running.") + " %s.", strDataDir, _(PACKAGE_NAME), e.what())); + fs::path datadir = GetDataDir(); + if (!LockDirectory(datadir, ".lock", probeOnly)) { + return InitError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running."), datadir.string(), _(PACKAGE_NAME))); } return true; } diff --git a/src/util.cpp b/src/util.cpp index 150bc503df..80eed24ffd 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -72,6 +72,7 @@ #include // for to_lower() #include // for startswith() and endswith() +#include #include #include #include @@ -375,6 +376,27 @@ int LogPrintStr(const std::string &str) return ret; } +bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only) +{ + fs::path pathLockFile = directory / lockfile_name; + FILE* file = fsbridge::fopen(pathLockFile, "a"); // empty lock file; created if it doesn't exist. + if (file) fclose(file); + + try { + static std::map locks; + boost::interprocess::file_lock& lock = locks.emplace(pathLockFile.string(), pathLockFile.string().c_str()).first->second; + if (!lock.try_lock()) { + return false; + } + if (probe_only) { + lock.unlock(); + } + } catch (const boost::interprocess::interprocess_exception& e) { + return error("Error while attempting to lock directory %s: %s", directory.string(), e.what()); + } + return true; +} + /** Interpret string as boolean, for argument parsing */ static bool InterpretBool(const std::string& strValue) { diff --git a/src/util.h b/src/util.h index 6a0d6a31e7..277b4c66af 100644 --- a/src/util.h +++ b/src/util.h @@ -173,6 +173,7 @@ bool TruncateFile(FILE *file, unsigned int length); 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); bool TryCreateDirectories(const fs::path& p); fs::path GetDefaultDataDir(); const fs::path &GetDataDir(bool fNetSpecific = true); diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index e3366e1060..23c6279128 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -18,7 +18,6 @@ #include #endif -#include #include namespace { @@ -53,24 +52,6 @@ void CheckUniqueFileid(const CDBEnv& env, const std::string& filename, Db& db) } } } - -bool LockEnvDirectory(const fs::path& env_path) -{ - // Make sure only a single Bitcoin process is using the wallet directory. - fs::path lock_file_path = env_path / ".walletlock"; - FILE* file = fsbridge::fopen(lock_file_path, "a"); // empty lock file; created if it doesn't exist. - if (file) fclose(file); - - try { - static boost::interprocess::file_lock lock(lock_file_path.string().c_str()); - if (!lock.try_lock()) { - return false; - } - } catch (const boost::interprocess::interprocess_exception& e) { - return error("Error obtaining lock on wallet directory %s: %s.", env_path.string(), e.what()); - } - return true; -} } // namespace // @@ -122,7 +103,7 @@ bool CDBEnv::Open(const fs::path& pathIn, bool retry) boost::this_thread::interruption_point(); strPath = pathIn.string(); - if (!LockEnvDirectory(pathIn)) { + if (!LockDirectory(pathIn, ".walletlock")) { LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it.\n", strPath); return false; }