From 0c162195b99c076b796d7495e62c7b997bac2a78 Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Fri, 24 Apr 2020 12:28:38 +0100 Subject: [PATCH] Safely read override values when no config file is present (#2727) One of the toml read methods was not reading within a try/catch. Re-arranged the methods slightly to avoid duplicating code. There was also this check before setting the error when reading from a stream: ``` auto pos (stream.tellg ()); if (pos != std::streampos (0)) ``` Removing this check has all tests pass nonetheless, and now only the lowest level method is responsible for setting the internal tree and error. --- nano/core_test/toml.cpp | 44 ++++++++++++++++++++++++++++++++++++++ nano/lib/rpcconfig.cpp | 2 +- nano/lib/tomlconfig.cpp | 40 ++++++++++++++-------------------- nano/lib/tomlconfig.hpp | 4 ++-- nano/node/daemonconfig.cpp | 2 +- 5 files changed, 64 insertions(+), 28 deletions(-) diff --git a/nano/core_test/toml.cpp b/nano/core_test/toml.cpp index 2f91693941..9ce9322e7e 100644 --- a/nano/core_test/toml.cpp +++ b/nano/core_test/toml.cpp @@ -798,3 +798,47 @@ TEST (toml, daemon_config_deserialize_errors) ASSERT_EQ (toml2.get_error ().get_message (), "frontiers_confirmation value is invalid (available: always, auto, disabled)"); ASSERT_EQ (conf2.node.frontiers_confirmation, nano::frontiers_confirmation_mode::invalid); } + +TEST (toml, daemon_read_config) +{ + auto path (nano::unique_path ()); + boost::filesystem::create_directories (path); + nano::daemon_config config; + std::vector invalid_overrides1{ "node.max_work_generate_multiplier=0" }; + std::string expected_message1{ "max_work_generate_multiplier must be greater than or equal to 1" }; + + std::vector invalid_overrides2{ "node.websocket.enable=true", "node.foo" }; + std::string expected_message2{ "Value must follow after a '=' at line 2" }; + + // Reading when there is no config file + ASSERT_FALSE (boost::filesystem::exists (nano::get_node_toml_config_path (path))); + ASSERT_FALSE (nano::read_node_config_toml (path, config)); + { + auto error = nano::read_node_config_toml (path, config, invalid_overrides1); + ASSERT_TRUE (error); + ASSERT_EQ (error.get_message (), expected_message1); + } + { + auto error = nano::read_node_config_toml (path, config, invalid_overrides2); + ASSERT_TRUE (error); + ASSERT_EQ (error.get_message (), expected_message2); + } + + // Create an empty config + nano::tomlconfig toml; + toml.write (nano::get_node_toml_config_path (path)); + + // Reading when there is a config file + ASSERT_TRUE (boost::filesystem::exists (nano::get_node_toml_config_path (path))); + ASSERT_FALSE (nano::read_node_config_toml (path, config)); + { + auto error = nano::read_node_config_toml (path, config, invalid_overrides1); + ASSERT_TRUE (error); + ASSERT_EQ (error.get_message (), expected_message1); + } + { + auto error = nano::read_node_config_toml (path, config, invalid_overrides2); + ASSERT_TRUE (error); + ASSERT_EQ (error.get_message (), expected_message2); + } +} diff --git a/nano/lib/rpcconfig.cpp b/nano/lib/rpcconfig.cpp index f34f2bd5b9..da6fbc1a79 100644 --- a/nano/lib/rpcconfig.cpp +++ b/nano/lib/rpcconfig.cpp @@ -270,7 +270,7 @@ nano::error read_rpc_config_toml (boost::filesystem::path const & data_path_a, n } else { - toml.read (config_overrides_stream); + error = toml.read (config_overrides_stream); } } diff --git a/nano/lib/tomlconfig.cpp b/nano/lib/tomlconfig.cpp index 42bc938627..b574939878 100644 --- a/nano/lib/tomlconfig.cpp +++ b/nano/lib/tomlconfig.cpp @@ -23,10 +23,6 @@ void nano::tomlconfig::doc (std::string const & key, std::string const & doc) tree->document (key, doc); } -/** - * Reads a json object from the stream - * @return nano::error&, including a descriptive error message if the config file is malformed. - */ nano::error & nano::tomlconfig::read (boost::filesystem::path const & path_a) { std::stringstream stream_override_empty; @@ -40,34 +36,30 @@ nano::error & nano::tomlconfig::read (std::istream & stream_overrides, boost::fi open_or_create (stream, path_a.string ()); if (!stream.fail ()) { - try - { - read (stream_overrides, stream); - } - catch (std::runtime_error const & ex) - { - auto pos (stream.tellg ()); - if (pos != std::streampos (0)) - { - *error = ex; - } - } - stream.close (); + read (stream_overrides, stream); } return *error; } -/** Read from two streams where keys in the first will take precedence over those in the second stream. */ -void nano::tomlconfig::read (std::istream & stream_first_a, std::istream & stream_second_a) +nano::error & nano::tomlconfig::read (std::istream & stream_a) { - tree = cpptoml::parse_base_and_override_files (stream_first_a, stream_second_a, cpptoml::parser::merge_type::ignore, true); + std::stringstream stream_override_empty; + stream_override_empty << std::endl; + return read (stream_override_empty, stream_a); } -void nano::tomlconfig::read (std::istream & stream_a) +/** Read from two streams where keys in the first will take precedence over those in the second stream. */ +nano::error & nano::tomlconfig::read (std::istream & stream_first_a, std::istream & stream_second_a) { - std::stringstream stream_override_empty; - stream_override_empty << std::endl; - tree = cpptoml::parse_base_and_override_files (stream_override_empty, stream_a, cpptoml::parser::merge_type::ignore, true); + try + { + tree = cpptoml::parse_base_and_override_files (stream_first_a, stream_second_a, cpptoml::parser::merge_type::ignore, true); + } + catch (std::runtime_error const & ex) + { + *error = ex; + } + return *error; } void nano::tomlconfig::write (boost::filesystem::path const & path_a) diff --git a/nano/lib/tomlconfig.hpp b/nano/lib/tomlconfig.hpp index 0bfac87eb9..7f34944962 100644 --- a/nano/lib/tomlconfig.hpp +++ b/nano/lib/tomlconfig.hpp @@ -33,8 +33,8 @@ class tomlconfig : public nano::configbase void doc (std::string const & key, std::string const & doc); nano::error & read (boost::filesystem::path const & path_a); nano::error & read (std::istream & stream_overrides, boost::filesystem::path const & path_a); - void read (std::istream & stream_first_a, std::istream & stream_second_a); - void read (std::istream & stream_a); + nano::error & read (std::istream & stream_a); + nano::error & read (std::istream & stream_first_a, std::istream & stream_second_a); void write (boost::filesystem::path const & path_a); void write (std::ostream & stream_a) const; void open_or_create (std::fstream & stream_a, std::string const & path_a); diff --git a/nano/node/daemonconfig.cpp b/nano/node/daemonconfig.cpp index 2361d00c10..7b58ca4c85 100644 --- a/nano/node/daemonconfig.cpp +++ b/nano/node/daemonconfig.cpp @@ -223,7 +223,7 @@ nano::error nano::read_node_config_toml (boost::filesystem::path const & data_pa } else { - toml.read (config_overrides_stream); + error = toml.read (config_overrides_stream); } }