wallet: Remove trailing separators from -walletdir arg #14146

PierreRochard · 2018-09-04T14:36:39Z

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)

PierreRochard · 2018-09-04T14:38:48Z

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

DrahtBot · 2018-09-04T18:14:50Z

Reviewers, this pull request conflicts with the following ones:

Free BerkeleyEnvironment instances when not in use #11911 (Free BerkeleyEnvironment instances when not in use by ryanofsky)
Refactor: separate wallet from node #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 · 2018-09-04T21:30:19Z

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

ken2812221 · 2018-09-04T23:55:46Z

src/wallet/db.cpp

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(); }

Thank you, that's much cleaner, fixed!

promag

Move the path cleanup here?

bitcoin/src/wallet/walletutil.cpp

Line 12 in c39fa34

path = gArgs.GetArg("-walletdir", "");

promag · 2018-09-04T23:46:18Z

src/wallet/test/db_tests.cpp

nit, remove space after ++i.

promag · 2018-09-04T23:48:08Z

src/wallet/test/db_tests.cpp

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

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

Yes, just wanted give full type for reference.

Thank you, that's great, fixed!

ken2812221 · 2018-09-05T09:58:39Z

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"

PierreRochard · 2018-09-06T02:32:24Z

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

Force push diff: PierreRochard@5a28a99

promag · 2018-09-06T14:33:42Z

src/wallet/walletutil.cpp


+    // Ensure that the directory does not end with a trailing separator to avoid
+    // creating two Berkeley environments in the same directory
+    while (fs::detail::is_directory_separator(path.string().back())) {


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.

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.

Sounds good, you could replace fs::exists with fs::canonical in

bitcoin/src/wallet/init.cpp

Line 182 in 465a583

if (!fs::exists(wallet_dir)) {

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

promag · 2018-09-06T14:35:29Z

src/wallet/test/walletutil_tests.cpp

+{
+    fs::path expected_wallet_dir = SetDataDir("get_wallet_dir");
+    std::string trailing_separators;
+    for (int i = 0; i < 4; ++i) {


I would add a couple of cases instead.

PierreRochard · 2018-09-11T01:33:13Z

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.

ken2812221 · 2018-09-11T12:35:51Z

src/wallet/test/init_tests.cpp

+#include <wallet/test/init_test_fixture.h>
+
+#include <wallet/wallet.h>
+#include <wallet/init.cpp>


Why this include cpp file?

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?

Include src/walletinitinterface.h?

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

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

There is already an instance. Just replace the include.

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

PierreRochard · 2018-09-11T19:19:08Z

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

promag · 2018-09-11T19:19:16Z

src/wallet/test/init_test_fixture.cpp

+    m_walletdir_path_cases["trailing"] = m_datadir / "wallets" / sep;
+    m_walletdir_path_cases["trailing2"] = m_datadir / "wallets" / sep / sep;
+
+


Remove 2nd empty line.

promag · 2018-09-11T19:59:41Z

src/wallet/test/init_tests.cpp

+#include <wallet/test/init_test_fixture.h>
+
+#include <wallet/wallet.h>
+#include <wallet/init.cpp>


There is already an instance. Just replace the include.

PierreRochard · 2018-09-12T02:05:17Z

I replaced the wallet/init.cpp include and removed extraneous whitespace, force push diff here c8b2076
~~I'm still working on the Windows memory leak issue.~~

PierreRochard · 2018-09-13T01:12:15Z

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: 6e2616d

ken2812221 · 2018-09-13T18:06:31Z

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

PierreRochard · 2018-09-13T18:49:44Z

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 · 2018-09-15T06:03:47Z

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

2d47163 wallet: Remove trailing separators from -walletdir arg (Pierre Rochard) ea3009e wallet: Add walletdir arg unit tests (Pierre Rochard) Pull request description: 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) Tree-SHA512: f2bbf1749d904fd3f326b88f2ead58c8386034355910906d7faea155d518642e9cd4ceb3cae272f2d9d8feb61f126523e1c97502799d24e4315bb53e49fd7c09

ryanofsky

utACK 2d47163. 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).

99d33a6 appveyor: Script improvement part II (Chun Kuan Lee) Pull request description: - decrease clone depth to 5 - Upgrade to python 3.7 that we can use `PYTHONUTF8` from PEP540. - Set clcache version to `v4.2.0` - Do not fetch the latest vcpkg package (The issue does not exist anymore) - Set test_bitcoin report sink and log sink to stdout and redirect stderr to NUL to drop confusing error messages that introduced by bitcoin#14146 - discard vcpkg, bench_bitcoin output - Set functional test `--failfast` flag - Make the log be as clear as possible. (Only ~100 lines) Tree-SHA512: e7e1f5c2698e8a5d15394edfb4b574508081e99ef4a353995f55657cb51e642567a128d6432a899ecae6f742494c143ac16e2e64df6c26e1e575421ee4a1df50

Due to the use of boost::filesystem::canonical(), the minimum required version of Boost is actually 1.48.0. Use of canonical was introduced in bitcoin#14146. See also Boost filesystem 1.48.0 release notes: https://github.com/boostorg/filesystem/blob/6b5e38134a336b6ea777cd8a69b6ae929819db7e/doc/release_history.html#L508

3562c15 build: set minimum required Boost to 1.48.0 (fanquake) Pull request description: Due to the use of [`boost::filesystem::canonical()`](https://github.com/bitcoin/bitcoin/blob/80aa83aa406447d9b0932301b37966a30d0e1b6e/src/wallet/load.cpp#L21), the minimum required version of Boost is actually 1.48.0. Use of canonical was introduced in #14146. See also [Boost filesystem 1.48.0 release notes](https://github.com/boostorg/filesystem/blob/6b5e38134a336b6ea777cd8a69b6ae929819db7e/doc/release_history.html#L508). Also discussed in #20080. ACKs for top commit: practicalswift: ACK 3562c15: correct is better than incorrect :) hebasto: ACK 3562c15, this is the status quo. Tree-SHA512: 1d2226c60accb8e2276e023120a72f070392a6c1d3db97fb23e7759c174984226f81fed6d94f3203ef663fb4b3648e65960aaf15ed718895b6673e3ebeb082bd

Due to the use of boost::filesystem::canonical(), the minimum required version of Boost is actually 1.48.0. Use of canonical was introduced in bitcoin#14146. See also Boost filesystem 1.48.0 release notes: https://github.com/boostorg/filesystem/blob/6b5e38134a336b6ea777cd8a69b6ae929819db7e/doc/release_history.html#L508 Github-Pull: bitcoin#20142 Rebased-From: 3562c15

Due to the use of boost::filesystem::canonical(), the minimum required version of Boost is actually 1.48.0. Use of canonical was introduced in bitcoin#14146. See also Boost filesystem 1.48.0 release notes: https://github.com/boostorg/filesystem/blob/6b5e38134a336b6ea777cd8a69b6ae929819db7e/doc/release_history.html#L508

99d33a6 appveyor: Script improvement part II (Chun Kuan Lee) Pull request description: - decrease clone depth to 5 - Upgrade to python 3.7 that we can use `PYTHONUTF8` from PEP540. - Set clcache version to `v4.2.0` - Do not fetch the latest vcpkg package (The issue does not exist anymore) - Set test_bitcoin report sink and log sink to stdout and redirect stderr to NUL to drop confusing error messages that introduced by bitcoin#14146 - discard vcpkg, bench_bitcoin output - Set functional test `--failfast` flag - Make the log be as clear as possible. (Only ~100 lines) Tree-SHA512: e7e1f5c2698e8a5d15394edfb4b574508081e99ef4a353995f55657cb51e642567a128d6432a899ecae6f742494c143ac16e2e64df6c26e1e575421ee4a1df50

…ir arg 2d47163 wallet: Remove trailing separators from -walletdir arg (Pierre Rochard) ea3009e wallet: Add walletdir arg unit tests (Pierre Rochard) Pull request description: 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 bitcoin#12493 (comment) Tree-SHA512: f2bbf1749d904fd3f326b88f2ead58c8386034355910906d7faea155d518642e9cd4ceb3cae272f2d9d8feb61f126523e1c97502799d24e4315bb53e49fd7c09

fanquake added the Wallet label Sep 4, 2018

PierreRochard force-pushed the wallet-env-bug branch from be3ebab to 7d641c6 Compare September 4, 2018 17:32

ken2812221 reviewed Sep 4, 2018

View reviewed changes

promag reviewed Sep 5, 2018

View reviewed changes

fanquake changed the title ~~[wallet] Remove trailing separator for Berkeley db env directories~~ wallet: Remove trailing separator for Berkeley db env directories Sep 6, 2018

PierreRochard force-pushed the wallet-env-bug branch from 7d641c6 to d129dba Compare September 6, 2018 02:28

PierreRochard changed the title ~~wallet: Remove trailing separator for Berkeley db env directories~~ wallet: Remove trailing separators from -walletdir arg Sep 6, 2018

promag reviewed Sep 6, 2018

View reviewed changes

PierreRochard force-pushed the wallet-env-bug branch from d129dba to a07e9e5 Compare September 11, 2018 01:28

PierreRochard force-pushed the wallet-env-bug branch from a07e9e5 to ab3bf76 Compare September 11, 2018 02:17

bitcoin deleted a comment Sep 11, 2018

ken2812221 reviewed Sep 11, 2018

View reviewed changes

promag reviewed Sep 11, 2018

View reviewed changes

PierreRochard force-pushed the wallet-env-bug branch 2 times, most recently from 3f4d819 to 63fad10 Compare September 12, 2018 02:04

PierreRochard added 2 commits September 12, 2018 21:05

wallet: Add walletdir arg unit tests

ea3009e

wallet: Remove trailing separators from -walletdir arg

2d47163

PierreRochard force-pushed the wallet-env-bug branch from 63fad10 to 2d47163 Compare September 13, 2018 01:06

ryanofsky reviewed Oct 18, 2018

View reviewed changes

PierreRochard deleted the wallet-env-bug branch October 20, 2018 02:14

ken2812221 mentioned this pull request Nov 6, 2018

appveyor: Script improvement part II #14665

Merged

0xB10C mentioned this pull request May 4, 2019

Path error messages while executing test_bitcoin #15944

Closed

bitcoin deleted a comment May 4, 2019

luke-jr mentioned this pull request Oct 4, 2020

Strip any trailing / in -datadir and -blocksdir paths #20080

Merged

fanquake mentioned this pull request Oct 13, 2020

[0.20] build: set minimum required Boost to 1.48.0 #20142

Merged

kwvg mentioned this pull request Oct 12, 2021

merge bitcoin#10973: separate wallet from node dashpay/dash#4513

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

		m_walletdir_path_cases["trailing"] = m_datadir / "wallets" / sep;
		m_walletdir_path_cases["trailing2"] = m_datadir / "wallets" / sep / sep;

wallet: Remove trailing separators from -walletdir arg #14146

wallet: Remove trailing separators from -walletdir arg #14146

Uh oh!

Conversation

PierreRochard commented Sep 4, 2018

Uh oh!

PierreRochard commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

PierreRochard commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

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 Sep 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!

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

Uh oh!

Uh oh!

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

Uh oh!

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!

PierreRochard commented Sep 11, 2018

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!

PierreRochard commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

PierreRochard commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PierreRochard commented Sep 4, 2018 •

edited

Loading

DrahtBot commented Sep 4, 2018 •

edited

Loading

PierreRochard commented Sep 4, 2018 •

edited

Loading

promag Sep 5, 2018 •

edited

Loading

ken2812221 commented Sep 5, 2018 •

edited

Loading

PierreRochard commented Sep 6, 2018 •

edited

Loading

PierreRochard commented Sep 11, 2018 •

edited

Loading

PierreRochard commented Sep 12, 2018 •

edited

Loading

PierreRochard commented Sep 13, 2018 •

edited

Loading

ken2812221 commented Sep 15, 2018 •

edited

Loading