Merge #14552: wallet: detecting duplicate wallet by comparing the db filename.

5912031 wallet: Create IsDatabaseLoaded function (Chun Kuan Lee)
15c93f0 wallet: Add trailing wallet.dat when detecting duplicate wallet if it's a directory. (Chun Kuan Lee)
c456fbd Refactor: Move m_db pointers into BerkeleyDatabase (Russell Yanofsky)

Pull request description:

  Fix #14538

  Fix crash attempting to load the same wallet with different path strings that resolve to the same absolute path. The primary check which prevents loading the same wallet twice is:

  https://github.com/bitcoin/bitcoin/blob/6b8d0a2164b30eab76e7bccb1ffb056a10fba406/src/wallet/db.cpp#L44

  But this check is skipped if both wallet paths resolve to the same absolute path, due to caching here:

  https://github.com/bitcoin/bitcoin/blob/6b8d0a2164b30eab76e7bccb1ffb056a10fba406/src/wallet/db.cpp#L467

  Meanwhile a secondary check for duplicate wallets is not reliable because it based on a literal comparison, instead of comparison using absolute paths:

  https://github.com/bitcoin/bitcoin/blob/6b8d0a2164b30eab76e7bccb1ffb056a10fba406/src/wallet/wallet.cpp#L3853

  This PR fixes the latter check to compare the absolute path of a new wallet being loaded to absolute paths of wallets already loaded, so there should no longer be any way to load the same wallet more than once.

Tree-SHA512: 2fa01811c160b57be3b76c6b4983556a04bbce71a3f8202429987ec020664a062e897deedcd9248bc04e9baaa2fc7b464e2595dcaeff2af0818387bf1fcdbf6f

Author: laanwj

src/wallet/db.cpp

@@ -56,9 +56,8 @@ bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId& rhs) const
     return memcmp(value, &rhs.value, sizeof(value)) == 0;
 }
 
-BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename)
+static void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::string& database_filename)
 {
-    fs::path env_directory;
     if (fs::is_regular_file(wallet_path)) {
         // Special case for backwards compatibility: if wallet path points to an
         // existing file, treat it as the path to a BDB data file in a parent
@@ -71,6 +70,23 @@ BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& data
         env_directory = wallet_path;
         database_filename = "wallet.dat";
     }
+}
+
+bool IsWalletLoaded(const fs::path& wallet_path)
+{
+    fs::path env_directory;
+    std::string database_filename;
+    SplitWalletPath(wallet_path, env_directory, database_filename);
+    LOCK(cs_db);
+    auto env = g_dbenvs.find(env_directory.string());
+    if (env == g_dbenvs.end()) return false;
+    return env->second.IsDatabaseLoaded(database_filename);
+}
+
+BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename)
+{
+    fs::path env_directory;
+    SplitWalletPath(wallet_path, env_directory, database_filename);
     LOCK(cs_db);
     // Note: An unused temporary BerkeleyEnvironment object may be created inside the
     // emplace function if the key already exists. This is a little inefficient,
@@ -90,13 +106,13 @@ void BerkeleyEnvironment::Close()
 
     fDbEnvInit = false;
 
-    for (auto& db : mapDb) {
+    for (auto& db : m_databases) {
         auto count = mapFileUseCount.find(db.first);
         assert(count == mapFileUseCount.end() || count->second == 0);
-        if (db.second) {
-            db.second->close(0);
-            delete db.second;
-            db.second = nullptr;
+        BerkeleyDatabase& database = db.second.get();
+        if (database.m_db) {
+            database.m_db->close(0);
+            database.m_db.reset();
         }
     }
 
@@ -463,7 +479,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
         if (!env->Open(false /* retry */))
             throw std::runtime_error("BerkeleyBatch: Failed to open database environment.");
 
-        pdb = env->mapDb[strFilename];
+        pdb = database.m_db.get();
         if (pdb == nullptr) {
             int ret;
             std::unique_ptr<Db> pdb_temp = MakeUnique<Db>(env->dbenv.get(), 0);
@@ -508,7 +524,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
             }
 
             pdb = pdb_temp.release();
-            env->mapDb[strFilename] = pdb;
+            database.m_db.reset(pdb);
 
             if (fCreate && !Exists(std::string("version"))) {
                 bool fTmp = fReadOnly;
@@ -563,12 +579,13 @@ void BerkeleyEnvironment::CloseDb(const std::string& strFile)
 {
     {
         LOCK(cs_db);
-        if (mapDb[strFile] != nullptr) {
+        auto it = m_databases.find(strFile);
+        assert(it != m_databases.end());
+        BerkeleyDatabase& database = it->second.get();
+        if (database.m_db) {
             // Close the database handle
-            Db* pdb = mapDb[strFile];
-            pdb->close(0);
-            delete pdb;
-            mapDb[strFile] = nullptr;
+            database.m_db->close(0);
+            database.m_db.reset();
         }
     }
 }
@@ -586,7 +603,7 @@ void BerkeleyEnvironment::ReloadDbEnv()
     });
 
     std::vector<std::string> filenames;
-    for (auto it : mapDb) {
+    for (auto it : m_databases) {
         filenames.push_back(it.first);
     }
     // Close the individual Db's

src/wallet/db.h

@@ -31,6 +31,8 @@ struct WalletDatabaseFileId {
     bool operator==(const WalletDatabaseFileId& rhs) const;
 };
 
+class BerkeleyDatabase;
+
 class BerkeleyEnvironment
 {
 private:
@@ -43,7 +45,7 @@ class BerkeleyEnvironment
 public:
     std::unique_ptr<DbEnv> dbenv;
     std::map<std::string, int> mapFileUseCount;
-    std::map<std::string, Db*> mapDb;
+    std::map<std::string, std::reference_wrapper<BerkeleyDatabase>> m_databases;
     std::unordered_map<std::string, WalletDatabaseFileId> m_fileids;
     std::condition_variable_any m_db_in_use;
 
@@ -54,6 +56,7 @@ class BerkeleyEnvironment
     void MakeMock();
     bool IsMock() const { return fMockDb; }
     bool IsInitialized() const { return fDbEnvInit; }
+    bool IsDatabaseLoaded(const std::string& db_filename) const { return m_databases.find(db_filename) != m_databases.end(); }
     fs::path Directory() const { return strPath; }
 
     /**
@@ -95,6 +98,9 @@ class BerkeleyEnvironment
     }
 };
 
+/** Return whether a wallet database is currently loaded. */
+bool IsWalletLoaded(const fs::path& wallet_path);
+
 /** Get BerkeleyEnvironment and database filename given a wallet path. */
 BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename);
 
@@ -115,13 +121,22 @@ class BerkeleyDatabase
         nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0)
     {
         env = GetWalletEnv(wallet_path, strFile);
+        auto inserted = env->m_databases.emplace(strFile, std::ref(*this));
+        assert(inserted.second);
         if (mock) {
             env->Close();
             env->Reset();
             env->MakeMock();
         }
     }
 
+    ~BerkeleyDatabase() {
+        if (env) {
+            size_t erased = env->m_databases.erase(strFile);
+            assert(erased == 1);
+        }
+    }
+
     /** Return object for accessing database at specified path. */
     static std::unique_ptr<BerkeleyDatabase> Create(const fs::path& path)
     {
@@ -161,6 +176,9 @@ class BerkeleyDatabase
     unsigned int nLastFlushed;
     int64_t nLastWalletUpdate;
 
+    /** Database pointer. This is initialized lazily and reset during flushes, so it can be null. */
+    std::unique_ptr<Db> m_db;
+
 private:
     /** BerkeleyDB specific */
     BerkeleyEnvironment *env;

src/wallet/wallet.cpp

@@ -3872,11 +3872,9 @@ bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, b
     }
 
     // Make sure that the wallet path doesn't clash with an existing wallet path
-    for (auto wallet : GetWallets()) {
-        if (wallet->GetLocation().GetPath() == wallet_path) {
-            error_string = strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", location.GetName());
-            return false;
-        }
+    if (IsWalletLoaded(wallet_path)) {
+        error_string = strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", location.GetName());
+        return false;
     }
 
     try {

test/functional/wallet_multiwallet.py

@@ -223,6 +223,9 @@ def wallet_file(name):
         # Fail to load duplicate wallets
         assert_raises_rpc_error(-4, 'Wallet file verification failed: Error loading wallet w1. Duplicate -wallet filename specified.', self.nodes[0].loadwallet, wallet_names[0])
 
+        # Fail to load duplicate wallets by different ways (directory and filepath)
+        assert_raises_rpc_error(-4, "Wallet file verification failed: Error loading wallet wallet.dat. Duplicate -wallet filename specified.", self.nodes[0].loadwallet, 'wallet.dat')
+
         # Fail to load if one wallet is a copy of another
         assert_raises_rpc_error(-1, "BerkeleyBatch: Can't open database w8_copy (duplicates fileid", self.nodes[0].loadwallet, 'w8_copy')