From ee47ca29d6ef55650a0af63bca817c5d494f31ef Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 22 Aug 2024 19:41:54 -0300 Subject: [PATCH] init: fix fatal error on '-wallet' negated option value Because we don't have type checking for command-line/settings/config args, strings are interpreted as 'false' for non-boolean args. By convention, this "forces" us to interpret negated strings as 'true', which conflicts with the negated option definition in all the settings classes (they expect negated options to always be false and ignore any other value preceding them). Consequently, when retrieving all "wallet" values from the command-line/settings/config, we also fetch the negated string boolean value, which is not of the expected 'string' type. This mismatch leads to an internal fatal error, resulting in an unclean shutdown during initialization. Furthermore, this error displays a poorly descriptive error message: "JSON value of type bool is not of expected type string" This commit fixes the fatal error by ensuring that only string values are returned in the "wallet" settings list, failing otherwise. It also improves the clarity of the returned error message. Co-authored-by: Ryan Ofsky --- src/wallet/load.cpp | 10 ++++++++++ test/functional/feature_config_args.py | 7 +++++++ test/functional/feature_settings.py | 23 +++++++++++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index e26347d437..4502bb125a 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -77,6 +77,11 @@ bool VerifyWallets(WalletContext& context) std::set wallet_paths; for (const auto& wallet : chain.getSettingsList("wallet")) { + if (!wallet.isStr()) { + chain.initError(_("Invalid value detected for '-wallet' or '-nowallet'. " + "'-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets")); + return false; + } const auto& wallet_file = wallet.get_str(); const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(wallet_file)); @@ -110,6 +115,11 @@ bool LoadWallets(WalletContext& context) try { std::set wallet_paths; for (const auto& wallet : chain.getSettingsList("wallet")) { + if (!wallet.isStr()) { + chain.initError(_("Invalid value detected for '-wallet' or '-nowallet'. " + "'-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets")); + return false; + } const auto& name = wallet.get_str(); if (!wallet_paths.insert(fs::PathFromString(name)).second) { continue; diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index bb20e2baa8..44c7edf962 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -153,6 +153,13 @@ class ConfArgsTest(BitcoinTestFramework): expected_msg='Error: Error parsing command line arguments: Can not set -proxy with no value. Please specify value with -proxy=value.', extra_args=['-proxy'], ) + # Provide a value different from 1 to the -wallet negated option + if self.is_wallet_compiled(): + for value in [0, 'not_a_boolean']: + self.nodes[0].assert_start_raises_init_error( + expected_msg="Error: Invalid value detected for '-wallet' or '-nowallet'. '-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets", + extra_args=[f'-nowallet={value}'], + ) def test_log_buffer(self): self.stop_node(0) diff --git a/test/functional/feature_settings.py b/test/functional/feature_settings.py index 2189eac7dd..a7294944bf 100755 --- a/test/functional/feature_settings.py +++ b/test/functional/feature_settings.py @@ -13,11 +13,32 @@ from test_framework.util import assert_equal class SettingsTest(BitcoinTestFramework): + def add_options(self, parser): + self.add_wallet_options(parser) + def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 1 self.wallet_names = [] + def test_wallet_settings(self, settings_path): + if not self.is_wallet_compiled(): + return + + self.log.info("Testing wallet settings..") + node = self.nodes[0] + # Create wallet to use it during tests + self.start_node(0) + node.createwallet(wallet_name='w1') + self.stop_node(0) + + # Verify wallet settings can only be strings. Either names or paths. Not booleans, nums nor anything else. + for wallets_data in [[10], [True], [[]], [{}], ["w1", 10], ["w1", False]]: + with settings_path.open("w") as fp: + json.dump({"wallet": wallets_data}, fp) + node.assert_start_raises_init_error(expected_msg="Error: Invalid value detected for '-wallet' or '-nowallet'. '-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets", + extra_args=[f'-settings={settings_path}']) + def run_test(self): node, = self.nodes settings = node.chain_path / "settings.json" @@ -86,6 +107,8 @@ class SettingsTest(BitcoinTestFramework): self.start_node(0, extra_args=[f"-settings={altsettings}"]) self.stop_node(0) + self.test_wallet_settings(settings) + if __name__ == '__main__': SettingsTest(__file__).main()