fixed #12523 - greatly improved errorhandling of --rule-file= (#6147)

danmar · Mar 21, 2024 · 91f1a25 · 91f1a25
1 parent 14c24de
commit 91f1a25
Show file tree

Hide file tree

Showing 5 changed files with 159 additions and 28 deletions.
diff --git 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
@@ -361,4 +361,10 @@ namespace cppcheck
     }
 }
 
+template<typename T>
+static inline T* empty_if_null(T* p)
+{
+    return p ? p : "";
+}
+
 #endif
diff --git 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'.
+- 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.
diff --git 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
@@ -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",
                         "<rules>\n"
                         "<rule>\n"
+                        "<tokenlist>toklist1</tokenlist>\n"
                         "<pattern>.+</pattern>\n"
+                        "<message>\n"
+                        "<severity>error</severity>\n"
+                        "<id>ruleId1</id>\n"
+                        "<summary>ruleSummary1</summary>\n"
+                        "</message>\n"
+                        "</rule>\n"
+                        "<rule>\n"
+                        "<tokenlist>toklist2</tokenlist>\n"
+                        "<pattern>.*</pattern>\n"
+                        "<message>\n"
+                        "<severity>warning</severity>\n"
+                        "<id>ruleId2</id>\n"
+                        "<summary>ruleSummary2</summary>\n"
+                        "</message>\n"
                         "</rule>\n"
                         "</rules>");
         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",
+                        "<rule>\n"
+                        "<tokenlist>toklist</tokenlist>\n"
+                        "<pattern>.+</pattern>\n"
+                        "<message>\n"
+                        "<severity>error</severity>\n"
+                        "<id>ruleId</id>\n"
+                        "<summary>ruleSummary</summary>\n"
+                        "</message>\n"
+                        "</rule>\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", "<?xml version=\"1.0\"?>");
         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",
+                        "<rule>"
+                        "<tokenlist/>"
+                        "<pattern/>"
+                        "<message/>"
+                        "</rule>"
+                        );
+        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",
+                        "<rule>"
+                        "<message>"
+                        "<severity/>"
+                        "<id/>"
+                        "<summary/>"
+                        "</message>"
+                        "</rule>"
+                        );
+        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",
+                        "<rule>"
+                        "<messages/>"
+                        "</rule>"
+                        );
+        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",
+                        "<rule>"
+                        "<message>"
+                        "<pattern/>"
+                        "</message>"
+                        "</rule>"
+                        );
+        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() {