From 4f872b24501f40bd410227b5413bda2f2569af24 Mon Sep 17 00:00:00 2001 From: Evan Klitzke Date: Wed, 21 Mar 2018 19:22:52 -0700 Subject: [PATCH 1/2] Add additional tests for GetBoolArg() This is meant to be an intermediate commit to prove that the next does not introduce any changes in the semantics of boolean option parsing. --- src/test/util_tests.cpp | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index b6f3cbe2b..094bc66ac 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -223,6 +223,32 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters) BOOST_CHECK(testArgs.GetArgs("-ccc").size() == 2); } +BOOST_AUTO_TEST_CASE(util_GetBoolArg) +{ + TestArgsManager testArgs; + const char *argv_test[] = { + "ignored", "-a", "-nob", "-c=0", "-d=1", "-e=false", "-f=true"}; + testArgs.ParseParameters(7, (char**)argv_test); + + // Each letter should be set. + for (char opt : "abcdef") + BOOST_CHECK(testArgs.IsArgSet({'-', opt}) || !opt); + + // Nothing else should be in the map + BOOST_CHECK(testArgs.GetMapArgs().size() == 6 && + testArgs.GetMapMultiArgs().size() == 6); + + // The -no prefix should get stripped on the way in. + BOOST_CHECK(!testArgs.IsArgSet("-nob")); + + // Check expected values. + BOOST_CHECK(testArgs.GetBoolArg("-a", false) == true); + BOOST_CHECK(testArgs.GetBoolArg("-b", true) == false); + BOOST_CHECK(testArgs.GetBoolArg("-c", true) == false); + BOOST_CHECK(testArgs.GetBoolArg("-d", false) == true); + BOOST_CHECK(testArgs.GetBoolArg("-e", true) == false); + BOOST_CHECK(testArgs.GetBoolArg("-f", true) == false); +} BOOST_AUTO_TEST_CASE(util_GetArg) { TestArgsManager testArgs; From f7683cba7b070b722a2e0641f4d1516112392ed6 Mon Sep 17 00:00:00 2001 From: Evan Klitzke Date: Wed, 21 Mar 2018 19:24:17 -0700 Subject: [PATCH 2/2] Track negated arguments in the argument paser. This commit adds tracking for negated arguments. This change will be used in a future commit that allows disabling the debug.log file using -nodebuglogfile. --- src/test/util_tests.cpp | 36 ++++++++++----- src/util.cpp | 97 ++++++++++++++++++++++++++++------------- src/util.h | 17 ++++++++ 3 files changed, 110 insertions(+), 40 deletions(-) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 094bc66ac..053ec92ca 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -185,17 +185,11 @@ BOOST_AUTO_TEST_CASE(util_FormatISO8601Time) BOOST_CHECK_EQUAL(FormatISO8601Time(1317425777), "23:36:17Z"); } -class TestArgsManager : public ArgsManager +struct TestArgsManager : public ArgsManager { -public: - std::map& GetMapArgs() - { - return mapArgs; - }; - const std::map >& GetMapMultiArgs() - { - return mapMultiArgs; - }; + std::map& GetMapArgs() { return mapArgs; } + const std::map >& GetMapMultiArgs() { return mapMultiArgs; } + const std::unordered_set& GetNegatedArgs() { return m_negated_args; } }; BOOST_AUTO_TEST_CASE(util_ParseParameters) @@ -241,6 +235,11 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg) // The -no prefix should get stripped on the way in. BOOST_CHECK(!testArgs.IsArgSet("-nob")); + // The -b option is flagged as negated, and nothing else is + BOOST_CHECK(testArgs.IsArgNegated("-b")); + BOOST_CHECK(testArgs.GetNegatedArgs().size() == 1); + BOOST_CHECK(!testArgs.IsArgNegated("-a")); + // Check expected values. BOOST_CHECK(testArgs.GetBoolArg("-a", false) == true); BOOST_CHECK(testArgs.GetBoolArg("-b", true) == false); @@ -249,6 +248,23 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg) BOOST_CHECK(testArgs.GetBoolArg("-e", true) == false); BOOST_CHECK(testArgs.GetBoolArg("-f", true) == false); } + +BOOST_AUTO_TEST_CASE(util_GetBoolArgEdgeCases) +{ + // Test some awful edge cases that hopefully no user will ever exercise. + TestArgsManager testArgs; + const char *argv_test[] = {"ignored", "-nofoo", "-foo", "-nobar=0"}; + testArgs.ParseParameters(4, (char**)argv_test); + + // This was passed twice, second one overrides the negative setting. + BOOST_CHECK(!testArgs.IsArgNegated("-foo")); + BOOST_CHECK(testArgs.GetBoolArg("-foo", false) == true); + + // A double negative is a positive. + BOOST_CHECK(testArgs.IsArgNegated("-bar")); + BOOST_CHECK(testArgs.GetBoolArg("-bar", false) == true); +} + BOOST_AUTO_TEST_CASE(util_GetArg) { TestArgsManager testArgs; diff --git a/src/util.cpp b/src/util.cpp index dbf906511..490897899 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -70,8 +70,6 @@ #include #endif -#include // for to_lower() -#include // for startswith() and endswith() #include #include #include @@ -432,7 +430,23 @@ bool DirIsWritable(const fs::path& directory) return true; } -/** Interpret string as boolean, for argument parsing */ +/** + * Interpret a string argument as a boolean. + * + * The definition of atoi() requires that non-numeric string values like "foo", + * return 0. This means that if a user unintentionally supplies a non-integer + * argument here, the return value is always false. This means that -foo=false + * does what the user probably expects, but -foo=true is well defined but does + * not do what they probably expected. + * + * The return value of atoi() is undefined when given input not representable as + * an int. On most systems this means string value between "-2147483648" and + * "2147483647" are well defined (this method will return true). Setting + * -txindex=2147483648 on most systems, however, is probably undefined. + * + * For a more extensive discussion of this topic (and a wide range of opinions + * on the Right Way to change this code), see PR12713. + */ static bool InterpretBool(const std::string& strValue) { if (strValue.empty()) @@ -440,13 +454,30 @@ static bool InterpretBool(const std::string& strValue) return (atoi(strValue) != 0); } -/** Turn -noX into -X=0 */ -static void InterpretNegativeSetting(std::string& strKey, std::string& strValue) +/** + * Interpret -nofoo as if the user supplied -foo=0. + * + * This method also tracks when the -no form was supplied, and treats "-foo" as + * a negated option when this happens. This can be later checked using the + * IsArgNegated() method. One use case for this is to have a way to disable + * options that are not normally boolean (e.g. using -nodebuglogfile to request + * that debug log output is not sent to any file at all). + */ +void ArgsManager::InterpretNegatedOption(std::string& key, std::string& val) { - if (strKey.length()>3 && strKey[0]=='-' && strKey[1]=='n' && strKey[2]=='o') - { - strKey = "-" + strKey.substr(3); - strValue = InterpretBool(strValue) ? "0" : "1"; + if (key.substr(0, 3) == "-no") { + bool bool_val = InterpretBool(val); + if (!bool_val ) { + // Double negatives like -nofoo=0 are supported (but discouraged) + LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val); + } + key.erase(1, 2); + m_negated_args.insert(key); + val = bool_val ? "0" : "1"; + } else { + // In an invocation like "bitcoind -nofoo -foo" we want to unmark -foo + // as negated when we see the second option. + m_negated_args.erase(key); } } @@ -455,34 +486,34 @@ void ArgsManager::ParseParameters(int argc, const char* const argv[]) LOCK(cs_args); mapArgs.clear(); mapMultiArgs.clear(); + m_negated_args.clear(); - for (int i = 1; i < argc; i++) - { - std::string str(argv[i]); - std::string strValue; - size_t is_index = str.find('='); - if (is_index != std::string::npos) - { - strValue = str.substr(is_index+1); - str = str.substr(0, is_index); + for (int i = 1; i < argc; i++) { + std::string key(argv[i]); + std::string val; + size_t is_index = key.find('='); + if (is_index != std::string::npos) { + val = key.substr(is_index + 1); + key.erase(is_index); } #ifdef WIN32 - boost::to_lower(str); - if (boost::algorithm::starts_with(str, "/")) - str = "-" + str.substr(1); + std::transform(key.begin(), key.end(), key.begin(), ::tolower); + if (key[0] == '/') + key[0] = '-'; #endif - if (str[0] != '-') + if (key[0] != '-') break; - // Interpret --foo as -foo. - // If both --foo and -foo are set, the last takes effect. - if (str.length() > 1 && str[1] == '-') - str = str.substr(1); - InterpretNegativeSetting(str, strValue); + // Transform --foo to -foo + if (key.length() > 1 && key[1] == '-') + key.erase(0, 1); - mapArgs[str] = strValue; - mapMultiArgs[str].push_back(strValue); + // Transform -nofoo to -foo=0 + InterpretNegatedOption(key, val); + + mapArgs[key] = val; + mapMultiArgs[key].push_back(val); } } @@ -500,6 +531,12 @@ bool ArgsManager::IsArgSet(const std::string& strArg) const return mapArgs.count(strArg); } +bool ArgsManager::IsArgNegated(const std::string& strArg) const +{ + LOCK(cs_args); + return m_negated_args.find(strArg) != m_negated_args.end(); +} + std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const { LOCK(cs_args); @@ -711,7 +748,7 @@ void ArgsManager::ReadConfigFile(const std::string& confPath) // Don't overwrite existing settings so command line settings override bitcoin.conf std::string strKey = std::string("-") + it->string_key; std::string strValue = it->value[0]; - InterpretNegativeSetting(strKey, strValue); + InterpretNegatedOption(strKey, strValue); if (mapArgs.count(strKey) == 0) mapArgs[strKey] = strValue; mapMultiArgs[strKey].push_back(strValue); diff --git a/src/util.h b/src/util.h index 592041c0c..4c473c935 100644 --- a/src/util.h +++ b/src/util.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -224,6 +225,8 @@ protected: mutable CCriticalSection cs_args; std::map mapArgs; std::map> mapMultiArgs; + std::unordered_set m_negated_args; + public: void ParseParameters(int argc, const char*const argv[]); void ReadConfigFile(const std::string& confPath); @@ -244,6 +247,15 @@ public: */ bool IsArgSet(const std::string& strArg) const; + /** + * Return true if the argument was originally passed as a negated option, + * i.e. -nofoo. + * + * @param strArg Argument to get (e.g. "-foo") + * @return true if the argument was passed negated + */ + bool IsArgNegated(const std::string& strArg) const; + /** * Return string argument or default value * @@ -292,6 +304,11 @@ public: // Forces an arg setting. Called by SoftSetArg() if the arg hasn't already // been set. Also called directly in testing. void ForceSetArg(const std::string& strArg, const std::string& strValue); + +private: + + // Munge -nofoo into -foo=0 and track the value as negated. + void InterpretNegatedOption(std::string &key, std::string &val); }; extern ArgsManager gArgs;