diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -100,12 +100,14 @@ "Fri, 30 Sep 2011 23:36:17 +0000"); } -class TestArgsManager : public ArgsManager { -public: - std::map &GetMapArgs() { return mapArgs; }; +struct TestArgsManager : public ArgsManager { + 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) { @@ -141,6 +143,53 @@ 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")); + + // 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); + 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_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; testArgs.GetMapArgs().clear(); diff --git a/src/util.h b/src/util.h --- a/src/util.h +++ b/src/util.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -96,6 +97,7 @@ 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[]); @@ -117,6 +119,15 @@ */ 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. * @@ -172,6 +183,10 @@ // Remove an arg setting, used only in testing void ClearArg(const std::string &strArg); + +private: + // Munge -nofoo into -foo=0 and track the value as negated. + void InterpretNegatedOption(std::string &key, std::string &val); }; extern ArgsManager gArgs; diff --git a/src/util.cpp b/src/util.cpp --- a/src/util.cpp +++ b/src/util.cpp @@ -76,8 +76,6 @@ #include #endif -#include // for to_lower() -#include // for startswith() and endswith() #include #include @@ -146,7 +144,23 @@ } } instance_of_cinit; -/** 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()) { return true; @@ -154,13 +168,31 @@ return (atoi(strValue) != 0); } -/** Turn -noX into -X=0 */ -static void InterpretNegativeSetting(std::string &strKey, - std::string &strValue) { - if (strKey.length() > 3 && strKey[0] == '-' && strKey[1] == 'n' && - strKey[2] == 'o') { - strKey = "-" + strKey.substr(3); - strValue = InterpretBool(strValue) ? "0" : "1"; +/** + * 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 (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); } } @@ -168,35 +200,37 @@ 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('='); + std::string key(argv[i]); + std::string val; + size_t is_index = key.find('='); if (is_index != std::string::npos) { - strValue = str.substr(is_index + 1); - str = str.substr(0, is_index); + 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); + // Transform --foo to -foo + if (key.length() > 1 && key[1] == '-') { + key.erase(0, 1); } - InterpretNegativeSetting(str, strValue); - mapArgs[str] = strValue; - mapMultiArgs[str].push_back(strValue); + // Transform -nofoo to -foo=0 + InterpretNegatedOption(key, val); + + mapArgs[key] = val; + mapMultiArgs[key].push_back(val); } } @@ -214,6 +248,11 @@ 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); @@ -423,7 +462,7 @@ // 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; }