Skip to content

Commit

Permalink
refactor: Implement missing error checking for ArgsManager flags
Browse files Browse the repository at this point in the history 
Trigger startup errors if bitcoin is configured with bad setting values
according to flags. Also raise internal errors if settings are registered and
retrieved with inconsistent flags.

This change has no effect on behavior because ArgsManager flags were recently
added in bitcoin#16097 and aren't used anywhere yet.
  • Loading branch information
@ryanofsky
ryanofsky committed Sep 1, 2020
1 parent 48c1083 commit f801ea9
Show file tree
Hide file tree
Showing 5 changed files with 339 additions and 74 deletions.
37 changes: 31 additions & 6 deletions src/test/fuzz/system.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ void test_one_input(const std::vector<uint8_t>& buffer)
break;
}
case 1: {
args_manager.SoftSetArg(fuzzed_data_provider.ConsumeRandomLengthString(16), fuzzed_data_provider.ConsumeRandomLengthString(16));
// Avoid Can't call SoftSetBoolArg on arg registered with flags 0x8d8d8d00 (requires 0x2, disallows 0x10)
try {
args_manager.SoftSetArg(fuzzed_data_provider.ConsumeRandomLengthString(16), fuzzed_data_provider.ConsumeRandomLengthString(16));
} catch (const std::logic_error&) {
}
break;
}
case 2: {
Expand All @@ -57,7 +61,16 @@ void test_one_input(const std::vector<uint8_t>& buffer)
if (args_manager.GetArgFlags(argument_name) != nullopt) {
break;
}
args_manager.AddArg(argument_name, fuzzed_data_provider.ConsumeRandomLengthString(16), fuzzed_data_provider.ConsumeIntegral<unsigned int>(), options_category);
unsigned int flags = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
// Avoid hitting "ALLOW_{BOOL|INT|STRING} flags would have no effect with ALLOW_ANY present (ALLOW_ANY disables validation)"
if (flags & ArgsManager::ALLOW_ANY) {
flags &= ~(ArgsManager::ALLOW_BOOL | ArgsManager::ALLOW_INT | ArgsManager::ALLOW_STRING);
}
// Avoid hitting "ALLOW_INT would have no effect with ALLOW_STRING present (any valid integer is also a valid string)"
if (flags & ArgsManager::ALLOW_STRING) {
flags &= ~ArgsManager::ALLOW_INT;
}
args_manager.AddArg(argument_name, fuzzed_data_provider.ConsumeRandomLengthString(16), flags, options_category);
break;
}
case 5: {
Expand Down Expand Up @@ -104,11 +117,23 @@ void test_one_input(const std::vector<uint8_t>& buffer)
const int64_t i64 = fuzzed_data_provider.ConsumeIntegral<int64_t>();
const bool b = fuzzed_data_provider.ConsumeBool();

(void)args_manager.GetArg(s1, i64);
(void)args_manager.GetArg(s1, s2);
try {
(void)args_manager.GetArg(s1, i64);
} catch (const std::logic_error&) {
}
try {
(void)args_manager.GetArg(s1, s2);
} catch (const std::logic_error&) {
}
(void)args_manager.GetArgFlags(s1);
(void)args_manager.GetArgs(s1);
(void)args_manager.GetBoolArg(s1, b);
try {
(void)args_manager.GetArgs(s1);
} catch (const std::logic_error&) {
}
try {
(void)args_manager.GetBoolArg(s1, b);
} catch (const std::logic_error&) {
}
try {
(void)args_manager.GetChainName();
} catch (const std::runtime_error&) {
Expand Down
2 changes: 1 addition & 1 deletion src/test/getarg_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ BOOST_AUTO_TEST_CASE(logargs)

LogInstance().DeleteCallback(print_connection);
// Check that what should appear does, and what shouldn't doesn't.
BOOST_CHECK(str.find("Command-line arg: okaylog-bool=\"\"") != std::string::npos);
BOOST_CHECK(str.find("Command-line arg: okaylog-bool=true") != std::string::npos);
BOOST_CHECK(str.find("Command-line arg: okaylog-negbool=false") != std::string::npos);
BOOST_CHECK(str.find("Command-line arg: okaylog=\"public\"") != std::string::npos);
BOOST_CHECK(str.find("dontlog=****") != std::string::npos);
Expand Down
149 changes: 140 additions & 9 deletions src/test/util_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ class CheckValueTest : public TestChain100Setup

if (expect.error) {
BOOST_CHECK(!success);
BOOST_CHECK_NE(error.find(expect.error), std::string::npos);
BOOST_CHECK_EQUAL(error, expect.error);
} else {
BOOST_CHECK(success);
BOOST_CHECK_EQUAL(error, "");
Expand All @@ -246,16 +246,16 @@ class CheckValueTest : public TestChain100Setup
BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), "zzzzz");
} else if (expect.string_value) {
BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), expect.string_value);
} else {
BOOST_CHECK(!success);
} else if (success) {
BOOST_CHECK_THROW(test.GetArg("-value", "zzzzz"), std::logic_error);
}

if (expect.default_int) {
BOOST_CHECK_EQUAL(test.GetArg("-value", 99999), 99999);
} else if (expect.int_value) {
BOOST_CHECK_EQUAL(test.GetArg("-value", 99999), *expect.int_value);
} else {
BOOST_CHECK(!success);
} else if (success) {
BOOST_CHECK_THROW(test.GetArg("-value", 99999), std::logic_error);
}

if (expect.default_bool) {
Expand All @@ -264,15 +264,16 @@ class CheckValueTest : public TestChain100Setup
} else if (expect.bool_value) {
BOOST_CHECK_EQUAL(test.GetBoolArg("-value", false), *expect.bool_value);
BOOST_CHECK_EQUAL(test.GetBoolArg("-value", true), *expect.bool_value);
} else {
BOOST_CHECK(!success);
} else if (success) {
BOOST_CHECK_THROW(test.GetBoolArg("-value", false), std::logic_error);
BOOST_CHECK_THROW(test.GetBoolArg("-value", true), std::logic_error);
}

if (expect.list_value) {
auto l = test.GetArgs("-value");
BOOST_CHECK_EQUAL_COLLECTIONS(l.begin(), l.end(), expect.list_value->begin(), expect.list_value->end());
} else {
BOOST_CHECK(!success);
} else if (success) {
BOOST_CHECK_THROW(test.GetArgs("-value"), std::logic_error);
}
}
};
Expand All @@ -294,6 +295,136 @@ BOOST_FIXTURE_TEST_CASE(util_CheckValue, CheckValueTest)
CheckValue(M::ALLOW_ANY, "-value=1", Expect{"1"}.String("1").Int(1).Bool(true).List({"1"}));
CheckValue(M::ALLOW_ANY, "-value=2", Expect{"2"}.String("2").Int(2).Bool(true).List({"2"}));
CheckValue(M::ALLOW_ANY, "-value=abc", Expect{"abc"}.String("abc").Int(0).Bool(false).List({"abc"}));

CheckValue(M::ALLOW_BOOL, nullptr, Expect{{}}.DefaultBool());
CheckValue(M::ALLOW_BOOL, "-novalue", Expect{false}.Bool(false));
CheckValue(M::ALLOW_BOOL, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_BOOL, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_BOOL, "-novalue=1", Expect{false}.Bool(false));
CheckValue(M::ALLOW_BOOL, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_BOOL, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_BOOL, "-value", Expect{true}.Bool(true));
CheckValue(M::ALLOW_BOOL, "-value=", Expect{""}.DefaultBool());
CheckValue(M::ALLOW_BOOL, "-value=0", Expect{false}.Bool(false));
CheckValue(M::ALLOW_BOOL, "-value=1", Expect{true}.Bool(true));
CheckValue(M::ALLOW_BOOL, "-value=2", Expect{{}}.Error("Can not set -value value to '2'. It must be set to 0 or 1."));
CheckValue(M::ALLOW_BOOL, "-value=abc", Expect{{}}.Error("Can not set -value value to 'abc'. It must be set to 0 or 1."));

CheckValue(M::ALLOW_INT, nullptr, Expect{{}}.DefaultInt().DefaultBool());
CheckValue(M::ALLOW_INT, "-novalue", Expect{false}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_INT, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_INT, "-novalue=1", Expect{false}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_INT, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_INT, "-value", Expect{{}}.Error("Can not set -value with no value. It must be set to an integer."));
CheckValue(M::ALLOW_INT, "-value=", Expect{""}.DefaultInt().DefaultBool());
CheckValue(M::ALLOW_INT, "-value=0", Expect{0}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT, "-value=1", Expect{1}.Int(1).Bool(true));
CheckValue(M::ALLOW_INT, "-value=2", Expect{2}.Int(2).Bool(true));
CheckValue(M::ALLOW_INT, "-value=abc", Expect{{}}.Error("Can not set -value value to 'abc'. It must be set to an integer."));

CheckValue(M::ALLOW_STRING, nullptr, Expect{{}}.DefaultString().DefaultBool());
CheckValue(M::ALLOW_STRING, "-novalue", Expect{false}.String("").Bool(false));
CheckValue(M::ALLOW_STRING, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_STRING, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_STRING, "-novalue=1", Expect{false}.String("").Bool(false));
CheckValue(M::ALLOW_STRING, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_STRING, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_STRING, "-value", Expect{{}}.Error("Can not set -value with no value. It must be set to a string."));
CheckValue(M::ALLOW_STRING, "-value=", Expect{""}.DefaultString().DefaultBool());
CheckValue(M::ALLOW_STRING, "-value=0", Expect{"0"}.String("0").DefaultBool());
CheckValue(M::ALLOW_STRING, "-value=1", Expect{"1"}.String("1").DefaultBool());
CheckValue(M::ALLOW_STRING, "-value=2", Expect{"2"}.String("2").DefaultBool());
CheckValue(M::ALLOW_STRING, "-value=abc", Expect{"abc"}.String("abc").DefaultBool());

CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, nullptr, Expect{{}}.DefaultInt().DefaultBool());
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue", Expect{false}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=1", Expect{false}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value", Expect{true}.Int(1).Bool(true));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=", Expect{""}.DefaultInt().DefaultBool());
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=0", Expect{0}.Int(0).Bool(false));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=1", Expect{1}.Int(1).Bool(true));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=2", Expect{2}.Int(2).Bool(true));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=abc", Expect{{}}.Error("Can not set -value value to 'abc'. It must be set to an integer."));

CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, nullptr, Expect{{}}.DefaultString().DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue", Expect{false}.String("").Bool(false));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=1", Expect{false}.String("").Bool(false));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value", Expect{true}.DefaultString().Bool(true));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=", Expect{""}.DefaultString().DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=0", Expect{"0"}.String("0").DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=1", Expect{"1"}.String("1").DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=2", Expect{"2"}.String("2").DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=abc", Expect{"abc"}.String("abc").DefaultBool());

CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, nullptr, Expect{{}}.List({}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue", Expect{false}.List({}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=", Expect{{}}.Error("Can not negate -value at the same time as setting value ''."));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=0", Expect{{}}.Error("Can not negate -value at the same time as setting value '0'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=1", Expect{false}.List({}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=2", Expect{{}}.Error("Can not negate -value at the same time as setting value '2'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=abc", Expect{{}}.Error("Can not negate -value at the same time as setting value 'abc'."));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value", Expect{{}}.Error("Can not set -value with no value. It must be set to a string."));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=", Expect{""}.List({""}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=0", Expect{"0"}.List({"0"}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=1", Expect{"1"}.List({"1"}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=2", Expect{"2"}.List({"2"}));
CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=abc", Expect{"abc"}.List({"abc"}));
}

BOOST_FIXTURE_TEST_CASE(util_CheckBoolStringsNotSpecial, CheckValueTest)
{
using M = ArgsManager;

// Check that "true" and "false" strings are rejected for ALLOW_BOOL
// arguments. We might want to change this behavior in the future and
// interpret strings like "true" as true, and strings like "false" as false.
// But because it would be confusing to interpret "true" as true for
// ALLOW_BOOL arguments but false for ALLOW_ANY arguments (because
// atoi("true")==0), for now it is safer to just disallow strings like
// "true" and "false" for ALLOW_BOOL arguments as long as there are still
// other boolean arguments interpreted with ALLOW_ANY.
CheckValue(M::ALLOW_BOOL, "-value=true", Expect{{}}.Error("Can not set -value value to 'true'. It must be set to 0 or 1."));
CheckValue(M::ALLOW_BOOL, "-value=false", Expect{{}}.Error("Can not set -value value to 'false'. It must be set to 0 or 1."));

// Similarly, check "true" and "false" are not treated specially when
// ALLOW_BOOL is combined with ALLOW_INT and ALLOW_STRING. (The only
// difference ALLOW_BOOL makes for int and string arguments is that it
// enables "-foo" syntax with no equal sign assigning explicit int or string
// values. This is useful for arguments like "-upgradewallet" or "-listen"
// that primarily toggle features on and off, but also accept optional int
// or string values to influence behavior.)
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=true", Expect{{}}.Error("Can not set -value value to 'true'. It must be set to an integer."));
CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=false", Expect{{}}.Error("Can not set -value value to 'false'. It must be set to an integer."));
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=true", Expect{"true"}.String("true").DefaultBool());
CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=false", Expect{"false"}.String("false").DefaultBool());
}

BOOST_AUTO_TEST_CASE(util_CheckSingleValue)
{
TestArgsManager test;
test.SetupArgs({{"-single", ArgsManager::ALLOW_INT}});
std::istringstream stream("single=1\nsingle=2\n");
std::string error;
BOOST_CHECK(!test.ReadConfigStream(stream, "file.conf", error));
BOOST_CHECK_EQUAL(error, "Multiple values specified for -single in same section of config file.");
}

BOOST_AUTO_TEST_CASE(util_CheckBadFlagCombinations)
{
TestArgsManager test;
using M = ArgsManager;
BOOST_CHECK_THROW(test.AddArg("-arg1", "name", M::ALLOW_INT | M::ALLOW_ANY, OptionsCategory::OPTIONS), std::logic_error);
BOOST_CHECK_THROW(test.AddArg("-arg2", "name", M::ALLOW_INT | M::ALLOW_STRING, OptionsCategory::OPTIONS), std::logic_error);
}

BOOST_AUTO_TEST_CASE(util_ParseParameters)
Expand Down
Loading

0 comments on commit f801ea9

Please sign in to comment.