From 1196696ae2a6a2b30934135661e99ea16e10a6d5 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 19 Dec 2019 18:00:04 -0500 Subject: [PATCH] Fix nonsensical -norpcwhitelist, -norpcallowip and related behavior This change fixes some corner cases handling negated list options: -norpcwhitelist, -norpcallowip, -norpcbind, -nobind, -nowhitebind, -noconnect, -noexternalip, -noonlynet, and -nosignetchalleng. Negating these options is now the same as not specifying them at all. This is useful for being able to override config file options on the command line without seeing parameter interaction side effects from the otherwise ignored config options. The code change here is just avoid calling the IsArgSet() function on ALLOW_LIST options, and to disallow such calls in the future. Code that uses IsArgSet() with list options is confusing and leads to mistakes due to the easy to overlook case where an argument is negated and IsArgSet() returns true, but GetArgs() returns an empty list. This change includes release notes, but the release notes don't go into details about specific options. For reference this change: - Treats specifying -norpcwhitelist exactly the same as not specifying any -rpcwhitelist, instead of behaving almost the same but flipping the default -rpcwhitelistdefault value. - Treats specifying -norpcallowip and -norpcbind exactly the same as not specifying -rpcallowip or -rpcbind, instead of failing to bind to localhost and failing to show warnings when one value is set without the other. - Treats specifying -nobind, -nowhitebind, and -noconnect exactly the same as not specifying -bind, -whitebind, or -connect values instead of treating them almost the same but causing parameter interactions with -dnsseed, -listen, and m_use_addrman_outgoing values. - Treats specifying -noexternalip exactly the same as not specifying any -externalip, instead of treating it almost the same but interacting with the -discover value. - Treats specifying -noonlynet exactly the same as not specifying -onlynet instead of marking all networks unreachable. - Treats specifying -nosignetchallenge exactly the same as not specifying -signetchallenge instead of throwing strange error "-signetchallenge cannot be multiple values" - Clarifies -vbparams and -debug handling code and fixes misleading comments without changing behavior. --- doc/release-notes-17783.md | 4 ++++ src/chainparams.cpp | 10 ++++++++-- src/httprpc.cpp | 2 +- src/httpserver.cpp | 12 ++++++++---- src/init.cpp | 19 +++++++++---------- src/test/util_tests.cpp | 18 +++++++++++++----- src/util/system.cpp | 13 ++++++++++++- 7 files changed, 55 insertions(+), 23 deletions(-) create mode 100644 doc/release-notes-17783.md diff --git a/doc/release-notes-17783.md b/doc/release-notes-17783.md new file mode 100644 index 0000000000000..6f6f10b697f76 --- /dev/null +++ b/doc/release-notes-17783.md @@ -0,0 +1,4 @@ +Configuration +------------- + +Some corner cases handling negated list options `-norpcallowip`, `-norpcbind`, `-nobind`, `-nowhitebind`, `-noconnect`, `-noexternalip`, `-noonlynet`, `-nosignetchallenge` have been fixed. Now negating these options is the same as not specifying them at all. diff --git a/src/chainparams.cpp b/src/chainparams.cpp index 3e778838562d6..a16c25d392891 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -274,7 +274,15 @@ class SigNetParams : public CChainParams { std::vector bin; vSeeds.clear(); +<<<<<<< HEAD if (!args.IsArgSet("-signetchallenge")) { +||||||| merged common ancestors + if (!args.IsArgSet("-signetchallenge")) { + LogPrintf("Using default signet network\n"); +======= + if (args.GetArgs("-signetchallenge").empty()) { + LogPrintf("Using default signet network\n"); +>>>>>>> Fix nonsensical -norpcwhitelist, -norpcallowip and related behavior bin = ParseHex("512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae"); vSeeds.emplace_back("178.128.221.177"); vSeeds.emplace_back("2a01:7c8:d005:390::5"); @@ -488,8 +496,6 @@ void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args) consensus.SegwitHeight = static_cast(height); } - if (!args.IsArgSet("-vbparams")) return; - for (const std::string& strDeployment : args.GetArgs("-vbparams")) { std::vector vDeploymentParams; boost::split(vDeploymentParams, strDeployment, boost::is_any_of(":")); diff --git a/src/httprpc.cpp b/src/httprpc.cpp index a0f1d40918b11..e74d89261d122 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -266,7 +266,7 @@ static bool InitRPCAuthentication() } } - g_rpc_whitelist_default = gArgs.GetBoolArg("-rpcwhitelistdefault", gArgs.IsArgSet("-rpcwhitelist")); + g_rpc_whitelist_default = gArgs.GetBoolArg("-rpcwhitelistdefault", gArgs.GetArgs("-rpcwhitelist").size() > 0); for (const std::string& strRPCWhitelist : gArgs.GetArgs("-rpcwhitelist")) { auto pos = strRPCWhitelist.find(':'); std::string strUser = strRPCWhitelist.substr(0, pos); diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 12395f5b24025..383ac0cb5080c 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -294,16 +294,20 @@ static bool HTTPBindAddresses(struct evhttp* http) std::vector> endpoints; // Determine what addresses to bind to - if (!(gArgs.IsArgSet("-rpcallowip") && gArgs.IsArgSet("-rpcbind"))) { // Default to loopback if not allowing external IPs + // To prevent misconfiguration and accidental exposure of the RPC + // interface, require -rpcallowip and -rpcbind to both be specified + // together. If either is missing, ignore both values, bind to localhost + // instead, and log warnings. + if (gArgs.GetArgs("-rpcallowip").size() == 0 || gArgs.GetArgs("-rpcbind").size() == 0) { // Default to loopback if not allowing external IPs endpoints.push_back(std::make_pair("::1", http_port)); endpoints.push_back(std::make_pair("127.0.0.1", http_port)); - if (gArgs.IsArgSet("-rpcallowip")) { + if (gArgs.GetArgs("-rpcallowip").size() > 0) { LogPrintf("WARNING: option -rpcallowip was specified without -rpcbind; this doesn't usually make sense\n"); } - if (gArgs.IsArgSet("-rpcbind")) { + if (gArgs.GetArgs("-rpcbind").size() > 0) { LogPrintf("WARNING: option -rpcbind was ignored because -rpcallowip was not specified, refusing to allow everyone to connect\n"); } - } else if (gArgs.IsArgSet("-rpcbind")) { // Specific bind address + } else { // Specific bind addresses for (const std::string& strRPCBind : gArgs.GetArgs("-rpcbind")) { uint16_t port{http_port}; std::string host; diff --git a/src/init.cpp b/src/init.cpp index bac3a98c6064e..84be3d21edcd5 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -809,16 +809,16 @@ void InitParameterInteraction(ArgsManager& args) { // when specifying an explicit binding address, you want to listen on it // even when -connect or -proxy is specified - if (args.IsArgSet("-bind")) { + if (args.GetArgs("-bind").size() > 0) { if (args.SoftSetBoolArg("-listen", true)) LogPrintf("%s: parameter interaction: -bind set -> setting -listen=1\n", __func__); } - if (args.IsArgSet("-whitebind")) { + if (args.GetArgs("-whitebind").size() > 0) { if (args.SoftSetBoolArg("-listen", true)) LogPrintf("%s: parameter interaction: -whitebind set -> setting -listen=1\n", __func__); } - if (args.IsArgSet("-connect")) { + if (args.GetArgs("-connect").size() > 0) { // when only connecting to trusted nodes, do not seed via DNS, or listen by default if (args.SoftSetBoolArg("-dnsseed", false)) LogPrintf("%s: parameter interaction: -connect set -> setting -dnsseed=0\n", __func__); @@ -858,7 +858,7 @@ void InitParameterInteraction(ArgsManager& args) } } - if (args.IsArgSet("-externalip")) { + if (args.GetArgs("-externalip").size() > 0) { // if an explicit public IP is specified, do not try to find others if (args.SoftSetBoolArg("-discover", false)) LogPrintf("%s: parameter interaction: -externalip set -> setting -discover=0\n", __func__); @@ -1071,10 +1071,9 @@ bool AppInitParameterInteraction(const ArgsManager& args) InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections)); // ********************************************************* Step 3: parameter-to-internal-flags - if (args.IsArgSet("-debug")) { - // Special-case: if -debug=0/-nodebug is set, turn off debugging messages - const std::vector categories = args.GetArgs("-debug"); - + const std::vector categories = args.GetArgs("-debug"); + if (!categories.empty()) { + // Special-case: if -debug=0/-debug=none is set, turn off debugging messages if (std::none_of(categories.begin(), categories.end(), [](std::string cat){return cat == "0" || cat == "none";})) { for (const auto& cat : categories) { @@ -1441,7 +1440,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) strSubVersion.size(), MAX_SUBVERSION_LENGTH)); } - if (args.IsArgSet("-onlynet")) { + if (args.GetArgs("-onlynet").size() > 0) { std::set nets; for (const std::string& snet : args.GetArgs("-onlynet")) { enum Network net = ParseNetwork(snet); @@ -1994,7 +1993,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) connOptions.vSeedNodes = args.GetArgs("-seednode"); // Initiate outbound connections unless connect=0 - connOptions.m_use_addrman_outgoing = !args.IsArgSet("-connect"); + connOptions.m_use_addrman_outgoing = args.GetArgs("-connect").empty(); if (!connOptions.m_use_addrman_outgoing) { const auto connect = args.GetArgs("-connect"); if (connect.size() != 1 || connect[0] != "0") { diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 9b6fc73373e2f..9d177f8ad6aeb 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -256,6 +256,14 @@ struct TestArgsManager : public ArgsManager if (test.arg) test.arg->m_flags &= ~ALLOW_LIST; return GetBoolArg(name, default_value); } + //! Call IsArgSet(), temporarily disabling ALLOW_LIST so call can succeed. + //! This is called by old tests written before ALLOW_LIST was enforced. + bool TestArgSet(const std::string& name) + { + TestFlags test(*this, name); + if (test.arg) test.arg->m_flags &= ~ALLOW_LIST; + return IsArgSet(name); + } using ArgsManager::GetSetting; using ArgsManager::GetSettingsList; using ArgsManager::ReadConfigStream; @@ -538,7 +546,7 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters) // -a, -b and -ccc end up in map, -d ignored because it is after // a non-option argument (non-GNU option parsing) BOOST_CHECK(testArgs.m_settings.command_line_options.size() == 3 && testArgs.m_settings.ro_config.empty()); - BOOST_CHECK(testArgs.IsArgSet("-a") && testArgs.IsArgSet("-b") && testArgs.IsArgSet("-ccc") + BOOST_CHECK(testArgs.IsArgSet("-a") && testArgs.IsArgSet("-b") && testArgs.TestArgSet("-ccc") && !testArgs.IsArgSet("f") && !testArgs.IsArgSet("-d")); BOOST_CHECK(testArgs.m_settings.command_line_options.count("a") && testArgs.m_settings.command_line_options.count("b") && testArgs.m_settings.command_line_options.count("ccc") && !testArgs.m_settings.command_line_options.count("f") && !testArgs.m_settings.command_line_options.count("d")); @@ -783,12 +791,12 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream) BOOST_CHECK(test_args.IsArgSet("-a")); BOOST_CHECK(test_args.IsArgSet("-b")); - BOOST_CHECK(test_args.IsArgSet("-ccc")); + BOOST_CHECK(test_args.TestArgSet("-ccc")); BOOST_CHECK(test_args.IsArgSet("-d")); BOOST_CHECK(test_args.IsArgSet("-fff")); BOOST_CHECK(test_args.IsArgSet("-ggg")); - BOOST_CHECK(test_args.IsArgSet("-h")); - BOOST_CHECK(test_args.IsArgSet("-i")); + BOOST_CHECK(test_args.TestArgSet("-h")); + BOOST_CHECK(test_args.TestArgSet("-i")); BOOST_CHECK(!test_args.IsArgSet("-zzz")); BOOST_CHECK(!test_args.IsArgSet("-iii")); @@ -1168,7 +1176,7 @@ BOOST_FIXTURE_TEST_CASE(util_ArgsMerge, ArgsMergeTestingSetup) desc += " || "; - if (!parser.IsArgSet(key)) { + if (!parser.TestArgSet(key)) { desc += "unset"; BOOST_CHECK(!parser.IsArgNegated(key)); BOOST_CHECK_EQUAL(parser.TestArgString(key, "default"), "default"); diff --git a/src/util/system.cpp b/src/util/system.cpp index ac0063ffc8a25..499ea6325fc2a 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -487,6 +487,17 @@ std::vector ArgsManager::GetArgs(const std::string& strArg) const bool ArgsManager::IsArgSet(const std::string& strArg) const { + // Don't allow IsArgSet() to be called on list arguments, because the odd + // case where an argument is negated and IsArgSet() returns true but + // GetArgs() returns an empty list has resulted in confusing code and bugs. + // + // In most cases it's best to treat empty lists and negated lists the same. + // In cases where it's useful treat them differently (cases where the + // default list value is effectively non-empty), code is less confusing if + // it explicity calls IsArgNegated() to distinguish the negated and empty + // conditions instead of IsArgSet() which makes the distinction more + // indirectly. + CheckArgFlags(strArg, /* require= */ 0, /* forbid= */ ALLOW_LIST, __func__); return !GetSetting(strArg).isNull(); } @@ -602,7 +613,7 @@ bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strValue) { LOCK(cs_args); - if (IsArgSet(strArg)) return false; + if (!GetSetting(strArg).isNull()) return false; ForceSetArg(strArg, strValue); return true; }