Skip to content

Commit 91f1a25

Browse files
firewavefirewave
authored
fixed #12523 - greatly improved errorhandling of --rule-file= (#6147)
1 parent 14c24de commit 91f1a25

5 files changed

Lines changed: 159 additions & 28 deletions

File tree

‎cli/cmdlineparser.cpp‎

Lines changed: 37 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1071,44 +1071,57 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
10711071
// Rule file
10721072
else if (std::strncmp(argv[i], "--rule-file=", 12) == 0) {
10731073
#ifdef HAVE_RULES
1074-
// TODO: improved error handling - unknown elements, wrong root node, etc.
1074+
// TODO: improved error handling - wrong root node, etc.
1075+
// TODO: consume unused "version" attribute
10751076
const std::string ruleFile = argv[i] + 12;
10761077
tinyxml2::XMLDocument doc;
10771078
const tinyxml2::XMLError err = doc.LoadFile(ruleFile.c_str());
10781079
if (err == tinyxml2::XML_SUCCESS) {
10791080
const tinyxml2::XMLElement *node = doc.FirstChildElement();
1080-
// TODO: this looks like legacy handling - deprecate it
1081+
// check if it is a single or multi rule configuration
10811082
if (node && strcmp(node->Value(), "rules") == 0)
10821083
node = node->FirstChildElement("rule");
10831084
for (; node && strcmp(node->Value(), "rule") == 0; node = node->NextSiblingElement()) {
10841085
Settings::Rule rule;
10851086

1086-
const tinyxml2::XMLElement *tokenlist = node->FirstChildElement("tokenlist");
1087-
if (tokenlist)
1088-
rule.tokenlist = tokenlist->GetText();
1089-
1090-
const tinyxml2::XMLElement *pattern = node->FirstChildElement("pattern");
1091-
if (pattern) {
1092-
rule.pattern = pattern->GetText();
1087+
for (const tinyxml2::XMLElement *subnode = node->FirstChildElement(); subnode; subnode = subnode->NextSiblingElement()) {
1088+
const char * const subtext = subnode->GetText();
1089+
if (std::strcmp(subnode->Name(), "tokenlist") == 0) {
1090+
rule.tokenlist = empty_if_null(subtext);
1091+
}
1092+
else if (std::strcmp(subnode->Name(), "pattern") == 0) {
1093+
rule.pattern = empty_if_null(subtext);
1094+
}
1095+
else if (std::strcmp(subnode->Name(), "message") == 0) {
1096+
for (const tinyxml2::XMLElement *msgnode = subnode->FirstChildElement(); msgnode; msgnode = msgnode->NextSiblingElement()) {
1097+
const char * const msgtext = msgnode->GetText();
1098+
if (std::strcmp(msgnode->Name(), "severity") == 0) {
1099+
rule.severity = severityFromString(empty_if_null(msgtext));
1100+
}
1101+
else if (std::strcmp(msgnode->Name(), "id") == 0) {
1102+
rule.id = empty_if_null(msgtext);
1103+
}
1104+
else if (std::strcmp(msgnode->Name(), "summary") == 0) {
1105+
rule.summary = empty_if_null(msgtext);
1106+
}
1107+
else {
1108+
mLogger.printError("unable to load rule-file '" + ruleFile + "' - unknown element '" + msgnode->Name() + "' encountered in 'message'.");
1109+
return Result::Fail;
1110+
}
1111+
}
1112+
}
1113+
else {
1114+
mLogger.printError("unable to load rule-file '" + ruleFile + "' - unknown element '" + subnode->Name() + "' encountered in 'rule'.");
1115+
return Result::Fail;
1116+
}
10931117
}
10941118

1095-
const tinyxml2::XMLElement *message = node->FirstChildElement("message");
1096-
if (message) {
1097-
const tinyxml2::XMLElement *severity = message->FirstChildElement("severity");
1098-
if (severity)
1099-
rule.severity = severityFromString(severity->GetText());
1100-
1101-
const tinyxml2::XMLElement *id = message->FirstChildElement("id");
1102-
if (id)
1103-
rule.id = id->GetText();
1104-
1105-
const tinyxml2::XMLElement *summary = message->FirstChildElement("summary");
1106-
if (summary)
1107-
rule.summary = summary->GetText() ? summary->GetText() : "";
1119+
if (rule.pattern.empty()) {
1120+
mLogger.printError("unable to load rule-file '" + ruleFile + "' - a rule is lacking a pattern.");
1121+
return Result::Fail;
11081122
}
11091123

1110-
if (!rule.pattern.empty())
1111-
mSettings.rules.emplace_back(std::move(rule));
1124+
mSettings.rules.emplace_back(std::move(rule));
11121125
}
11131126
} else {
11141127
mLogger.printError("unable to load rule-file '" + ruleFile + "' (" + tinyxml2::XMLDocument::ErrorIDToName(err) + ").");

‎lib/utils.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,4 +361,10 @@ namespace cppcheck
361361
}
362362
}
363363

364+
template<typename T>
365+
static inline T* empty_if_null(T* p)
366+
{
367+
return p ? p : "";
368+
}
369+
364370
#endif

‎releasenotes.txt‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,6 @@ Other:
2424
- Removed deprecated platform type 'Unspecified'. Please use 'unspecified' instead.
2525
- Removed deprecated 'Makefile' option 'SRCDIR'.
2626
- Added CMake option 'DISALLOW_THREAD_EXECUTOR' to control the inclusion of the executor which performs the analysis within a thread of the main process.
27-
- Removed CMake option 'USE_THREADS' in favor of 'DISALLOW_THREAD_EXECUTOR'.
27+
- Removed CMake option 'USE_THREADS' in favor of 'DISALLOW_THREAD_EXECUTOR'.
28+
- Fixed crash with '--rule-file=' if some data was missing.
29+
- '--rule-file' will now bail out if a rule could not be added or a file contains unexpected data.

‎test/fixture.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ class TestFixture : public ErrorLogger {
280280
};
281281

282282
// TODO: most asserts do not actually assert i.e. do not return
283-
#define TEST_CASE( NAME ) do { if (prepareTest(#NAME)) { setVerbose(false); NAME(); teardownTest(); } } while (false)
283+
#define TEST_CASE( NAME ) do { if (prepareTest(#NAME)) { setVerbose(false); try { NAME(); teardownTest(); } catch (...) { assertNoThrowFail(__FILE__, __LINE__); } } } while (false)
284284
#define ASSERT( CONDITION ) if (!assert_(__FILE__, __LINE__, (CONDITION))) return
285285
#define ASSERT_LOC( CONDITION, FILE_, LINE_ ) assert_(FILE_, LINE_, (CONDITION))
286286
#define CHECK_EQUALS( EXPECTED, ACTUAL ) assertEquals(__FILE__, __LINE__, (EXPECTED), (ACTUAL))

‎test/testcmdlineparser.cpp‎

Lines changed: 112 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -334,11 +334,16 @@ class TestCmdlineParser : public TestFixture {
334334
TEST_CASE(ruleNotSupported);
335335
#endif
336336
#ifdef HAVE_RULES
337-
TEST_CASE(ruleFile);
337+
TEST_CASE(ruleFileMulti);
338+
TEST_CASE(ruleFileSingle);
338339
TEST_CASE(ruleFileEmpty);
339340
TEST_CASE(ruleFileMissing);
340341
TEST_CASE(ruleFileInvalid);
341342
TEST_CASE(ruleFileNoRoot);
343+
TEST_CASE(ruleFileEmptyElements1);
344+
TEST_CASE(ruleFileEmptyElements2);
345+
TEST_CASE(ruleFileUnknownElement1);
346+
TEST_CASE(ruleFileUnknownElement2);
342347
#else
343348
TEST_CASE(ruleFileNotSupported);
344349
#endif
@@ -2147,19 +2152,67 @@ class TestCmdlineParser : public TestFixture {
21472152
#endif
21482153

21492154
#ifdef HAVE_RULES
2150-
void ruleFile() {
2155+
void ruleFileMulti() {
21512156
REDIRECT;
21522157
ScopedFile file("rule.xml",
21532158
"<rules>\n"
21542159
"<rule>\n"
2160+
"<tokenlist>toklist1</tokenlist>\n"
21552161
"<pattern>.+</pattern>\n"
2162+
"<message>\n"
2163+
"<severity>error</severity>\n"
2164+
"<id>ruleId1</id>\n"
2165+
"<summary>ruleSummary1</summary>\n"
2166+
"</message>\n"
2167+
"</rule>\n"
2168+
"<rule>\n"
2169+
"<tokenlist>toklist2</tokenlist>\n"
2170+
"<pattern>.*</pattern>\n"
2171+
"<message>\n"
2172+
"<severity>warning</severity>\n"
2173+
"<id>ruleId2</id>\n"
2174+
"<summary>ruleSummary2</summary>\n"
2175+
"</message>\n"
21562176
"</rule>\n"
21572177
"</rules>");
21582178
const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
21592179
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv));
2180+
ASSERT_EQUALS(2, settings->rules.size());
2181+
auto it = settings->rules.cbegin();
2182+
ASSERT_EQUALS("toklist1", it->tokenlist);
2183+
ASSERT_EQUALS(".+", it->pattern);
2184+
ASSERT_EQUALS_ENUM(Severity::error, it->severity);
2185+
ASSERT_EQUALS("ruleId1", it->id);
2186+
ASSERT_EQUALS("ruleSummary1", it->summary);
2187+
++it;
2188+
ASSERT_EQUALS("toklist2", it->tokenlist);
2189+
ASSERT_EQUALS(".*", it->pattern);
2190+
ASSERT_EQUALS_ENUM(Severity::warning, it->severity);
2191+
ASSERT_EQUALS("ruleId2", it->id);
2192+
ASSERT_EQUALS("ruleSummary2", it->summary);
2193+
}
2194+
2195+
void ruleFileSingle() {
2196+
REDIRECT;
2197+
ScopedFile file("rule.xml",
2198+
"<rule>\n"
2199+
"<tokenlist>toklist</tokenlist>\n"
2200+
"<pattern>.+</pattern>\n"
2201+
"<message>\n"
2202+
"<severity>error</severity>\n"
2203+
"<id>ruleId</id>\n"
2204+
"<summary>ruleSummary</summary>\n"
2205+
"</message>\n"
2206+
"</rule>\n");
2207+
const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
2208+
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv));
21602209
ASSERT_EQUALS(1, settings->rules.size());
21612210
auto it = settings->rules.cbegin();
2211+
ASSERT_EQUALS("toklist", it->tokenlist);
21622212
ASSERT_EQUALS(".+", it->pattern);
2213+
ASSERT_EQUALS_ENUM(Severity::error, it->severity);
2214+
ASSERT_EQUALS("ruleId", it->id);
2215+
ASSERT_EQUALS("ruleSummary", it->summary);
21632216
}
21642217

21652218
void ruleFileEmpty() {
@@ -2189,6 +2242,63 @@ class TestCmdlineParser : public TestFixture {
21892242
ScopedFile file("rule.xml", "<?xml version=\"1.0\"?>");
21902243
const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
21912244
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv));
2245+
ASSERT_EQUALS(0, settings->rules.size());
2246+
}
2247+
2248+
void ruleFileEmptyElements1() {
2249+
REDIRECT;
2250+
ScopedFile file("rule.xml",
2251+
"<rule>"
2252+
"<tokenlist/>"
2253+
"<pattern/>"
2254+
"<message/>"
2255+
"</rule>"
2256+
);
2257+
const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
2258+
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv)); // do not crash
2259+
ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule is lacking a pattern.\n", logger->str());
2260+
}
2261+
2262+
void ruleFileEmptyElements2() {
2263+
REDIRECT;
2264+
ScopedFile file("rule.xml",
2265+
"<rule>"
2266+
"<message>"
2267+
"<severity/>"
2268+
"<id/>"
2269+
"<summary/>"
2270+
"</message>"
2271+
"</rule>"
2272+
);
2273+
const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
2274+
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv)); // do not crash
2275+
ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule is lacking a pattern.\n", logger->str());
2276+
}
2277+
2278+
void ruleFileUnknownElement1() {
2279+
REDIRECT;
2280+
ScopedFile file("rule.xml",
2281+
"<rule>"
2282+
"<messages/>"
2283+
"</rule>"
2284+
);
2285+
const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
2286+
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
2287+
ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - unknown element 'messages' encountered in 'rule'.\n", logger->str());
2288+
}
2289+
2290+
void ruleFileUnknownElement2() {
2291+
REDIRECT;
2292+
ScopedFile file("rule.xml",
2293+
"<rule>"
2294+
"<message>"
2295+
"<pattern/>"
2296+
"</message>"
2297+
"</rule>"
2298+
);
2299+
const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"};
2300+
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
2301+
ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - unknown element 'pattern' encountered in 'message'.\n", logger->str());
21922302
}
21932303
#else
21942304
void ruleFileNotSupported() {

0 commit comments

Comments
 (0)