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

ken2812221 · 2018-10-23T07:59:52Z

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:

bitcoin/src/wallet/db.cpp

Line 44 in 6b8d0a2

    
           throw std::runtime_error(strprintf("BerkeleyBatch: Can't open database %s (duplicates fileid %s from %s)", filename,

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

bitcoin/src/wallet/db.cpp

Line 467 in 6b8d0a2

if (pdb == nullptr) {

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

bitcoin/src/wallet/wallet.cpp

Line 3853 in 6b8d0a2

    
           error_string = strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", location.GetName());

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.

ryanofsky · 2018-10-23T19:36:09Z

Would prefer to check this at database level, rather than in higher level wallet code. I think it's better if high level code just passes along wallet paths and isn't concerned with how data is stored in them.

I posted a tweaked version of this PR at 40a6475 which moves the check. @ken2812221, could you incorporate 40a6475 into this PR if you think it makes sense?

ken2812221 · 2018-10-23T20:34:11Z

@ryanofsky The element in mapDb is not always exist.

For example:

Load wallet dir/w1
Load wallet dir/w2
Unload wallet dir/w1

Now mapDb contains neither dir/w1 nor dir/w2 because it would flush all wallet files that share same BerkeleyEnvironment

DrahtBot · 2018-10-23T22:01:03Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#11911 (Free BerkeleyEnvironment instances when not in use by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ryanofsky · 2018-10-24T21:02:11Z

@ryanofsky The element in mapDb is not always exist.

Good catch. I think a small refactoring could fix this: 9c945d0. And then the fix would change slightly to aef19be.

I pushed a branch with these two commits to: https://github.com/ryanofsky/bitcoin/commits/pr/bloaded

I think this is a better than checking for wallet.dat files outside the database layer, but let me know what you think, and feel free to use the code or commits in this PR if you think this approach makes sense.

ryanofsky

utACK aef19be

ken2812221 · 2018-11-05T13:28:25Z

Rebased

ryanofsky

utACK b0375538375eb0c419b308d1015723ab64d53294. Only change since last review is rebase.

@promag, this might be an easy PR for to you review. First commit is just refactoring that doesn't change behavior. Second commit is the actual fix.

@ken2812221, it might help for this to have a more complete PR description.

Suggested PR description:

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:

bitcoin/src/wallet/db.cpp

Line 44 in 6b8d0a2

    
           throw std::runtime_error(strprintf("BerkeleyBatch: Can't open database %s (duplicates fileid %s from %s)", filename,

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

bitcoin/src/wallet/db.cpp

Line 467 in 6b8d0a2

if (pdb == nullptr) {

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

bitcoin/src/wallet/wallet.cpp

Line 3853 in 6b8d0a2

    
           error_string = strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", location.GetName());

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.

ken2812221 · 2018-11-05T15:51:10Z

@ryanofsky Thanks for your review. I would take your suggestion.

promag · 2018-11-05T15:47:40Z

src/wallet/db.cpp

@@ -463,7 +463,8 @@ 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];
+        BerkeleyDatabase& database = env->m_databases.at(strFilename).get();


Some comments here:

there's a slight change when replacing map::operator[] with map::at, maybe worth a comemnt or an assertion?

also, isn't this equivalent to the above database argument?

if this is really necessary then you could avoid shadowing database.

re: #14552 (comment)

Oh, I didn't even realize there was a database argument being shadowed above. Should just delete this line and use the existing database, which points to the same thing.

promag · 2018-11-05T15:49:20Z

src/wallet/db.h

        if (mock) {
            env->Close();
            env->Reset();
            env->MakeMock();
        }
    }

+    ~BerkeleyDatabase() {


nit, could add final to class since this is not virtual.

final can only be added to virtual method.

@ken2812221 I mean class BerkeleyDatabase final.

Change would be unrelated to this PR, and IMO, it's better only to use final when you have virtual methods and actual optimizations this would allow or bugs it would prevent. There's a core guideline that touches on this: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-final

Nice thanks.

promag · 2018-11-05T15:53:44Z

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)
+void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::string& database_filename)


Could be static?

promag · 2018-11-05T16:11:01Z

src/wallet/db.cpp

@@ -563,12 +581,11 @@ void BerkeleyEnvironment::CloseDb(const std::string& strFile)
 {
    {
        LOCK(cs_db);
-        if (mapDb[strFile] != nullptr) {
+        BerkeleyDatabase& database = m_databases.at(strFile).get();


Same as above, there's a slight change when replacing map::operator[] with map::at, maybe worth a comment or an assertion?

This is a refactoring change that doesn't affect behavior. The motivation behind the change is give BerkeleyEnvironment objects access to BerkeleyDatabase objects so it will be possible to simplify the duplicate wallet check and more reliably avoid opening the same databases twice.

…'s a directory.

promag · 2018-11-06T10:20:33Z

utACK 15c93f0.

laanwj · 2018-11-06T12:01:40Z

src/wallet/db.cpp

+    LOCK(cs_db);
+    auto env = g_dbenvs.find(env_directory.string());
+    if (env == g_dbenvs.end()) return false;
+    auto db = env->second.m_databases.find(database_filename);


I think this is leaking the implementation detail of m_databases, wouldn't checking whether a database exists ideally be a responsibility of the database environment. e.g. add a bool IsDatabaseLoaded(name) on DBenv?

Note that BerkeleyEnvironment::mapDb is currently used outside. I consider this a bugfix so any improvement/refactor could come next.

This is now resolved (by adding IsDatabaseLoaded)

ryanofsky · 2018-11-08T16:33:17Z

utACK 5912031. Just minor tweaks since last review: replacing map::at exception with assert, adding IsDatabaseLoaded.

@promag and @MarcoFalke, I've noticed that in your recent reviews you've been advocating replacing exceptions with asserts or return codes. Maybe one of you can write up some guidelines about when to use exceptions in the developer guide. Personally, I tend to like exceptions and think that whatever drawbacks they have will be fixed by new c++ features like throws/std::error in
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0709r0.pdf (see §4.1.1 "Elevator Pitch")

meshcollider · 2018-11-09T13:37:36Z

utACK 5912031, looks good

ryanofsky · 2018-11-15T17:35:41Z

Can this be merged? It fixes an ugly bug and has some refactoring changes I want to build on. If it can't be merged, maybe it could be added to https://github.com/bitcoin/bitcoin/projects/8

maflcko · 2018-11-15T18:21:13Z

Is this for backport?

Summary: ``` Fix crash attempting to load the same wallet with different path strings that resolve to the same absolute path. ``` More details here: bitcoin/bitcoin#14552 Backport of core PR14552: https://github.com/bitcoin/bitcoin/pull/14552/files Depends on D4885. Test Plan: ninja check ./test/functional/test_runner.py wallet_* Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D4886

…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 bitcoin#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

fanquake added the Wallet label Oct 23, 2018

ken2812221 changed the title ~~wallet: throw an error if user load the wallet file by different ways~~ wallet: Add trailing wallet.dat when detecting duplicate wallet if it's a directory. Oct 23, 2018

ryanofsky mentioned this pull request Oct 23, 2018

loadwallet command crashes bitcoind #14538

Closed

DrahtBot mentioned this pull request Oct 23, 2018

Add WalletLocation class #14350

Merged

ryanofsky approved these changes Oct 25, 2018

View reviewed changes

ryanofsky mentioned this pull request Oct 25, 2018

Crash with loading wallet async via RPC #14572

Closed

DrahtBot mentioned this pull request Oct 26, 2018

Free BerkeleyEnvironment instances when not in use #11911

Merged

ken2812221 changed the title ~~wallet: Add trailing wallet.dat when detecting duplicate wallet if it's a directory.~~ wallet: detecting duplicate wallet by comparing the db file. Oct 26, 2018

ken2812221 changed the title ~~wallet: detecting duplicate wallet by comparing the db file.~~ wallet: detecting duplicate wallet by comparing the db filename. Oct 30, 2018

DrahtBot added Needs rebase and removed Needs rebase labels Nov 5, 2018

promag mentioned this pull request Nov 5, 2018

bitcoind crashes when opening wallet.dat second time without multiwallet mode #14657

Closed

ryanofsky approved these changes Nov 5, 2018

View reviewed changes

promag reviewed Nov 5, 2018

View reviewed changes

ryanofsky and others added 2 commits November 6, 2018 08:28

wallet: Add trailing wallet.dat when detecting duplicate wallet if it…

15c93f0

…'s a directory.

laanwj reviewed Nov 6, 2018

View reviewed changes

wallet: Create IsDatabaseLoaded function

5912031

PastaPastaPasta mentioned this pull request May 9, 2020

Backport loadwallet dashpay/dash#3462

Merged

PastaPastaPasta mentioned this pull request Jun 11, 2020

RPC Loadwallet crash dashpay/dash#3521

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

ken2812221 commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Oct 23, 2018

Uh oh!

ken2812221 commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky commented Oct 24, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Nov 5, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Nov 5, 2018

Uh oh!

promag Nov 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Nov 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Nov 6, 2018

Uh oh!

laanwj Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 8, 2018

Uh oh!

meshcollider commented Nov 9, 2018

Uh oh!

ryanofsky commented Nov 15, 2018

Uh oh!

maflcko commented Nov 15, 2018

Uh oh!

Uh oh!

ken2812221 commented Oct 23, 2018 •

edited

Loading

ken2812221 commented Oct 23, 2018 •

edited

Loading

DrahtBot commented Oct 23, 2018 •

edited

Loading

promag Nov 5, 2018 •

edited

Loading

promag Nov 5, 2018 •

edited

Loading

laanwj Nov 6, 2018 •

edited

Loading

ryanofsky Nov 8, 2018 •

edited

Loading