wallet: Remove trailing separators from -walletdir arg #14146

pull PierreRochard wants to merge 2 commits into bitcoin:master from PierreRochard:wallet-env-bug changing 6 files +153 −4
  1. PierreRochard commented at 2:36 PM on September 4, 2018: contributor

    If a user passes in a path with a trailing separator as the walletdir, multiple BerkeleyEnvironments may be created in the same directory which can lead to data corruption.

    Discovered while reviewing #12493 (comment)

  2. fanquake added the label Wallet on Sep 4, 2018
  3. PierreRochard commented at 2:38 PM on September 4, 2018: contributor

    Would it be better if the test mocked the g_dbenvs in db.cpp? I wasn't able to figure out how to do that.

  4. PierreRochard force-pushed on Sep 4, 2018
  5. DrahtBot commented at 6:14 PM on September 4, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->Reviewers, this pull request conflicts with the following ones:

    • #11911 (Free BerkeleyEnvironment instances when not in use by ryanofsky)
    • #10973 (Refactor: separate wallet from node 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.

  6. PierreRochard commented at 9:30 PM on September 4, 2018: contributor

    I'm unable to reproduce the CI failures locally for debugging and the logs don't seem to provide enough information to pinpoint the problem

  7. in src/wallet/test/db_tests.cpp:31 in 7d641c6b81 outdated 
      26 | +    fs::path data_dir = SetDataDir("get_wallet_env");
  27 | +    fs::path wallet_dir = data_dir / "wallets";
  28 | +
  29 | +    std::vector<fs::path> wallet_test_paths;
  30 | +    std::string trailing_slashes;
  31 | +    for (int i = 0; i < 4; ++i ) {
    promag commented at 11:46 PM on September 4, 2018:

    nit, remove space after ++i.

    PierreRochard commented at 2:29 AM on September 6, 2018:

    Fixed

  8. in src/wallet/test/db_tests.cpp:19 in 7d641c6b81 outdated 
      14 | +/** If a user passes in a path with a trailing separator as the walletdir, multiple BerkeleyEnvironments
  15 | + * may be created in the same directory which can lead to data corruption.
  16 | + */
  17 | +BOOST_AUTO_TEST_CASE(get_wallet_env_trailing_slash)
  18 | +{
  19 | +    const char kPathSeparator =
    promag commented at 11:48 PM on September 4, 2018:

    Use boost::filesystem::path::preferred_separator?

    fanquake commented at 1:28 AM on September 5, 2018:

    @promag Not sure if you're aware, but anything boost::filesystem:: should use fs:: as is already being done here. See https://github.com/bitcoin/bitcoin/blob/master/src/fs.h.

    promag commented at 10:04 AM on September 5, 2018:

    Yes, just wanted give full type for reference.

    PierreRochard commented at 2:30 AM on September 6, 2018:

    Thank you, that's great, fixed!

  9. in src/wallet/db.cpp:77 in 7d641c6b81 outdated 
      70 | @@ -71,8 +71,13 @@ BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& data
  71 |          env_directory = wallet_path;
  72 |          database_filename = "wallet.dat";
  73 |      }
  74 | +    // Ensure that the directory does not end with a trailing separator to avoid
  75 | +    // creating two Berkeley environments in the same directory
  76 | +    while ((env_directory.string().back() == '/') || (env_directory.string().back() == '\\'))
  77 | +        env_directory = env_directory.remove_trailing_separator();
    ken2812221 commented at 11:55 PM on September 4, 2018:

    I would prefer the code down there. It might cause an infinity loop on Unix if the wallet filename is wallet\

    while (fs::detail::is_directory_separator(env_directory.string().back())) {
    env_directory.remove_trailing_separator();
}
    PierreRochard commented at 2:29 AM on September 6, 2018:

    Thank you, that's much cleaner, fixed!

  10. promag commented at 12:08 AM on September 5, 2018: member

    Move the path cleanup here? https://github.com/bitcoin/bitcoin/blob/c39fa34bfd90b1de0016ad45c207213bd03ecd75/src/wallet/walletutil.cpp#L12

  11. ken2812221 commented at 9:58 AM on September 5, 2018: contributor

    I'm unable to reproduce the CI failures locally for debugging and the logs don't seem to provide enough information to pinpoint the problem

    You can take a look at appveyor CI. That might be helpful.

    get_fileid: file ID not set
Running 315 test cases...
unknown location(0): fatal error: in "psbt_wallet_tests/psbt_updater_test": class std::runtime_error: BerkeleyBatch: Can't open database wallet.dat (get_fileid failed with 22)
c:\projects\bitcoin\src\wallet\test\psbt_wallet_tests.cpp(18): last checkpoint: "psbt_updater_test" fixture ctor
get_fileid: file ID not set
unknown location(0): fatal error: in "psbt_wallet_tests/parse_hd_keypath": class std::runtime_error: BerkeleyBatch: Can't open database wallet.dat (get_fileid failed with 22)
c:\projects\bitcoin\src\wallet\test\psbt_wallet_tests.cpp(74): last checkpoint: "parse_hd_keypath" fixture ctor
get_fileid: file ID not set
unknown location(0): fatal error: in "wallet_tests/ComputeTimeSmart": class std::runtime_error: BerkeleyBatch: Can't open database wallet.dat (get_fileid failed with 22)
c:\projects\bitcoin\src\wallet\test\wallet_tests.cpp(230): last checkpoint: "ComputeTimeSmart" fixture ctor
get_fileid: file ID not set
unknown location(0): fatal error: in "wallet_tests/LoadReceiveRequests": class std::runtime_error: BerkeleyBatch: Can't open database wallet.dat (get_fileid failed with 22)
c:\projects\bitcoin\src\wallet\test\wallet_tests.cpp(256): last checkpoint: "LoadReceiveRequests" fixture ctor
get_fileid: file ID not set
unknown location(0): fatal error: in "wallet_tests/ListCoins": class std::runtime_error: BerkeleyBatch: Can't open database wallet.dat (get_fileid failed with 22)
c:\projects\bitcoin\src\wallet\test\wallet_tests.cpp(317): last checkpoint: "ListCoins" fixture ctor
*** 5 failures are detected in the test module "Bitcoin Test Suite"
  12. fanquake renamed this:
    [wallet] Remove trailing separator for Berkeley db env directories
    wallet: Remove trailing separator for Berkeley db env directories
    on Sep 6, 2018
  13. PierreRochard force-pushed on Sep 6, 2018
  14. PierreRochard renamed this:
    wallet: Remove trailing separator for Berkeley db env directories
    wallet: Remove trailing separators from -walletdir arg
    on Sep 6, 2018
  15. PierreRochard commented at 2:32 AM on September 6, 2018: contributor

    @promag I moved the modification to walletutil.cpp, excellent suggestion, thank you!

    Force push diff: https://github.com/PierreRochard/bitcoin/commit/5a28a99d2887be85d02cb9a9a062f6bde96f56a2

  16. in src/wallet/walletutil.cpp:28 in d129dbab30 outdated 
      22 | @@ -23,5 +23,11 @@ fs::path GetWalletDir()
  23 |          }
  24 |      }
  25 |  
  26 | +    // Ensure that the directory does not end with a trailing separator to avoid
  27 | +    // creating two Berkeley environments in the same directory
  28 | +    while (fs::detail::is_directory_separator(path.string().back())) {
    promag commented at 2:33 PM on September 6, 2018:

    I think you could add above:

    if (gArgs.IsArgSet("-walletdir")) {
    path = gArgs.GetArg("-walletdir", "");
    boost::system::error_code error;
    path = fs::canonical(path, error);
    if (error || !fs::is_directory(path)) {
        ...

    fs::canonical checks if it exists and also cleans the path. For instance, locally calling canonical("/Users/promag/.///.//") gives /Users/promag.

    PierreRochard commented at 3:42 PM on September 6, 2018:

    Ah fs::canonical is even better! What do you think about me adding this to WalletInit::Verify here and then gArgs.ForceSetArg with the new clean path? That way we keep the error checking in one place and the path gets cleaned up once, rather than every time GetWalletDir is called.

    promag commented at 3:49 PM on September 6, 2018:

    Sounds good, you could replace fs::exists with fs::canonical in https://github.com/bitcoin/bitcoin/blob/465a583f9dcc2383eb20bbb98bffa66fe82f2cde/src/wallet/init.cpp#L182

    PierreRochard commented at 4:28 PM on September 6, 2018:

    The nuance with that approach is that we want to avoid transforming a relative file path to an absolute one, per @ryanofsky's commit message there:

    Specifying paths relative to the current working directory in a daemon process can be dangerous, because files can fail to be located even if the configuration doesn't change, but the daemon is started up differently.

    I'm thinking to first check wallet_dir.is_absolute() and then use fs::canonical

  17. in src/wallet/test/walletutil_tests.cpp:20 in d129dbab30 outdated 
      15 | + */
  16 | +BOOST_AUTO_TEST_CASE(get_wallet_dir_trailing_separators)
  17 | +{
  18 | +    fs::path expected_wallet_dir = SetDataDir("get_wallet_dir");
  19 | +    std::string trailing_separators;
  20 | +    for (int i = 0; i < 4; ++i) {
    promag commented at 2:35 PM on September 6, 2018:

    I would add a couple of cases instead.

  18. PierreRochard force-pushed on Sep 11, 2018
  19. PierreRochard commented at 1:33 AM on September 11, 2018: contributor

    Took up @promag's excellent suggestion to clean up the path with fs::canonical. Added unit tests for the walletdir interaction in wallet init. If there are other (cross-platform) test cases you would like to see, I'm happy to add them. The commits have completely changed, no value in a force push diff.

  20. PierreRochard force-pushed on Sep 11, 2018
  21. laanwj deleted a comment on Sep 11, 2018
  22. in src/wallet/test/init_tests.cpp:11 in ab3bf76ac6 outdated 
       6 | +
   7 | +#include <test/test_bitcoin.h>
   8 | +#include <wallet/test/init_test_fixture.h>
   9 | +
  10 | +#include <wallet/wallet.h>
  11 | +#include <wallet/init.cpp>
    ken2812221 commented at 12:35 PM on September 11, 2018:

    Why this include cpp file?

    PierreRochard commented at 7:12 PM on September 11, 2018:

    The WalletInit class was moved from the header file to the cpp file in this pull request #12836

    Should I move it back or is there a bigger picture architectural issue that I'm not seeing?

    promag commented at 7:15 PM on September 11, 2018:

    Include src/walletinitinterface.h?

    PierreRochard commented at 7:44 PM on September 11, 2018:

    🤔 how would I create an instance of the WalletInit class to test its implementation of Verify?

    (I'll take "read this book / article" as an answer if I'm out of my depth here haha)

    ken2812221 commented at 7:50 PM on September 11, 2018:

    The program will find the implementation by C++ virtual method table?

    promag commented at 7:59 PM on September 11, 2018:

    There is already an instance. Just replace the include.

    PierreRochard commented at 2:01 AM on September 12, 2018:

    Ah ok, fixed, I added init.h as well as it has the global instance in it

  23. PierreRochard commented at 7:19 PM on September 11, 2018: contributor

    Appveyor is failing, I rebooted into my Windows environment and see a memory leak is being detected, I'm debugging it now.

  24. in src/wallet/test/init_test_fixture.cpp:26 in ab3bf76ac6 outdated 
      20 | +    m_walletdir_path_cases["file"] = m_datadir / "not_a_directory.dat";
  21 | +    m_walletdir_path_cases["relative"] = "wallets";
  22 | +    m_walletdir_path_cases["trailing"] = m_datadir / "wallets" / sep;
  23 | +    m_walletdir_path_cases["trailing2"] = m_datadir / "wallets" / sep / sep;
  24 | +
  25 | +
    promag commented at 7:19 PM on September 11, 2018:

    Remove 2nd empty line.

    PierreRochard commented at 2:01 AM on September 12, 2018:

    Fixed

  25. PierreRochard force-pushed on Sep 12, 2018
  26. PierreRochard force-pushed on Sep 12, 2018
  27. PierreRochard commented at 2:05 AM on September 12, 2018: contributor

    I replaced the wallet/init.cpp include and removed extraneous whitespace, force push diff here https://github.com/bitcoin/bitcoin/commit/c8b2076d6206ac8fe349c08a97611189d2e729a8 I'm still working on the Windows memory leak issue.

  28. wallet: Add walletdir arg unit tests ea3009ee94
  29. wallet: Remove trailing separators from -walletdir arg 2d471636eb
  30. PierreRochard force-pushed on Sep 13, 2018
  31. PierreRochard commented at 1:12 AM on September 13, 2018: contributor

    I solved the problem on my local Windows machine, the fixture destructor was removing the current working directory - apparently this is ok on posix but not on Windows, force push diff: https://github.com/bitcoin/bitcoin/commit/6e2616deffd815051c1ac2e826bd55850e2898f9

  32. ken2812221 commented at 6:06 PM on September 13, 2018: contributor

    The unit tests is spamming error messages while running test_bitcoin. Would it confuse the user?

  33. PierreRochard commented at 6:49 PM on September 13, 2018: contributor

    The unit tests is spamming error messages while running test_bitcoin. Would it confuse the user?

    It definitely could, I've actually been researching how to properly mock CClientUIInterface. A shortcut would be to create a unit-test version of noui_connect but I was struggling on how to capture the output to check it in the unit test. If you have ideas I'm interested, whether they fit into the scope of this PR or not, as I'm sure this isn't the last time I'll be hitting InitMessage in a unit test.

  34. ken2812221 commented at 6:03 AM on September 15, 2018: contributor

    @PierreRochard Haven't tested, but can we use gArgs.SoftSetBoolArg("-printtoconsole", false);?

  35. PierreRochard commented at 11:01 PM on September 15, 2018: contributor

    Unfortunately that didn't work. In noui.cpp the noui_ThreadSafeMessageBox function uses fprintf for output

  36. ken2812221 commented at 4:48 AM on September 16, 2018: contributor

    IMO you could just replace it with LogPrintf. Maybe ask for other dev's opinion

  37. PierreRochard commented at 11:42 AM on September 16, 2018: contributor

    I just looked into opening a PR for a simple replace of fprintf with LogPrintf but I think it would have to be a larger refactoring, as stderr output is necessary in some cases, for example when the debug log file can't be opened:

    AssertionError: [node 0] Expected message "Error: Could not open debug log file \S+$" does not fully match stderr:
""
  38. in src/wallet/test/init_tests.cpp:12 in 2d471636eb 
       7 | +#include <test/test_bitcoin.h>
   8 | +#include <wallet/test/init_test_fixture.h>
   9 | +
  10 | +#include <init.h>
  11 | +#include <walletinitinterface.h>
  12 | +#include <wallet/wallet.h>
    practicalswift commented at 7:51 AM on September 23, 2018:

    Please sort includes and make the grouping consistent to how it is done in other files.

  39. in src/wallet/test/init_test_fixture.h:10 in 2d471636eb 
       5 | +#ifndef BITCOIN_WALLET_TEST_INIT_TEST_FIXTURE_H
   6 | +#define BITCOIN_WALLET_TEST_INIT_TEST_FIXTURE_H
   7 | +
   8 | +#include <test/test_bitcoin.h>
   9 | +
  10 | +
    practicalswift commented at 7:51 AM on September 23, 2018:

    Nit: Extra newline.

  40. laanwj commented at 8:58 AM on October 18, 2018: member

    utACK 2d471636eb9160ab51b08e491e3f003f57adbc36 thanks for finding and addressing this bug (and adding tests too !)

  41. laanwj merged this on Oct 18, 2018
  42. laanwj closed this on Oct 18, 2018
  43. laanwj referenced this in commit d98777f302 on Oct 18, 2018
  44. ryanofsky commented at 4:03 PM on October 18, 2018: member

    utACK 2d471636eb9160ab51b08e491e3f003f57adbc36. This fix looks like it does the right thing, but it seems like it would be cleaner and more obvious to do the canonicalization in the GetWalletDir() function rather than WalletInit::Verify().

    In particular I don't like how GetWalletDir() can return the wrong result if it is called before WalletInit::Verify() and the right result after. Ideally GetWalletDir() would just return the same, canonical result all the time, and I'm curious if canonicalizing in GetWalletDir() wouldn't work for some reason.

    Another more minor issue predating this PR is that WalletInit::Verify() does the is_absolute check after checking whether the directory exists, and so might access the filesystem with a path relative to the working directory. Since bitcoind is a daemon, it's better if it can be agnostic to the working directory, and only access the filesystem with absolute paths (AbsPathForConfigVal util function is handy for this).

  45. PierreRochard deleted the branch on Oct 20, 2018
  46. MarcoFalke referenced this in commit 4e6dc7a0ef on Nov 7, 2018
  47. practicalswift deleted a comment on May 4, 2019
  48. fanquake referenced this in commit 3562c15be3 on Oct 13, 2020
  49. MarcoFalke referenced this in commit b9ac31f2d2 on Oct 13, 2020
  50. fanquake referenced this in commit 39cc842f1f on Oct 14, 2020
  51. fanquake referenced this in commit 71a9475957 on Oct 15, 2020
  52. fanquake referenced this in commit 6957419a28 on Oct 16, 2020
  53. Bushstar referenced this in commit f9244956af on Oct 21, 2020
  54. kittywhiskers referenced this in commit 8342d085d5 on Aug 22, 2021
  55. kittywhiskers referenced this in commit 3b87741cf7 on Aug 30, 2021
  56. linuxsh2 referenced this in commit 2ac39c463d on Sep 16, 2021
  57. PastaPastaPasta referenced this in commit ee29fd7954 on Sep 18, 2021
  58. PastaPastaPasta referenced this in commit 590886f0cc on Sep 24, 2021
  59. PastaPastaPasta referenced this in commit f2dce0fb91 on Sep 28, 2021
  60. kittywhiskers referenced this in commit fdef6755b1 on Oct 8, 2021
  61. kittywhiskers referenced this in commit 13da055f5c on Oct 12, 2021
  62. kittywhiskers referenced this in commit 0d15947f85 on Oct 16, 2021
  63. kittywhiskers referenced this in commit 76af0435ca on Oct 16, 2021
  64. kittywhiskers referenced this in commit 98a62e27cf on Oct 19, 2021
  65. vijaydasmp referenced this in commit 8159eb8096 on Oct 21, 2021
  66. pravblockc referenced this in commit 34219d5926 on Nov 18, 2021
  67. DrahtBot locked this on Dec 16, 2021
Contributors
PierreRochardDrahtBotpromagfanquakeken2812221practicalswiftlaanwjryanofsky
Labels
Wallet
Linked (view graph)
#12493 [wallet] Reopen CDBEnv after encryption instead of shutting down#14665 appveyor: Script improvement part II#20142 [0.20] build: set minimum required Boost to 1.48.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: 2026-04-27 09:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me