Skip to content

Commit d98777f

Browse files
laanwjlaanwj
committed
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 bitcoin/bitcoin#12493 (comment) Tree-SHA512: f2bbf1749d904fd3f326b88f2ead58c8386034355910906d7faea155d518642e9cd4ceb3cae272f2d9d8feb61f126523e1c97502799d24e4315bb53e49fd7c09
2 parents 9c5f0d5 + 2d47163 commit d98777f

File tree

6 files changed

+153
-4
lines changed

6 files changed

+153
-4
lines changed

‎build_msvc/test_bitcoin/test_bitcoin.vcxproj

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
<ClCompile Include="..\..\src\wallet\test\*_tests.cpp" />
2525
<ClCompile Include="..\..\src\test\test_bitcoin.cpp" />
2626
<ClCompile Include="..\..\src\test\test_bitcoin_main.cpp" />
27-
<ClCompile Include="..\..\src\wallet\test\wallet_test_fixture.cpp" />
27+
<ClCompile Include="..\..\src\wallet\test\*_fixture.cpp" />
2828
</ItemGroup>
2929
<ItemGroup>
3030
<ProjectReference Include="..\libbitcoinconsensus\libbitcoinconsensus.vcxproj">

‎src/Makefile.test.include

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,11 +111,14 @@ BITCOIN_TESTS += \
111111
wallet/test/psbt_wallet_tests.cpp \
112112
wallet/test/wallet_tests.cpp \
113113
wallet/test/wallet_crypto_tests.cpp \
114-
wallet/test/coinselector_tests.cpp
114+
wallet/test/coinselector_tests.cpp \
115+
wallet/test/init_tests.cpp
115116

116117
BITCOIN_TEST_SUITE += \
117118
wallet/test/wallet_test_fixture.cpp \
118-
wallet/test/wallet_test_fixture.h
119+
wallet/test/wallet_test_fixture.h \
120+
wallet/test/init_test_fixture.cpp \
121+
wallet/test/init_test_fixture.h
119122
endif
120123

121124
test_test_bitcoin_SOURCES = $(BITCOIN_TEST_SUITE) $(BITCOIN_TESTS) $(JSON_TEST_FILES) $(RAW_TEST_FILES)

‎src/wallet/init.cpp

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,13 +182,18 @@ bool WalletInit::Verify() const
182182

183183
if (gArgs.IsArgSet("-walletdir")) {
184184
fs::path wallet_dir = gArgs.GetArg("-walletdir", "");
185-
if (!fs::exists(wallet_dir)) {
185+
boost::system::error_code error;
186+
// The canonical path cleans the path, preventing >1 Berkeley environment instances for the same directory
187+
fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error);
188+
if (error || !fs::exists(wallet_dir)) {
186189
return InitError(strprintf(_("Specified -walletdir \"%s\" does not exist"), wallet_dir.string()));
187190
} else if (!fs::is_directory(wallet_dir)) {
188191
return InitError(strprintf(_("Specified -walletdir \"%s\" is not a directory"), wallet_dir.string()));
192+
// The canonical path transforms relative paths into absolute ones, so we check the non-canonical version
189193
} else if (!wallet_dir.is_absolute()) {
190194
return InitError(strprintf(_("Specified -walletdir \"%s\" is a relative path"), wallet_dir.string()));
191195
}
196+
gArgs.ForceSetArg("-walletdir", canonical_wallet_dir.string());
192197
}
193198

194199
LogPrintf("Using wallet directory %s\n", GetWalletDir().string());

‎src/wallet/test/init_test_fixture.cpp

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
// Copyright (c) 2018 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <fs.h>
6+
7+
#include <wallet/test/init_test_fixture.h>
8+
9+
InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainName): BasicTestingSetup(chainName)
10+
{
11+
std::string sep;
12+
sep += fs::path::preferred_separator;
13+
14+
m_datadir = SetDataDir("tempdir");
15+
m_cwd = fs::current_path();
16+
17+
m_walletdir_path_cases["default"] = m_datadir / "wallets";
18+
m_walletdir_path_cases["custom"] = m_datadir / "my_wallets";
19+
m_walletdir_path_cases["nonexistent"] = m_datadir / "path_does_not_exist";
20+
m_walletdir_path_cases["file"] = m_datadir / "not_a_directory.dat";
21+
m_walletdir_path_cases["trailing"] = m_datadir / "wallets" / sep;
22+
m_walletdir_path_cases["trailing2"] = m_datadir / "wallets" / sep / sep;
23+
24+
fs::current_path(m_datadir);
25+
m_walletdir_path_cases["relative"] = "wallets";
26+
27+
fs::create_directories(m_walletdir_path_cases["default"]);
28+
fs::create_directories(m_walletdir_path_cases["custom"]);
29+
fs::create_directories(m_walletdir_path_cases["relative"]);
30+
std::ofstream f(m_walletdir_path_cases["file"].BOOST_FILESYSTEM_C_STR);
31+
f.close();
32+
}
33+
34+
InitWalletDirTestingSetup::~InitWalletDirTestingSetup()
35+
{
36+
fs::current_path(m_cwd);
37+
}
38+
39+
void InitWalletDirTestingSetup::SetWalletDir(const fs::path& walletdir_path)
40+
{
41+
gArgs.ForceSetArg("-walletdir", walletdir_path.string());
42+
}

‎src/wallet/test/init_test_fixture.h

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// Copyright (c) 2018 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
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+
11+
struct InitWalletDirTestingSetup: public BasicTestingSetup {
12+
explicit InitWalletDirTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
13+
~InitWalletDirTestingSetup();
14+
void SetWalletDir(const fs::path& walletdir_path);
15+
16+
fs::path m_datadir;
17+
fs::path m_cwd;
18+
std::map<std::string, fs::path> m_walletdir_path_cases;
19+
};
20+
21+
#endif // BITCOIN_WALLET_TEST_INIT_TEST_FIXTURE_H

‎src/wallet/test/init_tests.cpp

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
// Copyright (c) 2018 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <boost/test/unit_test.hpp>
6+
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>
13+
14+
15+
BOOST_FIXTURE_TEST_SUITE(init_tests, InitWalletDirTestingSetup)
16+
17+
BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_default)
18+
{
19+
SetWalletDir(m_walletdir_path_cases["default"]);
20+
bool result = g_wallet_init_interface.Verify();
21+
BOOST_CHECK(result == true);
22+
fs::path walletdir = gArgs.GetArg("-walletdir", "");
23+
fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
24+
BOOST_CHECK(walletdir == expected_path);
25+
}
26+
27+
BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_custom)
28+
{
29+
SetWalletDir(m_walletdir_path_cases["custom"]);
30+
bool result = g_wallet_init_interface.Verify();
31+
BOOST_CHECK(result == true);
32+
fs::path walletdir = gArgs.GetArg("-walletdir", "");
33+
fs::path expected_path = fs::canonical(m_walletdir_path_cases["custom"]);
34+
BOOST_CHECK(walletdir == expected_path);
35+
}
36+
37+
BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_does_not_exist)
38+
{
39+
SetWalletDir(m_walletdir_path_cases["nonexistent"]);
40+
bool result = g_wallet_init_interface.Verify();
41+
BOOST_CHECK(result == false);
42+
}
43+
44+
BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_is_not_directory)
45+
{
46+
SetWalletDir(m_walletdir_path_cases["file"]);
47+
bool result = g_wallet_init_interface.Verify();
48+
BOOST_CHECK(result == false);
49+
}
50+
51+
BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_is_not_relative)
52+
{
53+
SetWalletDir(m_walletdir_path_cases["relative"]);
54+
bool result = g_wallet_init_interface.Verify();
55+
BOOST_CHECK(result == false);
56+
}
57+
58+
BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing)
59+
{
60+
SetWalletDir(m_walletdir_path_cases["trailing"]);
61+
bool result = g_wallet_init_interface.Verify();
62+
BOOST_CHECK(result == true);
63+
fs::path walletdir = gArgs.GetArg("-walletdir", "");
64+
fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
65+
BOOST_CHECK(walletdir == expected_path);
66+
}
67+
68+
BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing2)
69+
{
70+
SetWalletDir(m_walletdir_path_cases["trailing2"]);
71+
bool result = g_wallet_init_interface.Verify();
72+
BOOST_CHECK(result == true);
73+
fs::path walletdir = gArgs.GetArg("-walletdir", "");
74+
fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
75+
BOOST_CHECK(walletdir == expected_path);
76+
}
77+
78+
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)