Don't allow relative -walletdir paths

Also warn if bitcoind is configured to use a relative -datadir path.

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 also 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.
This commit is contained in:
Russell Yanofsky 2018-01-18 13:15:00 -05:00
parent e839d6570d
commit ec527c6c88
7 changed files with 32 additions and 16 deletions

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

@ -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")