From eefe56967b4eb4b5144325cde4f40fc1cbde3e65 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 27 Mar 2023 10:17:57 -0400 Subject: [PATCH] bugfix: Fix incorrect debug.log config file path Currently debug.log will show the wrong bitcoin.conf config file path when bitcoind is invoked without -conf or -datadir arguments, and there's a default bitcoin.conf file which specifies another datadir= location. When this happens, the debug.log will include an incorrect "Config file:" line referring to a bitcoin.conf file in the other datadir, instead of the referring to the actual configuration file in the default datadir which was parsed. The bad log print was reported and originally fixed in https://github.com/bitcoin/bitcoin/pull/27303 by Matthew Zipkin This PR takes a slightly different approach to fixing the bug, trying to avoid future bugs by not allowing the GetConfigFilePath function to be called before the the configuration is parsed, and deleting GetConfigFile function which could be confused with GetConfigFilePath. It also includes a test for the bug which the original fix did not have. Co-authored-by: Matthew Zipkin --- src/common/args.cpp | 3 ++- src/common/args.h | 2 +- src/common/config.cpp | 8 ++---- src/common/init.cpp | 2 +- src/qt/test/test_main.cpp | 3 +++ test/functional/feature_config_args.py | 36 ++++++++++++++++++++++++++ 6 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/common/args.cpp b/src/common/args.cpp index d29b8648bf..ccd49eb8a3 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -714,7 +714,8 @@ bool CheckDataDirOption(const ArgsManager& args) fs::path ArgsManager::GetConfigFilePath() const { - return GetConfigFile(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME)); + LOCK(cs_args); + return *Assert(m_config_path); } std::string ArgsManager::GetChainName() const diff --git a/src/common/args.h b/src/common/args.h index 430c392e2b..1e463e98d7 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -26,7 +26,6 @@ extern const char * const BITCOIN_SETTINGS_FILENAME; // Return true if -datadir option points to a valid directory or is not specified. bool CheckDataDirOption(const ArgsManager& args); -fs::path GetConfigFile(const ArgsManager& args, const fs::path& configuration_file_path); /** * Most paths passed as configuration arguments are treated as relative to @@ -136,6 +135,7 @@ protected: std::map> m_available_args GUARDED_BY(cs_args); bool m_accept_any_command GUARDED_BY(cs_args){true}; std::list m_config_sections GUARDED_BY(cs_args); + std::optional m_config_path GUARDED_BY(cs_args); mutable fs::path m_cached_blocks_path GUARDED_BY(cs_args); mutable fs::path m_cached_datadir_path GUARDED_BY(cs_args); mutable fs::path m_cached_network_datadir_path GUARDED_BY(cs_args); diff --git a/src/common/config.cpp b/src/common/config.cpp index 747503ad2a..6bc71ffa0d 100644 --- a/src/common/config.cpp +++ b/src/common/config.cpp @@ -26,11 +26,6 @@ #include #include -fs::path GetConfigFile(const ArgsManager& args, const fs::path& configuration_file_path) -{ - return AbsPathForConfigVal(args, configuration_file_path, /*net_specific=*/false); -} - static bool GetConfigOptions(std::istream& stream, const std::string& filepath, std::string& error, std::vector>& options, std::list& sections) { std::string str, prefix; @@ -125,6 +120,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) LOCK(cs_args); m_settings.ro_config.clear(); m_config_sections.clear(); + m_config_path = AbsPathForConfigVal(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false); } const auto conf_path{GetConfigFilePath()}; @@ -175,7 +171,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) const size_t default_includes = add_includes({}); for (const std::string& conf_file_name : conf_file_names) { - std::ifstream conf_file_stream{GetConfigFile(*this, fs::PathFromString(conf_file_name))}; + std::ifstream conf_file_stream{AbsPathForConfigVal(*this, fs::PathFromString(conf_file_name), /*net_specific=*/false)}; if (conf_file_stream.good()) { if (!ReadConfigStream(conf_file_stream, conf_file_name, error, ignore_invalid_keys)) { return false; diff --git a/src/common/init.cpp b/src/common/init.cpp index 60df14de9a..f5a412b1a1 100644 --- a/src/common/init.cpp +++ b/src/common/init.cpp @@ -32,7 +32,7 @@ std::optional InitConfig(ArgsManager& args, SettingsAbortFn setting // parse error, and specifying a datadir= location containing another // bitcoin.conf file just ignores the other file.) const fs::path orig_datadir_path{args.GetDataDirBase()}; - const fs::path orig_config_path = args.GetConfigFilePath(); + const fs::path orig_config_path{AbsPathForConfigVal(args, args.GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false)}; std::string error; if (!args.ReadConfigFiles(error, true)) { diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp index 2d069f76a0..a0cf80cd31 100644 --- a/src/qt/test/test_main.cpp +++ b/src/qt/test/test_main.cpp @@ -70,6 +70,9 @@ int main(int argc, char* argv[]) gArgs.ForceSetArg("-upnp", "0"); gArgs.ForceSetArg("-natpmp", "0"); + std::string error; + if (!gArgs.ReadConfigFiles(error, true)) QWARN(error.c_str()); + // Prefer the "minimal" platform for the test instead of the normal default // platform ("xcb", "windows", or "cocoa") so tests can't unintentionally // interfere with any background GUIs and don't require extra resources. diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index a45888513f..2257605870 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -113,6 +113,41 @@ class ConfArgsTest(BitcoinTestFramework): with open(inc_conf_file2_path, 'w', encoding='utf-8') as conf: conf.write('') # clear + def test_config_file_log(self): + # Disable this test for windows currently because trying to override + # the default datadir through the environment does not seem to work. + if sys.platform == "win32": + return + + self.log.info('Test that correct configuration path is changed when configuration file changes the datadir') + + # Create a temporary directory that will be treated as the default data + # directory by bitcoind. + env, default_datadir = util.get_temp_default_datadir(pathlib.Path(self.options.tmpdir, "test_config_file_log")) + default_datadir.mkdir(parents=True) + + # Write a bitcoin.conf file in the default data directory containing a + # datadir= line pointing at the node datadir. + node = self.nodes[0] + conf_text = pathlib.Path(node.bitcoinconf).read_text() + conf_path = default_datadir / "bitcoin.conf" + conf_path.write_text(f"datadir={node.datadir}\n{conf_text}") + + # Drop the node -datadir= argument during this test, because if it is + # specified it would take precedence over the datadir setting in the + # config file. + node_args = node.args + node.args = [arg for arg in node.args if not arg.startswith("-datadir=")] + + # Check that correct configuration file path is actually logged + # (conf_path, not node.bitcoinconf) + with self.nodes[0].assert_debug_log(expected_msgs=[f"Config file: {conf_path}"]): + self.start_node(0, ["-allowignoredconf"], env=env) + self.stop_node(0) + + # Restore node arguments after the test + node.args = node_args + def test_invalid_command_line_options(self): self.nodes[0].assert_start_raises_init_error( expected_msg='Error: Error parsing command line arguments: Can not set -proxy with no value. Please specify value with -proxy=value.', @@ -344,6 +379,7 @@ class ConfArgsTest(BitcoinTestFramework): self.test_connect_with_seednode() self.test_config_file_parser() + self.test_config_file_log() self.test_invalid_command_line_options() self.test_ignored_conf() self.test_ignored_default_conf()