aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorWladimir J. van der Laan <laanwj@gmail.com>2018-01-19 17:44:27 +0100
committerWladimir J. van der Laan <laanwj@gmail.com>2018-01-19 17:46:17 +0100
commitf4c942e3615435c9e5d7ff0cefd935be08768d0c (patch)
treefb8d7b36d94846595de88fae91df70ec1edc8b89
parent9a97f39afaa890caa7987c6bc001b9a66e3e74e8 (diff)
parentec527c6c88146d5b36de38a1fcebe4f6ea72bd1b (diff)
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
-rw-r--r--doc/release-notes.md5
-rw-r--r--src/init.cpp9
-rw-r--r--src/wallet/init.cpp12
-rw-r--r--src/wallet/walletutil.cpp2
-rwxr-xr-xtest/functional/multiwallet.py4
-rwxr-xr-xtest/functional/test_framework/test_framework.py12
-rwxr-xr-xtest/functional/test_framework/test_node.py4
7 files changed, 32 insertions, 16 deletions
diff --git a/doc/release-notes.md b/doc/release-notes.md
index ecca08d96d..29377d4a1d 100644
--- a/doc/release-notes.md
+++ b/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
`wallets/` subdirectory by default.
- The location of the wallets directory can be overridden by specifying a
- `-walletdir=<path>` option where `<path>` can be an absolute path or a
- relative path (relative to the current working directory). `<path>` can
- also be a path to symlink to a directory.
+ `-walletdir=<path>` option where `<path>` can be an absolute path to a
+ directory or directory symlink.
Care should be taken when choosing the wallets directory location, as if it
becomes unavailable during operation, funds may be lost.
diff --git a/src/init.cpp b/src/init.cpp
index b48802637b..d1d733af94 100644
--- a/src/init.cpp
+++ b/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 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();
InitScriptExecutionCache();
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index 0f453f154f..2d26f7ae0f 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -205,11 +205,15 @@ bool VerifyWallets()
return true;
}
- if (gArgs.IsArgSet("-walletdir") && !fs::is_directory(GetWalletDir())) {
- if (fs::exists(fs::system_complete(gArgs.GetArg("-walletdir", "")))) {
- return InitError(strprintf(_("Specified -walletdir \"%s\" is not a directory"), gArgs.GetArg("-walletdir", "").c_str()));
+ if (gArgs.IsArgSet("-walletdir")) {
+ fs::path wallet_dir = gArgs.GetArg("-walletdir", "");
+ 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());
diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp
index f15e5de1e2..7c97b668ae 100644
--- a/src/wallet/walletutil.cpp
+++ b/src/wallet/walletutil.cpp
@@ -9,7 +9,7 @@ fs::path GetWalletDir()
fs::path path;
if (gArgs.IsArgSet("-walletdir")) {
- path = fs::system_complete(gArgs.GetArg("-walletdir", ""));
+ path = gArgs.GetArg("-walletdir", "");
if (!fs::is_directory(path)) {
// If the path specified doesn't exist, we return the deliberately
// invalid empty string.
diff --git a/test/functional/multiwallet.py b/test/functional/multiwallet.py
index 0891829127..b07e451667 100755
--- a/test/functional/multiwallet.py
+++ b/test/functional/multiwallet.py
@@ -30,6 +30,10 @@ class MultiWalletTest(BitcoinTestFramework):
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
self.assert_start_raises_init_error(0, ['-wallet=w1', '-wallet=w1'], 'Error loading wallet w1. Duplicate -wallet filename specified.')
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 5d4f8e6720..f8d66def64 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -220,18 +220,18 @@ class BitcoinTestFramework():
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))
- def start_node(self, i, extra_args=None, stderr=None):
+ def start_node(self, i, *args, **kwargs):
"""Start a bitcoind"""
node = self.nodes[i]
- node.start(extra_args, stderr)
+ node.start(*args, **kwargs)
node.wait_for_rpc_connection()
if self.options.coveragedir is not None:
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"""
if extra_args is None:
@@ -239,7 +239,7 @@ class BitcoinTestFramework():
assert_equal(len(extra_args), self.num_nodes)
try:
for i, node in enumerate(self.nodes):
- node.start(extra_args[i])
+ node.start(extra_args[i], *args, **kwargs)
for node in self.nodes:
node.wait_for_rpc_connection()
except:
@@ -271,10 +271,10 @@ class BitcoinTestFramework():
self.stop_node(i)
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:
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)
except Exception as e:
assert 'bitcoind exited' in str(e) # node must have shutdown
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index 589a8f3969..7c784d8840 100755
--- a/test/functional/test_framework/test_node.py
+++ b/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"
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."""
if extra_args is None:
extra_args = self.extra_args
if stderr is None:
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.log.debug("bitcoind started, waiting for RPC to come up")