From 64e33f715f989268ee6c2358d4f8dcecd9fbac66 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 19 Jan 2018 17:44:27 +0100 Subject: [PATCH] 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 --- src/init.cpp | 9 +++++++++ src/wallet/init.cpp | 12 ++++++++---- src/wallet/walletutil.cpp | 2 +- test/functional/multiwallet.py | 4 ++++ test/functional/test_framework/test_framework.py | 12 ++++++------ test/functional/test_framework/test_node.py | 4 ++-- 6 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index cd69b46c8e..0625115107 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1548,6 +1548,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 Dash Core 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 Dash Core 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 77cf9222a1..f63b151567 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -212,11 +212,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 41df1a7c85..740ce9fc18 100755 --- a/test/functional/multiwallet.py +++ b/test/functional/multiwallet.py @@ -30,6 +30,10 @@ class MultiWalletTest(BitcoinTestFramework): self.stop_node(0) + 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 eeeff821aa..9e8492e0cf 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -254,18 +254,18 @@ class BitcoinTestFramework(object): for i in range(num_nodes): self.nodes.append(TestNode(old_num_nodes + i, self.options.tmpdir, extra_args[i], rpchost, timewait=timewait, binary=binary[i], stderr=stderr, 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 dashd""" 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, stderr=None): + def start_nodes(self, extra_args=None, stderr=None, *args, **kwargs): """Start multiple dashds""" if extra_args is None: @@ -273,7 +273,7 @@ class BitcoinTestFramework(object): assert_equal(len(extra_args), self.num_nodes) try: for i, node in enumerate(self.nodes): - node.start(extra_args[i], stderr) + node.start(extra_args[i], stderr, *args, **kwargs) for node in self.nodes: node.wait_for_rpc_connection() except: @@ -305,10 +305,10 @@ class BitcoinTestFramework(object): 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 'dashd 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 48adb33155..b9224c39c1 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -85,13 +85,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("dashd started, waiting for RPC to come up")