wallet: Remove trailing separators from -walletdir arg

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)

fanquake added the label Wallet on Sep 4, 2018

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

PierreRochard force-pushed on Sep 4, 2018

DrahtBot commented at 6:14 pm on September 4, 2018: member

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

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~~

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

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!

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\

0while (fs::detail::is_directory_separator(env_directory.string().back())) {
1    env_directory.remove_trailing_separator();
2}

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

Thank you, that’s much cleaner, fixed!

promag commented at 0:08 am on September 5, 2018: member

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

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.

 0get_fileid: file ID not set
 1Running 315 test cases...
 2unknown 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)
 3c:\projects\bitcoin\src\wallet\test\psbt_wallet_tests.cpp(18): last checkpoint: "psbt_updater_test" fixture ctor
 4get_fileid: file ID not set
 5unknown 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)
 6c:\projects\bitcoin\src\wallet\test\psbt_wallet_tests.cpp(74): last checkpoint: "parse_hd_keypath" fixture ctor
 7get_fileid: file ID not set
 8unknown location(0): fatal error: in "wallet_tests/ComputeTimeSmart": class std::runtime_error: BerkeleyBatch: Can't open database wallet.dat (get_fileid failed with 22)
 9c:\projects\bitcoin\src\wallet\test\wallet_tests.cpp(230): last checkpoint: "ComputeTimeSmart" fixture ctor
10get_fileid: file ID not set
11unknown location(0): fatal error: in "wallet_tests/LoadReceiveRequests": class std::runtime_error: BerkeleyBatch: Can't open database wallet.dat (get_fileid failed with 22)
12c:\projects\bitcoin\src\wallet\test\wallet_tests.cpp(256): last checkpoint: "LoadReceiveRequests" fixture ctor
13get_fileid: file ID not set
14unknown location(0): fatal error: in "wallet_tests/ListCoins": class std::runtime_error: BerkeleyBatch: Can't open database wallet.dat (get_fileid failed with 22)
15c:\projects\bitcoin\src\wallet\test\wallet_tests.cpp(317): last checkpoint: "ListCoins" fixture ctor
16*** 5 failures are detected in the test module "Bitcoin Test Suite"

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

PierreRochard force-pushed on Sep 6, 2018

PierreRochard renamed this:
~~wallet: Remove trailing separator for Berkeley db env directories~~
wallet: Remove trailing separators from -walletdir arg
on Sep 6, 2018

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

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:

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

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

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.

PierreRochard force-pushed on Sep 11, 2018

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.

PierreRochard force-pushed on Sep 11, 2018

laanwj deleted a comment on Sep 11, 2018

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

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

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

PierreRochard force-pushed on Sep 12, 2018

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

wallet: Add walletdir arg unit tests ea3009ee94

wallet: Remove trailing separators from -walletdir arg 2d471636eb

PierreRochard force-pushed on Sep 13, 2018

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

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?

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.

ken2812221 commented at 6:03 am on September 15, 2018: contributor

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

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

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

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:

0AssertionError: [node 0] Expected message "Error: Could not open debug log file \S+$" does not fully match stderr:
1""

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.

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.

laanwj commented at 8:58 am on October 18, 2018: member

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

laanwj merged this on Oct 18, 2018

laanwj closed this on Oct 18, 2018

laanwj referenced this in commit d98777f302 on Oct 18, 2018

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

PierreRochard deleted the branch on Oct 20, 2018

MarcoFalke referenced this in commit 4e6dc7a0ef on Nov 7, 2018

practicalswift deleted a comment on May 4, 2019

fanquake referenced this in commit 3562c15be3 on Oct 13, 2020

MarcoFalke referenced this in commit b9ac31f2d2 on Oct 13, 2020

fanquake referenced this in commit 39cc842f1f on Oct 14, 2020

fanquake referenced this in commit 71a9475957 on Oct 15, 2020

fanquake referenced this in commit 6957419a28 on Oct 16, 2020

Bushstar referenced this in commit f9244956af on Oct 21, 2020

kittywhiskers referenced this in commit 8342d085d5 on Aug 22, 2021

kittywhiskers referenced this in commit 3b87741cf7 on Aug 30, 2021

linuxsh2 referenced this in commit 2ac39c463d on Sep 16, 2021

PastaPastaPasta referenced this in commit ee29fd7954 on Sep 18, 2021

PastaPastaPasta referenced this in commit 590886f0cc on Sep 24, 2021

PastaPastaPasta referenced this in commit f2dce0fb91 on Sep 28, 2021

kittywhiskers referenced this in commit fdef6755b1 on Oct 8, 2021

kittywhiskers referenced this in commit 13da055f5c on Oct 12, 2021

kittywhiskers referenced this in commit 0d15947f85 on Oct 16, 2021

kittywhiskers referenced this in commit 76af0435ca on Oct 16, 2021

kittywhiskers referenced this in commit 98a62e27cf on Oct 19, 2021

vijaydasmp referenced this in commit 8159eb8096 on Oct 21, 2021

pravblockc referenced this in commit 34219d5926 on Nov 18, 2021

DrahtBot locked this on Dec 16, 2021

wallet: Remove trailing separators from -walletdir arg #14146