Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Nov 20, 2018

Conversation

@ken2812221
Copy link
Contributor

@ken2812221 ken2812221 commented Oct 23, 2018

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:

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:

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:

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 ken2812221 force-pushed the default-wallet-fix branch from 9ec1f04 to f245feb Oct 23, 2018
@ken2812221 ken2812221 force-pushed the default-wallet-fix branch from f245feb to 7b26c1f Oct 23, 2018
@ken2812221 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
@ken2812221 ken2812221 force-pushed the default-wallet-fix branch 2 times, most recently from 321d5aa to 8e95c92 Oct 23, 2018
@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Oct 23, 2018

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
Copy link
Contributor Author

@ken2812221 ken2812221 commented Oct 23, 2018

@ryanofsky The element in mapDb is not always exist.

For example:

  1. Load wallet dir/w1
  2. Load wallet dir/w2
  3. 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
Copy link
Contributor

@DrahtBot DrahtBot commented Oct 23, 2018

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
Copy link
Contributor

@ryanofsky ryanofsky commented Oct 24, 2018

@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.

@ken2812221 ken2812221 force-pushed the default-wallet-fix branch from 8e95c92 to aef19be Oct 25, 2018
Copy link
Contributor

@ryanofsky ryanofsky left a comment

utACK aef19be

@ken2812221 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 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
@ken2812221 ken2812221 force-pushed the default-wallet-fix branch 2 times, most recently from b2b5703 to b037553 Nov 5, 2018
@ken2812221
Copy link
Contributor Author

@ken2812221 ken2812221 commented Nov 5, 2018

Rebased

Copy link
Contributor

@ryanofsky ryanofsky left a comment

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:

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:

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:

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
Copy link
Contributor Author

@ken2812221 ken2812221 commented Nov 5, 2018

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

@@ -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();
Copy link
Member

@promag promag Nov 5, 2018

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.

Copy link
Contributor

@ryanofsky ryanofsky Nov 5, 2018

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.

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

~BerkeleyDatabase() {
Copy link
Member

@promag promag Nov 5, 2018

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

Copy link
Contributor Author

@ken2812221 ken2812221 Nov 5, 2018

final can only be added to virtual method.

Copy link
Member

@promag promag Nov 5, 2018

@ken2812221 I mean class BerkeleyDatabase final.

Copy link
Contributor

@ryanofsky ryanofsky Nov 5, 2018

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

Copy link
Member

@promag promag Nov 5, 2018

Nice thanks.

@@ -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)
Copy link
Member

@promag promag Nov 5, 2018

Could be static?

@@ -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();
Copy link
Member

@promag promag Nov 5, 2018

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

@ken2812221 ken2812221 force-pushed the default-wallet-fix branch 2 times, most recently from a949601 to 5fe35cf Nov 5, 2018
jonspock added a commit to jonspock/devault that referenced this issue Mar 24, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Apr 6, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Apr 8, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Apr 8, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Apr 8, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Apr 8, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Apr 9, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Apr 17, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue May 23, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue May 25, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Jul 9, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Jul 10, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Jul 17, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Jul 17, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Jul 20, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Jul 29, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Jul 31, 2020
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
jonspock added a commit to devaultcrypto/devault that referenced this issue Aug 5, 2020
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
jonspock added a commit to devaultcrypto/devault that referenced this issue Aug 6, 2020
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
jonspock added a commit to jonspock/devault that referenced this issue Aug 7, 2020
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
proteanx added a commit to devaultcrypto/devault that referenced this issue Aug 8, 2020
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
xdustinface added a commit to xdustinface/dash that referenced this issue Apr 4, 2021
…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
xdustinface added a commit to xdustinface/dash that referenced this issue Apr 4, 2021
…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
xdustinface added a commit to xdustinface/dash that referenced this issue Apr 4, 2021
…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
xdustinface added a commit to xdustinface/dash that referenced this issue Apr 4, 2021
…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
xdustinface added a commit to xdustinface/dash that referenced this issue Apr 5, 2021
…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
xdustinface added a commit to xdustinface/dash that referenced this issue Apr 13, 2021
…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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants