From 91f1a254c481c4484f998ff1eb500e594a149a15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20St=C3=B6neberg?= Date: Thu, 21 Mar 2024 23:02:30 +0100 Subject: [PATCH] fixed #12523 - greatly improved errorhandling of `--rule-file=` (#6147) --- cli/cmdlineparser.cpp | 61 ++++++++++++-------- lib/utils.h | 6 ++ releasenotes.txt | 4 +- test/fixture.h | 2 +- test/testcmdlineparser.cpp | 114 ++++++++++++++++++++++++++++++++++++- 5 files changed, 159 insertions(+), 28 deletions(-) diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp index 7f275ab00df..59cf67847fa 100644 --- a/cli/cmdlineparser.cpp +++ b/cli/cmdlineparser.cpp @@ -1071,44 +1071,57 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a // Rule file else if (std::strncmp(argv[i], "--rule-file=", 12) == 0) { #ifdef HAVE_RULES - // TODO: improved error handling - unknown elements, wrong root node, etc. + // TODO: improved error handling - wrong root node, etc. + // TODO: consume unused "version" attribute const std::string ruleFile = argv[i] + 12; tinyxml2::XMLDocument doc; const tinyxml2::XMLError err = doc.LoadFile(ruleFile.c_str()); if (err == tinyxml2::XML_SUCCESS) { const tinyxml2::XMLElement *node = doc.FirstChildElement(); - // TODO: this looks like legacy handling - deprecate it + // check if it is a single or multi rule configuration if (node && strcmp(node->Value(), "rules") == 0) node = node->FirstChildElement("rule"); for (; node && strcmp(node->Value(), "rule") == 0; node = node->NextSiblingElement()) { Settings::Rule rule; - const tinyxml2::XMLElement *tokenlist = node->FirstChildElement("tokenlist"); - if (tokenlist) - rule.tokenlist = tokenlist->GetText(); - - const tinyxml2::XMLElement *pattern = node->FirstChildElement("pattern"); - if (pattern) { - rule.pattern = pattern->GetText(); + for (const tinyxml2::XMLElement *subnode = node->FirstChildElement(); subnode; subnode = subnode->NextSiblingElement()) { + const char * const subtext = subnode->GetText(); + if (std::strcmp(subnode->Name(), "tokenlist") == 0) { + rule.tokenlist = empty_if_null(subtext); + } + else if (std::strcmp(subnode->Name(), "pattern") == 0) { + rule.pattern = empty_if_null(subtext); + } + else if (std::strcmp(subnode->Name(), "message") == 0) { + for (const tinyxml2::XMLElement *msgnode = subnode->FirstChildElement(); msgnode; msgnode = msgnode->NextSiblingElement()) { + const char * const msgtext = msgnode->GetText(); + if (std::strcmp(msgnode->Name(), "severity") == 0) { + rule.severity = severityFromString(empty_if_null(msgtext)); + } + else if (std::strcmp(msgnode->Name(), "id") == 0) { + rule.id = empty_if_null(msgtext); + } + else if (std::strcmp(msgnode->Name(), "summary") == 0) { + rule.summary = empty_if_null(msgtext); + } + else { + mLogger.printError("unable to load rule-file '" + ruleFile + "' - unknown element '" + msgnode->Name() + "' encountered in 'message'."); + return Result::Fail; + } + } + } + else { + mLogger.printError("unable to load rule-file '" + ruleFile + "' - unknown element '" + subnode->Name() + "' encountered in 'rule'."); + return Result::Fail; + } } - const tinyxml2::XMLElement *message = node->FirstChildElement("message"); - if (message) { - const tinyxml2::XMLElement *severity = message->FirstChildElement("severity"); - if (severity) - rule.severity = severityFromString(severity->GetText()); - - const tinyxml2::XMLElement *id = message->FirstChildElement("id"); - if (id) - rule.id = id->GetText(); - - const tinyxml2::XMLElement *summary = message->FirstChildElement("summary"); - if (summary) - rule.summary = summary->GetText() ? summary->GetText() : ""; + if (rule.pattern.empty()) { + mLogger.printError("unable to load rule-file '" + ruleFile + "' - a rule is lacking a pattern."); + return Result::Fail; } - if (!rule.pattern.empty()) - mSettings.rules.emplace_back(std::move(rule)); + mSettings.rules.emplace_back(std::move(rule)); } } else { mLogger.printError("unable to load rule-file '" + ruleFile + "' (" + tinyxml2::XMLDocument::ErrorIDToName(err) + ")."); diff --git a/lib/utils.h b/lib/utils.h index 481d9e890b3..22b511c90c8 100644 --- a/lib/utils.h +++ b/lib/utils.h @@ -361,4 +361,10 @@ namespace cppcheck } } +template +static inline T* empty_if_null(T* p) +{ + return p ? p : ""; +} + #endif diff --git a/releasenotes.txt b/releasenotes.txt index 65c9e9ef90a..f68333b7075 100644 --- a/releasenotes.txt +++ b/releasenotes.txt @@ -24,4 +24,6 @@ Other: - Removed deprecated platform type 'Unspecified'. Please use 'unspecified' instead. - Removed deprecated 'Makefile' option 'SRCDIR'. - Added CMake option 'DISALLOW_THREAD_EXECUTOR' to control the inclusion of the executor which performs the analysis within a thread of the main process. -- Removed CMake option 'USE_THREADS' in favor of 'DISALLOW_THREAD_EXECUTOR'. \ No newline at end of file +- Removed CMake option 'USE_THREADS' in favor of 'DISALLOW_THREAD_EXECUTOR'. +- Fixed crash with '--rule-file=' if some data was missing. +- '--rule-file' will now bail out if a rule could not be added or a file contains unexpected data. \ No newline at end of file diff --git a/test/fixture.h b/test/fixture.h index 7527f3261bd..af227609363 100644 --- a/test/fixture.h +++ b/test/fixture.h @@ -280,7 +280,7 @@ class TestFixture : public ErrorLogger { }; // TODO: most asserts do not actually assert i.e. do not return -#define TEST_CASE( NAME ) do { if (prepareTest(#NAME)) { setVerbose(false); NAME(); teardownTest(); } } while (false) +#define TEST_CASE( NAME ) do { if (prepareTest(#NAME)) { setVerbose(false); try { NAME(); teardownTest(); } catch (...) { assertNoThrowFail(__FILE__, __LINE__); } } } while (false) #define ASSERT( CONDITION ) if (!assert_(__FILE__, __LINE__, (CONDITION))) return #define ASSERT_LOC( CONDITION, FILE_, LINE_ ) assert_(FILE_, LINE_, (CONDITION)) #define CHECK_EQUALS( EXPECTED, ACTUAL ) assertEquals(__FILE__, __LINE__, (EXPECTED), (ACTUAL)) diff --git a/test/testcmdlineparser.cpp b/test/testcmdlineparser.cpp index 5437ca9f013..60a56efbccc 100644 --- a/test/testcmdlineparser.cpp +++ b/test/testcmdlineparser.cpp @@ -334,11 +334,16 @@ class TestCmdlineParser : public TestFixture { TEST_CASE(ruleNotSupported); #endif #ifdef HAVE_RULES - TEST_CASE(ruleFile); + TEST_CASE(ruleFileMulti); + TEST_CASE(ruleFileSingle); TEST_CASE(ruleFileEmpty); TEST_CASE(ruleFileMissing); TEST_CASE(ruleFileInvalid); TEST_CASE(ruleFileNoRoot); + TEST_CASE(ruleFileEmptyElements1); + TEST_CASE(ruleFileEmptyElements2); + TEST_CASE(ruleFileUnknownElement1); + TEST_CASE(ruleFileUnknownElement2); #else TEST_CASE(ruleFileNotSupported); #endif @@ -2147,19 +2152,67 @@ class TestCmdlineParser : public TestFixture { #endif #ifdef HAVE_RULES - void ruleFile() { + void ruleFileMulti() { REDIRECT; ScopedFile file("rule.xml", "\n" "\n" + "toklist1\n" ".+\n" + "\n" + "error\n" + "ruleId1\n" + "ruleSummary1\n" + "\n" + "\n" + "\n" + "toklist2\n" + ".*\n" + "\n" + "warning\n" + "ruleId2\n" + "ruleSummary2\n" + "\n" "\n" ""); const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"}; ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv)); + ASSERT_EQUALS(2, settings->rules.size()); + auto it = settings->rules.cbegin(); + ASSERT_EQUALS("toklist1", it->tokenlist); + ASSERT_EQUALS(".+", it->pattern); + ASSERT_EQUALS_ENUM(Severity::error, it->severity); + ASSERT_EQUALS("ruleId1", it->id); + ASSERT_EQUALS("ruleSummary1", it->summary); + ++it; + ASSERT_EQUALS("toklist2", it->tokenlist); + ASSERT_EQUALS(".*", it->pattern); + ASSERT_EQUALS_ENUM(Severity::warning, it->severity); + ASSERT_EQUALS("ruleId2", it->id); + ASSERT_EQUALS("ruleSummary2", it->summary); + } + + void ruleFileSingle() { + REDIRECT; + ScopedFile file("rule.xml", + "\n" + "toklist\n" + ".+\n" + "\n" + "error\n" + "ruleId\n" + "ruleSummary\n" + "\n" + "\n"); + const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"}; + ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv)); ASSERT_EQUALS(1, settings->rules.size()); auto it = settings->rules.cbegin(); + ASSERT_EQUALS("toklist", it->tokenlist); ASSERT_EQUALS(".+", it->pattern); + ASSERT_EQUALS_ENUM(Severity::error, it->severity); + ASSERT_EQUALS("ruleId", it->id); + ASSERT_EQUALS("ruleSummary", it->summary); } void ruleFileEmpty() { @@ -2189,6 +2242,63 @@ class TestCmdlineParser : public TestFixture { ScopedFile file("rule.xml", ""); const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"}; ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv)); + ASSERT_EQUALS(0, settings->rules.size()); + } + + void ruleFileEmptyElements1() { + REDIRECT; + ScopedFile file("rule.xml", + "" + "" + "" + "" + "" + ); + const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"}; + ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv)); // do not crash + ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule is lacking a pattern.\n", logger->str()); + } + + void ruleFileEmptyElements2() { + REDIRECT; + ScopedFile file("rule.xml", + "" + "" + "" + "" + "" + "" + "" + ); + const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"}; + ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv)); // do not crash + ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - a rule is lacking a pattern.\n", logger->str()); + } + + void ruleFileUnknownElement1() { + REDIRECT; + ScopedFile file("rule.xml", + "" + "" + "" + ); + const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"}; + ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv)); + ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - unknown element 'messages' encountered in 'rule'.\n", logger->str()); + } + + void ruleFileUnknownElement2() { + REDIRECT; + ScopedFile file("rule.xml", + "" + "" + "" + "" + "" + ); + const char * const argv[] = {"cppcheck", "--rule-file=rule.xml", "file.cpp"}; + ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv)); + ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' - unknown element 'pattern' encountered in 'message'.\n", logger->str()); } #else void ruleFileNotSupported() {