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: Remove trailing separators from -walletdir arg #14146

Merged
merged 2 commits into from Oct 18, 2018

Conversation

Projects
None yet
8 participants
@PierreRochard
Copy link
Contributor

PierreRochard commented Sep 4, 2018

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 fanquake added the Wallet label Sep 4, 2018

@PierreRochard

This comment has been minimized.

Copy link
Contributor Author

PierreRochard commented Sep 4, 2018

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 PierreRochard force-pushed the PierreRochard:wallet-env-bug branch Sep 4, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Sep 4, 2018

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.

@PierreRochard

This comment has been minimized.

Copy link
Contributor Author

PierreRochard commented Sep 4, 2018

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

src/wallet/db.cpp Outdated
// Ensure that the directory does not end with a trailing separator to avoid
// creating two Berkeley environments in the same directory
while ((env_directory.string().back() == '/') || (env_directory.string().back() == '\\'))
env_directory = env_directory.remove_trailing_separator();

This comment has been minimized.

@ken2812221

ken2812221 Sep 4, 2018

Member

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

This comment has been minimized.

@PierreRochard

PierreRochard Sep 6, 2018

Author Contributor

Thank you, that's much cleaner, fixed!

@promag
Copy link
Member

promag left a comment

Move the path cleanup here?

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

src/wallet/test/db_tests.cpp Outdated

std::vector<fs::path> wallet_test_paths;
std::string trailing_slashes;
for (int i = 0; i < 4; ++i ) {

This comment has been minimized.

@promag

promag Sep 5, 2018

Member

nit, remove space after ++i.

This comment has been minimized.

@PierreRochard

PierreRochard Sep 6, 2018

Author Contributor

Fixed

src/wallet/test/db_tests.cpp Outdated
*/
BOOST_AUTO_TEST_CASE(get_wallet_env_trailing_slash)
{
const char kPathSeparator =

This comment has been minimized.

@promag

promag Sep 5, 2018

Member

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

This comment has been minimized.

@fanquake

fanquake Sep 5, 2018

Member

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

This comment has been minimized.

@promag

promag Sep 5, 2018

Member

Yes, just wanted give full type for reference.

This comment has been minimized.

@PierreRochard

PierreRochard Sep 6, 2018

Author Contributor

Thank you, that's great, fixed!

@ken2812221

This comment has been minimized.

Copy link
Member

ken2812221 commented Sep 5, 2018

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"

@fanquake 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 PierreRochard force-pushed the PierreRochard:wallet-env-bug branch to d129dba Sep 6, 2018

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

@PierreRochard

This comment has been minimized.

Copy link
Contributor Author

PierreRochard commented Sep 6, 2018

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

Force push diff: PierreRochard@5a28a99

@@ -23,5 +23,11 @@ fs::path GetWalletDir()
}
}

// 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())) {

This comment has been minimized.

@promag

promag Sep 6, 2018

Member

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.

This comment has been minimized.

@PierreRochard

PierreRochard Sep 6, 2018

Author Contributor

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.

This comment has been minimized.

@promag

promag Sep 6, 2018

Member

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

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

This comment has been minimized.

@PierreRochard

PierreRochard Sep 6, 2018

Author Contributor

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

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

This comment has been minimized.

@promag

promag Sep 6, 2018

Member

I would add a couple of cases instead.

@PierreRochard PierreRochard force-pushed the PierreRochard:wallet-env-bug branch from d129dba Sep 11, 2018

@PierreRochard

This comment has been minimized.

Copy link
Contributor Author

PierreRochard commented Sep 11, 2018

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 PierreRochard force-pushed the PierreRochard:wallet-env-bug branch to ab3bf76 Sep 11, 2018

@bitcoin bitcoin deleted a comment Sep 11, 2018

#include <wallet/test/init_test_fixture.h>

#include <wallet/wallet.h>
#include <wallet/init.cpp>

This comment has been minimized.

@ken2812221

ken2812221 Sep 11, 2018

Member

Why this include cpp file?

This comment has been minimized.

@PierreRochard

PierreRochard Sep 11, 2018

Author Contributor

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?

This comment has been minimized.

@promag

promag Sep 11, 2018

Member

Include src/walletinitinterface.h?

This comment has been minimized.

@PierreRochard

PierreRochard Sep 11, 2018

Author Contributor

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

This comment has been minimized.

@ken2812221

ken2812221 Sep 11, 2018

Member

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

This comment has been minimized.

@promag

promag Sep 11, 2018

Member

There is already an instance. Just replace the include.

This comment has been minimized.

@PierreRochard

PierreRochard Sep 12, 2018

Author Contributor

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

@PierreRochard

This comment has been minimized.

Copy link
Contributor Author

PierreRochard commented Sep 11, 2018

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

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


This comment has been minimized.

@promag

promag Sep 11, 2018

Member

Remove 2nd empty line.

This comment has been minimized.

@PierreRochard

PierreRochard Sep 12, 2018

Author Contributor

Fixed

#include <wallet/test/init_test_fixture.h>

#include <wallet/wallet.h>
#include <wallet/init.cpp>

This comment has been minimized.

@promag

promag Sep 11, 2018

Member

There is already an instance. Just replace the include.

@PierreRochard PierreRochard force-pushed the PierreRochard:wallet-env-bug branch 2 times, most recently from 3f4d819 to 63fad10 Sep 12, 2018

@PierreRochard

This comment has been minimized.

Copy link
Contributor Author

PierreRochard commented Sep 12, 2018

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 PierreRochard force-pushed the PierreRochard:wallet-env-bug branch from 63fad10 to 2d47163 Sep 13, 2018

@PierreRochard

This comment has been minimized.

Copy link
Contributor Author

PierreRochard commented Sep 13, 2018

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

This comment has been minimized.

Copy link
Member

ken2812221 commented Sep 13, 2018

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

@PierreRochard

This comment has been minimized.

Copy link
Contributor Author

PierreRochard commented Sep 13, 2018

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

This comment has been minimized.

Copy link
Member

ken2812221 commented Sep 15, 2018

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

@PierreRochard

This comment has been minimized.

Copy link
Contributor Author

PierreRochard commented Sep 15, 2018

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

@ken2812221

This comment has been minimized.

Copy link
Member

ken2812221 commented Sep 16, 2018

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

@PierreRochard

This comment has been minimized.

Copy link
Contributor Author

PierreRochard commented Sep 16, 2018

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:
""
void InitWalletDirTestingSetup::SetWalletDir(const fs::path& walletdir_path)
{
gArgs.ForceSetArg("-walletdir", walletdir_path.string());
}

This comment has been minimized.

@practicalswift

practicalswift Sep 23, 2018

Member
2018-09-22 21:11:48 cpplint(pr=14146): src/wallet/test/init_test_fixture.cpp:42:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]

#include <init.h>
#include <walletinitinterface.h>
#include <wallet/wallet.h>

This comment has been minimized.

@practicalswift

practicalswift Sep 23, 2018

Member

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


#include <test/test_bitcoin.h>


This comment has been minimized.

@practicalswift

practicalswift Sep 23, 2018

Member

Nit: Extra newline.

void InitWalletDirTestingSetup::SetWalletDir(const fs::path& walletdir_path)
{
gArgs.ForceSetArg("-walletdir", walletdir_path.string());
}

This comment has been minimized.

@practicalswift

practicalswift Sep 25, 2018

Member
2018-09-25 20:31:54 clang(pr=14146): wallet/test/init_test_fixture.cpp:42:2: warning: no newline at end of file [-Wnewline-eof]
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 18, 2018

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

@laanwj laanwj merged commit 2d47163 into bitcoin:master Oct 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 18, 2018

Merge #14146: wallet: Remove trailing separators from -walletdir 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 #12493 (comment)

Tree-SHA512: f2bbf1749d904fd3f326b88f2ead58c8386034355910906d7faea155d518642e9cd4ceb3cae272f2d9d8feb61f126523e1c97502799d24e4315bb53e49fd7c09
@ryanofsky
Copy link
Contributor

ryanofsky left a comment

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

@PierreRochard PierreRochard deleted the PierreRochard:wallet-env-bug branch Oct 20, 2018

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Nov 7, 2018

Merge bitcoin#14665: appveyor: Script improvement part II
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.