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

pull ken2812221 wants to merge 3 commits into bitcoin:master from ken2812221:default-wallet-fix changing 4 files +57 −21
  1. ken2812221 commented at 7:59 am on October 23, 2018: contributor

    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.

  2. ken2812221 force-pushed on Oct 23, 2018
  3. fanquake added the label Wallet on Oct 23, 2018
  4. ken2812221 force-pushed on Oct 23, 2018
  5. ken2812221 renamed this:
    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.
    on Oct 23, 2018
  6. ken2812221 force-pushed on Oct 23, 2018
  7. ken2812221 force-pushed on Oct 23, 2018
  8. ryanofsky commented at 7:36 pm on October 23, 2018: member

    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 40a6475e59950838be532b0a2c58f4359eae5625 which moves the check. @ken2812221, could you incorporate 40a6475e59950838be532b0a2c58f4359eae5625 into this PR if you think it makes sense?

  9. ken2812221 commented at 8:34 pm on October 23, 2018: contributor

    @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

  10. DrahtBot commented at 10:01 pm on October 23, 2018: member

    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.

  11. ryanofsky commented at 9:02 pm on October 24, 2018: member

    @ryanofsky The element in mapDb is not always exist.

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

    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.

  12. ken2812221 force-pushed on Oct 25, 2018
  13. ryanofsky approved
  14. ryanofsky commented at 2:27 pm on October 25, 2018: member
    utACK aef19be5851ceb3b324fd80756060a00aad8e61c
  15. ken2812221 renamed this:
    wallet: Add trailing wallet.dat when detecting duplicate wallet if it's a directory.
    wallet: detecting duplicate wallet by comparing the db file.
    on Oct 26, 2018
  16. ken2812221 renamed this:
    wallet: detecting duplicate wallet by comparing the db file.
    wallet: detecting duplicate wallet by comparing the db filename.
    on Oct 30, 2018
  17. DrahtBot added the label Needs rebase on Nov 5, 2018
  18. ken2812221 force-pushed on Nov 5, 2018
  19. ken2812221 force-pushed on Nov 5, 2018
  20. DrahtBot removed the label Needs rebase on Nov 5, 2018
  21. ken2812221 commented at 1:28 pm on November 5, 2018: contributor
    Rebased
  22. ryanofsky approved
  23. ryanofsky commented at 3:32 pm on November 5, 2018: member

    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:

    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.

  24. in src/wallet/db.cpp:466 in 1a2b922366 outdated 
    462@@ -463,7 +463,8 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
463         if (!env->Open(false /* retry */))
464             throw std::runtime_error("BerkeleyBatch: Failed to open database environment.");
465 
466-        pdb = env->mapDb[strFilename];
467+        BerkeleyDatabase& database = env->m_databases.at(strFilename).get();
    promag commented at 3:47 pm on November 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.
    ryanofsky commented at 4:06 pm on November 5, 2018:

    re: #14552 (review)

    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.

  25. in src/wallet/db.h:132 in 1a2b922366 outdated 
    124             env->Reset();
125             env->MakeMock();
126         }
127     }
128 
129+    ~BerkeleyDatabase() {
    promag commented at 3:49 pm on November 5, 2018:
    nit, could add final to class since this is not virtual.
    ken2812221 commented at 4:46 pm on November 5, 2018:
    final can only be added to virtual method.
    promag commented at 4:48 pm on November 5, 2018:
    @ken2812221 I mean class BerkeleyDatabase final.
    ryanofsky commented at 5:23 pm on November 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
    promag commented at 5:30 pm on November 5, 2018:
    Nice thanks.
  26. ken2812221 commented at 3:51 pm on November 5, 2018: contributor
    @ryanofsky Thanks for your review. I would take your suggestion.
  27. in src/wallet/db.cpp:59 in b037553837 outdated 
    55@@ -56,9 +56,8 @@ bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId& rhs) const
56     return memcmp(value, &rhs.value, sizeof(value)) == 0;
57 }
58 
59-BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename)
60+void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::string& database_filename)
    promag commented at 3:53 pm on November 5, 2018:
    Could be static?
  28. in src/wallet/db.cpp:584 in b037553837 outdated 
    580@@ -563,12 +581,11 @@ void BerkeleyEnvironment::CloseDb(const std::string& strFile)
581 {
582     {
583         LOCK(cs_db);
584-        if (mapDb[strFile] != nullptr) {
585+        BerkeleyDatabase& database = m_databases.at(strFile).get();
    promag commented at 4:11 pm on November 5, 2018:
    Same as above, there’s a slight change when replacing map::operator[] with map::at, maybe worth a comment or an assertion?
  29. ken2812221 force-pushed on Nov 5, 2018
  30. ken2812221 force-pushed on Nov 5, 2018
  31. ken2812221 force-pushed on Nov 5, 2018
  32. ken2812221 force-pushed on Nov 5, 2018
  33. ken2812221 force-pushed on Nov 5, 2018
  34. Refactor: Move m_db pointers into BerkeleyDatabase 
    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.
    c456fbd8df
  35. wallet: Add trailing wallet.dat when detecting duplicate wallet if it's a directory. 15c93f075a
  36. ken2812221 force-pushed on Nov 6, 2018
  37. promag commented at 10:20 am on November 6, 2018: member
    utACK 15c93f0.
  38. wallet: Create IsDatabaseLoaded function 591203149f
  39. in src/wallet/db.cpp:83 in 15c93f075a outdated 
    78+    std::string database_filename;
79+    SplitWalletPath(wallet_path, env_directory, database_filename);
80+    LOCK(cs_db);
81+    auto env = g_dbenvs.find(env_directory.string());
82+    if (env == g_dbenvs.end()) return false;
83+    auto db = env->second.m_databases.find(database_filename);
    laanwj commented at 12:01 pm on November 6, 2018:
    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?
    promag commented at 12:15 pm on November 6, 2018:
    Note that BerkeleyEnvironment::mapDb is currently used outside. I consider this a bugfix so any improvement/refactor could come next.
    ryanofsky commented at 4:34 pm on November 8, 2018:
    This is now resolved (by adding IsDatabaseLoaded)
  40. ken2812221 force-pushed on Nov 8, 2018
  41. ryanofsky commented at 4:33 pm on November 8, 2018: member
    utACK 591203149f1700f594f781862e88cbbfe83d8d37. 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”)
  42. meshcollider commented at 1:37 pm on November 9, 2018: contributor
    utACK https://github.com/bitcoin/bitcoin/pull/14552/commits/591203149f1700f594f781862e88cbbfe83d8d37, looks good
  43. ryanofsky commented at 5:35 pm on November 15, 2018: member
    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
  44. MarcoFalke commented at 6:21 pm on November 15, 2018: member
    Is this for backport?
  45. MarcoFalke added this to the milestone 0.18.0 on Nov 15, 2018
  46. ryanofsky commented at 6:28 pm on November 15, 2018: member

    Is this for backport?

    I don’t have a great sense for which changes get backported and which changes don’t. I would guess it could be backported, but I don’t think it’s important to backport. Original bug #14538 reporter @chernyshev might have an opinion.

    Note: related fix #14320 (comment) does not appear to have been backported.

  47. laanwj added this to the "Blockers" column in a project
  48. in src/wallet/db.h:48 in 591203149f 
    44@@ -43,7 +45,7 @@ class BerkeleyEnvironment
45 public:
46     std::unique_ptr<DbEnv> dbenv;
47     std::map<std::string, int> mapFileUseCount;
48-    std::map<std::string, Db*> mapDb;
49+    std::map<std::string, std::reference_wrapper<BerkeleyDatabase>> m_databases;
    promag commented at 10:51 pm on November 19, 2018:
    Shouldn’t this be guarded by cs_db?
  49. laanwj merged this on Nov 20, 2018
  50. laanwj closed this on Nov 20, 2018
  51. laanwj referenced this in commit afa506f6eb on Nov 20, 2018
  52. ken2812221 deleted the branch on Nov 20, 2018
  53. fanquake removed this from the "Blockers" column in a project
  54. promag referenced this in commit 94e4430ebc on Mar 11, 2019
  55. promag referenced this in commit c8459462ce on Mar 11, 2019
  56. promag referenced this in commit 3308fb79f2 on Mar 11, 2019
  57. promag referenced this in commit caf1146b13 on Mar 11, 2019
  58. promag referenced this in commit 7751ea37b6 on Mar 11, 2019
  59. promag referenced this in commit 0a9af2d4cb on Mar 11, 2019
  60. laanwj referenced this in commit 6cf81b01b4 on Mar 20, 2019
  61. sidhujag referenced this in commit a2169d653d on Mar 28, 2019
  62. sidhujag referenced this in commit 2324af59de on Mar 28, 2019
  63. sidhujag referenced this in commit 0a1ff1dcd5 on Mar 28, 2019
  64. sidhujag referenced this in commit 3ee64a0aa7 on Mar 28, 2019
  65. uhliksk referenced this in commit f067ffa584 on Apr 21, 2019
  66. uhliksk referenced this in commit 16e5759069 on Apr 21, 2019
  67. uhliksk referenced this in commit 0038ce4232 on Apr 21, 2019
  68. Rishabh42 referenced this in commit ce3547fae6 on Apr 22, 2019
  69. uhliksk referenced this in commit 0c39627aa7 on May 1, 2019
  70. uhliksk referenced this in commit ccdb5ac356 on May 1, 2019
  71. uhliksk referenced this in commit 692fc5366f on May 1, 2019
  72. jasonbcox referenced this in commit 8915b2f748 on Oct 25, 2019
  73. deadalnix referenced this in commit bc0f717820 on Jan 10, 2020
  74. jonspock referenced this in commit 79c9435dfa on Feb 23, 2020
  75. jonspock referenced this in commit c0594047a9 on Mar 2, 2020
  76. jonspock referenced this in commit b39c41e807 on Mar 7, 2020
  77. jonspock referenced this in commit 4415e90a96 on Mar 14, 2020
  78. jonspock referenced this in commit 6113ba41a1 on Mar 21, 2020
  79. jonspock referenced this in commit f03d07c7b5 on Mar 24, 2020
  80. jonspock referenced this in commit 0b61067c21 on Apr 6, 2020
  81. jonspock referenced this in commit 08aa86239d on Apr 8, 2020
  82. jonspock referenced this in commit ac008af4a7 on Apr 8, 2020
  83. jonspock referenced this in commit 3a119eb80f on Apr 8, 2020
  84. jonspock referenced this in commit cae266a80d on Apr 8, 2020
  85. jonspock referenced this in commit 7886a79025 on Apr 9, 2020
  86. jonspock referenced this in commit e158cece1d on Apr 17, 2020
  87. jonspock referenced this in commit 56b889829b on May 23, 2020
  88. jonspock referenced this in commit c512167a2d on May 25, 2020
  89. jonspock referenced this in commit f3db79f0f4 on Jul 9, 2020
  90. jonspock referenced this in commit 5ca14be4f7 on Jul 10, 2020
  91. jonspock referenced this in commit 7de609164b on Jul 17, 2020
  92. jonspock referenced this in commit cfec14932d on Jul 17, 2020
  93. jonspock referenced this in commit 0bc63ca2a3 on Jul 20, 2020
  94. jonspock referenced this in commit 7bbdec19fe on Jul 29, 2020
  95. jonspock referenced this in commit cb0be568b0 on Jul 31, 2020
  96. jonspock referenced this in commit dcc91cd30a on Aug 5, 2020
  97. jonspock referenced this in commit 5ec7ea0b1c on Aug 6, 2020
  98. jonspock referenced this in commit 7455175d48 on Aug 7, 2020
  99. proteanx referenced this in commit 52e2850eb1 on Aug 8, 2020
  100. xdustinface referenced this in commit 3ab52117af on Apr 4, 2021
  101. xdustinface referenced this in commit 96cec4daac on Apr 4, 2021
  102. xdustinface referenced this in commit 39287dff5b on Apr 4, 2021
  103. xdustinface referenced this in commit 5b1213bf66 on Apr 4, 2021
  104. xdustinface referenced this in commit 5db88d27cb on Apr 5, 2021
  105. xdustinface referenced this in commit 6246ba9181 on Apr 13, 2021
  106. MarcoFalke locked this on Sep 8, 2021


ken2812221 ryanofsky DrahtBot promag laanwj meshcollider MarcoFalke

Labels
Wallet

Milestone
0.18.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me