diff options
author | Russell Yanofsky <russ@yanofsky.org> | 2018-01-18 13:15:00 -0500 |
---|---|---|
committer | Russell Yanofsky <russ@yanofsky.org> | 2018-01-18 15:09:27 -0500 |
commit | ec527c6c88146d5b36de38a1fcebe4f6ea72bd1b (patch) | |
tree | 6413ebc3e95bd2925a2963299dbfddf360b56329 | |
parent | e839d6570d9d0575a63ac45d35ad8bc248aeaacc (diff) | |
download | bitcoin-ec527c6c88146d5b36de38a1fcebe4f6ea72bd1b.tar.xz |
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.
-rw-r--r-- | doc/release-notes.md | 5 | ||||
-rw-r--r-- | src/init.cpp | 9 | ||||
-rw-r--r-- | src/wallet/init.cpp | 12 | ||||
-rw-r--r-- | src/wallet/walletutil.cpp | 2 | ||||
-rwxr-xr-x | test/functional/multiwallet.py | 4 | ||||
-rwxr-xr-x | test/functional/test_framework/test_framework.py | 12 | ||||
-rwxr-xr-x | test/functional/test_framework/test_node.py | 4 |
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") |