Browse Source

Merge #12220: Error if relative -walletdir is specified

ec527c6 Don't allow relative -walletdir paths (Russell Yanofsky)

Pull request description:

  This makes it an error to explicitly specify a non-absolute -walletdir path, and also adds a debug.log warning if a relative rather than absolute -datadir path is configured.

  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.

  Specifying a relative -datadir now adds a warning to the debug log. It would not be backwards-compatible to forbid relative -datadir paths entirely, and it could also be inconvenient for command line testing.

  Specifying a relative -walletdir now results in a startup error. But since the -walletdir option is new in 0.16.0, there should be no compatibility issues. Another reason not to use working directory paths for -walletdir specifically is that the default -walletdir is a "wallets" subdirectory inside the datadir, so it could be surprising that setting -walletdir manually would choose a directory rooted in a completely different location.

Tree-SHA512: 67cbdae677f82487a9031c5ec96b0205a488ab08718a0f4f49365e025119f3d7f6cfc88ba1eba04c1ecd8b9561a5b2c42e2e1a267af7c08c76b83e5e361f5a31
0.16
Wladimir J. van der Laan 7 years ago
parent
commit
f4c942e361
No known key found for this signature in database
GPG Key ID: 1E4AED62986CD25D
  1. 5
      doc/release-notes.md
  2. 9
      src/init.cpp
  3. 12
      src/wallet/init.cpp
  4. 2
      src/wallet/walletutil.cpp
  5. 4
      test/functional/multiwallet.py
  6. 12
      test/functional/test_framework/test_framework.py
  7. 4
      test/functional/test_framework/test_node.py

5
doc/release-notes.md

@ -90,9 +90,8 @@ bitcoin data directory. The behavior is now:
already exists in the data directory root, then wallets will be stored in the already exists in the data directory root, then wallets will be stored in the
`wallets/` subdirectory by default. `wallets/` subdirectory by default.
- The location of the wallets directory can be overridden by specifying a - The location of the wallets directory can be overridden by specifying a
`-walletdir=<path>` option where `<path>` can be an absolute path or a `-walletdir=<path>` option where `<path>` can be an absolute path to a
relative path (relative to the current working directory). `<path>` can directory or directory symlink.
also be a path to symlink to a directory.
Care should be taken when choosing the wallets directory location, as if it Care should be taken when choosing the wallets directory location, as if it
becomes unavailable during operation, funds may be lost. becomes unavailable during operation, funds may be lost.

9
src/init.cpp

@ -1210,6 +1210,15 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
LogPrintf("Using config file %s\n", GetConfigFile(gArgs.GetArg("-conf", BITCOIN_CONF_FILENAME)).string()); LogPrintf("Using config file %s\n", GetConfigFile(gArgs.GetArg("-conf", BITCOIN_CONF_FILENAME)).string());
LogPrintf("Using at most %i automatic connections (%i file descriptors available)\n", nMaxConnections, nFD); LogPrintf("Using at most %i automatic connections (%i file descriptors available)\n", nMaxConnections, nFD);
// Warn about relative -datadir path.
if (gArgs.IsArgSet("-datadir") && !fs::path(gArgs.GetArg("-datadir", "")).is_absolute()) {
LogPrintf("Warning: relative datadir option '%s' specified, which will be interpreted relative to the "
"current working directory '%s'. This is fragile, because if bitcoin is started in the future "
"from a different location, it will be unable to locate the current data files. There could "
"also be data loss if bitcoin is started while in a temporary directory.\n",
gArgs.GetArg("-datadir", ""), fs::current_path().string());
}
InitSignatureCache(); InitSignatureCache();
InitScriptExecutionCache(); InitScriptExecutionCache();

12
src/wallet/init.cpp

@ -205,11 +205,15 @@ bool VerifyWallets()
return true; return true;
} }
if (gArgs.IsArgSet("-walletdir") && !fs::is_directory(GetWalletDir())) { if (gArgs.IsArgSet("-walletdir")) {
if (fs::exists(fs::system_complete(gArgs.GetArg("-walletdir", "")))) { fs::path wallet_dir = gArgs.GetArg("-walletdir", "");
return InitError(strprintf(_("Specified -walletdir \"%s\" is not a directory"), gArgs.GetArg("-walletdir", "").c_str())); if (!fs::exists(wallet_dir)) {
return InitError(strprintf(_("Specified -walletdir \"%s\" does not exist"), wallet_dir.string()));
} else if (!fs::is_directory(wallet_dir)) {
return InitError(strprintf(_("Specified -walletdir \"%s\" is not a directory"), wallet_dir.string()));
} else if (!wallet_dir.is_absolute()) {
return InitError(strprintf(_("Specified -walletdir \"%s\" is a relative path"), wallet_dir.string()));
} }
return InitError(strprintf(_("Specified -walletdir \"%s\" does not exist"), gArgs.GetArg("-walletdir", "").c_str()));
} }
LogPrintf("Using wallet directory %s\n", GetWalletDir().string()); LogPrintf("Using wallet directory %s\n", GetWalletDir().string());

2
src/wallet/walletutil.cpp

@ -9,7 +9,7 @@ fs::path GetWalletDir()
fs::path path; fs::path path;
if (gArgs.IsArgSet("-walletdir")) { if (gArgs.IsArgSet("-walletdir")) {
path = fs::system_complete(gArgs.GetArg("-walletdir", "")); path = gArgs.GetArg("-walletdir", "");
if (!fs::is_directory(path)) { if (!fs::is_directory(path)) {
// If the path specified doesn't exist, we return the deliberately // If the path specified doesn't exist, we return the deliberately
// invalid empty string. // invalid empty string.

4
test/functional/multiwallet.py

@ -30,6 +30,10 @@ class MultiWalletTest(BitcoinTestFramework):
self.stop_nodes() self.stop_nodes()
self.assert_start_raises_init_error(0, ['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" does not exist')
self.assert_start_raises_init_error(0, ['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" is a relative path', cwd=data_dir())
self.assert_start_raises_init_error(0, ['-walletdir=debug.log'], 'Error: Specified -walletdir "debug.log" is not a directory', cwd=data_dir())
# should not initialize if there are duplicate wallets # should not initialize if there are duplicate wallets
self.assert_start_raises_init_error(0, ['-wallet=w1', '-wallet=w1'], 'Error loading wallet w1. Duplicate -wallet filename specified.') self.assert_start_raises_init_error(0, ['-wallet=w1', '-wallet=w1'], 'Error loading wallet w1. Duplicate -wallet filename specified.')

12
test/functional/test_framework/test_framework.py

@ -220,18 +220,18 @@ class BitcoinTestFramework():
for i in range(num_nodes): for i in range(num_nodes):
self.nodes.append(TestNode(i, self.options.tmpdir, extra_args[i], rpchost, timewait=timewait, binary=binary[i], stderr=None, mocktime=self.mocktime, coverage_dir=self.options.coveragedir, use_cli=self.options.usecli)) self.nodes.append(TestNode(i, self.options.tmpdir, extra_args[i], rpchost, timewait=timewait, binary=binary[i], stderr=None, mocktime=self.mocktime, coverage_dir=self.options.coveragedir, use_cli=self.options.usecli))
def start_node(self, i, extra_args=None, stderr=None): def start_node(self, i, *args, **kwargs):
"""Start a bitcoind""" """Start a bitcoind"""
node = self.nodes[i] node = self.nodes[i]
node.start(extra_args, stderr) node.start(*args, **kwargs)
node.wait_for_rpc_connection() node.wait_for_rpc_connection()
if self.options.coveragedir is not None: if self.options.coveragedir is not None:
coverage.write_all_rpc_commands(self.options.coveragedir, node.rpc) coverage.write_all_rpc_commands(self.options.coveragedir, node.rpc)
def start_nodes(self, extra_args=None): def start_nodes(self, extra_args=None, *args, **kwargs):
"""Start multiple bitcoinds""" """Start multiple bitcoinds"""
if extra_args is None: if extra_args is None:
@ -239,7 +239,7 @@ class BitcoinTestFramework():
assert_equal(len(extra_args), self.num_nodes) assert_equal(len(extra_args), self.num_nodes)
try: try:
for i, node in enumerate(self.nodes): for i, node in enumerate(self.nodes):
node.start(extra_args[i]) node.start(extra_args[i], *args, **kwargs)
for node in self.nodes: for node in self.nodes:
node.wait_for_rpc_connection() node.wait_for_rpc_connection()
except: except:
@ -271,10 +271,10 @@ class BitcoinTestFramework():
self.stop_node(i) self.stop_node(i)
self.start_node(i, extra_args) self.start_node(i, extra_args)
def assert_start_raises_init_error(self, i, extra_args=None, expected_msg=None): def assert_start_raises_init_error(self, i, extra_args=None, expected_msg=None, *args, **kwargs):
with tempfile.SpooledTemporaryFile(max_size=2**16) as log_stderr: with tempfile.SpooledTemporaryFile(max_size=2**16) as log_stderr:
try: try:
self.start_node(i, extra_args, stderr=log_stderr) self.start_node(i, extra_args, stderr=log_stderr, *args, **kwargs)
self.stop_node(i) self.stop_node(i)
except Exception as e: except Exception as e:
assert 'bitcoind exited' in str(e) # node must have shutdown assert 'bitcoind exited' in str(e) # node must have shutdown

4
test/functional/test_framework/test_node.py

@ -81,13 +81,13 @@ class TestNode():
assert self.rpc_connected and self.rpc is not None, "Error: no RPC connection" assert self.rpc_connected and self.rpc is not None, "Error: no RPC connection"
return getattr(self.rpc, name) return getattr(self.rpc, name)
def start(self, extra_args=None, stderr=None): def start(self, extra_args=None, stderr=None, *args, **kwargs):
"""Start the node.""" """Start the node."""
if extra_args is None: if extra_args is None:
extra_args = self.extra_args extra_args = self.extra_args
if stderr is None: if stderr is None:
stderr = self.stderr stderr = self.stderr
self.process = subprocess.Popen(self.args + extra_args, stderr=stderr) self.process = subprocess.Popen(self.args + extra_args, stderr=stderr, *args, **kwargs)
self.running = True self.running = True
self.log.debug("bitcoind started, waiting for RPC to come up") self.log.debug("bitcoind started, waiting for RPC to come up")

Loading…
Cancel
Save