improved handling/testing of XMLs without a a root node / added some …

…TODOs (danmar#6086) This is based on a Coverity finding in `SuppressionList::parseXmlFile()`. `--suppress-xml` was actually crashing when a XML without a root node was provided.
chrchr-github · Mar 6, 2024 · 260ae3c · 260ae3c
1 parent ca47f54
commit 260ae3c
Show file tree

Hide file tree

Showing 7 changed files with 167 additions and 7 deletions.
diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp
@@ -1040,11 +1040,13 @@ 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.
                 const std::string ruleFile = argv[i] + 12;
                 tinyxml2::XMLDocument doc;
                 const tinyxml2::XMLError err = doc.LoadFile(ruleFile.c_str());
                 if (err == tinyxml2::XML_SUCCESS) {
-                    tinyxml2::XMLElement *node = doc.FirstChildElement();
+                    const tinyxml2::XMLElement *node = doc.FirstChildElement();
+                    // TODO: this looks like legacy handling - deprecate it
                     if (node && strcmp(node->Value(), "rules") == 0)
                         node = node->FirstChildElement("rule");
                     for (; node && strcmp(node->Value(), "rule") == 0; node = node->NextSiblingElement()) {
@@ -1059,7 +1061,7 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
                             rule.pattern = pattern->GetText();
                         }
 
-                        tinyxml2::XMLElement *message = node->FirstChildElement("message");
+                        const tinyxml2::XMLElement *message = node->FirstChildElement("message");
                         if (message) {
                             const tinyxml2::XMLElement *severity = message->FirstChildElement("severity");
                             if (severity)

diff --git a/lib/suppressions.cpp b/lib/suppressions.cpp
@@ -103,15 +103,16 @@ std::string SuppressionList::parseXmlFile(const char *filename)
 {
     tinyxml2::XMLDocument doc;
     const tinyxml2::XMLError error = doc.LoadFile(filename);
-    if (error == tinyxml2::XML_ERROR_FILE_NOT_FOUND)
-        return "File not found";
     if (error != tinyxml2::XML_SUCCESS)
-        return "Failed to parse XML file";
+        return std::string("failed to load suppressions XML '") + filename + "' (" + tinyxml2::XMLDocument::ErrorIDToName(error) + ").";
 
     const tinyxml2::XMLElement * const rootnode = doc.FirstChildElement();
+    if (!rootnode)
+        return std::string("failed to load suppressions XML '") + filename + "' (no root node found).";
+    // TODO: check for proper root node 'suppressions'
     for (const tinyxml2::XMLElement * e = rootnode->FirstChildElement(); e; e = e->NextSiblingElement()) {
         if (std::strcmp(e->Name(), "suppress") != 0)
-            return "Invalid suppression xml file format, expected <suppress> element but got a \"" + std::string(e->Name()) + '\"';
+            return std::string("invalid suppression xml file '") + filename + "', expected 'suppress' element but got a '" + e->Name() + "'.";
 
         Suppression s;
         for (const tinyxml2::XMLElement * e2 = e->FirstChildElement(); e2; e2 = e2->NextSiblingElement()) {
@@ -127,7 +128,7 @@ std::string SuppressionList::parseXmlFile(const char *filename)
             else if (*text && std::strcmp(e2->Name(), "hash") == 0)
                 s.hash = strToInt<std::size_t>(text);
             else
-                return "Unknown suppression element \"" + std::string(e2->Name()) + "\", expected id/fileName/lineNumber/symbolName/hash";
+                return std::string("unknown element '") + e2->Name() + "' in suppressions XML '" + filename + "', expected id/fileName/lineNumber/symbolName/hash.";
         }
 
         const std::string err = addSuppression(std::move(s));

diff --git a/test/testcmdlineparser.cpp b/test/testcmdlineparser.cpp
@@ -338,6 +338,7 @@ class TestCmdlineParser : public TestFixture {
         TEST_CASE(ruleFileEmpty);
         TEST_CASE(ruleFileMissing);
         TEST_CASE(ruleFileInvalid);
+        TEST_CASE(ruleFileNoRoot);
 #else
         TEST_CASE(ruleFileNotSupported);
 #endif
@@ -348,6 +349,11 @@ class TestCmdlineParser : public TestFixture {
         TEST_CASE(signedCharUnsignedChar);
         TEST_CASE(library);
         TEST_CASE(libraryMissing);
+        TEST_CASE(suppressXml);
+        TEST_CASE(suppressXmlEmpty);
+        TEST_CASE(suppressXmlMissing);
+        TEST_CASE(suppressXmlInvalid);
+        TEST_CASE(suppressXmlNoRoot);
 
         TEST_CASE(ignorepaths1);
         TEST_CASE(ignorepaths2);
@@ -2162,6 +2168,13 @@ class TestCmdlineParser : public TestFixture {
         ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
         ASSERT_EQUALS("cppcheck: error: unable to load rule-file 'rule.xml' (XML_ERROR_EMPTY_DOCUMENT).\n", logger->str());
     }
+
+    void ruleFileNoRoot() {
+        REDIRECT;
+        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));
+    }
 #else
     void ruleFileNotSupported() {
         REDIRECT;
@@ -2188,6 +2201,52 @@ class TestCmdlineParser : public TestFixture {
         ASSERT_EQUALS("cppcheck: Failed to load library configuration file 'posix2'. File not found\n", logger->str());
     }
 
+    void suppressXml() {
+        REDIRECT;
+        ScopedFile file("suppress.xml",
+                        "<suppressions>\n"
+                        "<suppress>\n"
+                        "<id>uninitvar</id>\n"
+                        "</suppress>\n"
+                        "</suppressions>");
+        const char * const argv[] = {"cppcheck", "--suppress-xml=suppress.xml", "file.cpp"};
+        ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv));
+        const auto& supprs = settings->supprs.nomsg.getSuppressions();
+        ASSERT_EQUALS(1, supprs.size());
+        const auto it = supprs.cbegin();
+        ASSERT_EQUALS("uninitvar", it->errorId);
+    }
+
+    void suppressXmlEmpty() {
+        REDIRECT;
+        const char * const argv[] = {"cppcheck", "--suppress-xml=", "file.cpp"};
+        ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
+        ASSERT_EQUALS("cppcheck: error: failed to load suppressions XML '' (XML_ERROR_FILE_NOT_FOUND).\n", logger->str());
+    }
+
+    void suppressXmlMissing() {
+        REDIRECT;
+        const char * const argv[] = {"cppcheck", "--suppress-xml=suppress.xml", "file.cpp"};
+        ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
+        ASSERT_EQUALS("cppcheck: error: failed to load suppressions XML 'suppress.xml' (XML_ERROR_FILE_NOT_FOUND).\n", logger->str());
+    }
+
+    void suppressXmlInvalid() {
+        REDIRECT;
+        ScopedFile file("suppress.xml", "");
+        const char * const argv[] = {"cppcheck", "--suppress-xml=suppress.xml", "file.cpp"};
+        ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
+        ASSERT_EQUALS("cppcheck: error: failed to load suppressions XML 'suppress.xml' (XML_ERROR_EMPTY_DOCUMENT).\n", logger->str());
+    }
+
+    void suppressXmlNoRoot() {
+        REDIRECT;
+        ScopedFile file("suppress.xml", "<?xml version=\"1.0\"?>");
+        const char * const argv[] = {"cppcheck", "--suppress-xml=suppress.xml", "file.cpp"};
+        ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv));
+        ASSERT_EQUALS("cppcheck: error: failed to load suppressions XML 'suppress.xml' (no root node found).\n", logger->str());
+    }
+
     void ignorepaths1() {
         REDIRECT;
         const char * const argv[] = {"cppcheck", "-isrc", "file.cpp"};

diff --git a/test/testerrorlogger.cpp b/test/testerrorlogger.cpp
@@ -279,6 +279,8 @@ class TestErrorLogger : public TestFixture {
                                "</error>";
         tinyxml2::XMLDocument doc;
         ASSERT(doc.Parse(xmldata, sizeof(xmldata)) == tinyxml2::XML_SUCCESS);
+        const auto * const rootnode = doc.FirstChildElement();
+        ASSERT(rootnode);
         ErrorMessage msg(doc.FirstChildElement());
         ASSERT_EQUALS("errorId", msg.id);
         ASSERT_EQUALS_ENUM(Severity::error, msg.severity);

diff --git a/test/testlibrary.cpp b/test/testlibrary.cpp
@@ -984,6 +984,9 @@ class TestLibrary : public TestFixture {
 
     void loadLibErrors() const {
 
+        LOADLIBERROR("<?xml version=\"1.0\"?>",
+                     Library::ErrorCode::BAD_XML);
+
         LOADLIBERROR("<?xml version=\"1.0\"?>\n"
                      "<def>\n"
                      "   <X name=\"uint8_t,std::uint8_t\" size=\"1\"/>\n"

diff --git a/test/testplatform.cpp b/test/testplatform.cpp
@@ -46,6 +46,8 @@ class TestPlatform : public TestFixture {
         TEST_CASE(default_platform);
         TEST_CASE(limitsDefines);
         TEST_CASE(charMinMax);
+        TEST_CASE(no_root_node);
+        TEST_CASE(wrong_root_node);
     }
 
     static bool readPlatform(Platform& platform, const char* xmldata) {
@@ -417,6 +419,19 @@ class TestPlatform : public TestFixture {
         ASSERT_EQUALS(127, platform.signedCharMax());
         ASSERT_EQUALS(-128, platform.signedCharMin());
     }
+
+    void no_root_node() const {
+        constexpr char xmldata[] = "<?xml version=\"1.0\"?>";
+        Platform platform;
+        ASSERT(!readPlatform(platform, xmldata));
+    }
+
+    void wrong_root_node() const {
+        constexpr char xmldata[] = "<?xml version=\"1.0\"?>\n"
+                                   "<platforms/>";
+        Platform platform;
+        ASSERT(!readPlatform(platform, xmldata));
+    }
 };
 
 REGISTER_TEST(TestPlatform)
diff --git a/test/testsuppressions.cpp b/test/testsuppressions.cpp
@@ -97,6 +97,8 @@ class TestSuppressions : public TestFixture {
         TEST_CASE(suppressLocal);
 
         TEST_CASE(suppressUnmatchedSuppressions);
+
+        TEST_CASE(suppressionsParseXmlFile);
     }
 
     void suppressionsBadId1() const {
@@ -1462,6 +1464,82 @@ class TestSuppressions : public TestFixture {
         ASSERT_EQUALS(true, SuppressionList::reportUnmatchedSuppressions(suppressions, *this));
         ASSERT_EQUALS("[a.c:10]: (information) Unmatched suppression: abc\n", errout.str());
     }
+
+    void suppressionsParseXmlFile() const {
+        {
+            ScopedFile file("suppressparsexml.xml",
+                            "<suppressions>\n"
+                            "<suppress>\n"
+                            "<id>uninitvar</id>\n"
+                            "<fileName>file.c</fileName>\n"
+                            "<lineNumber>10</lineNumber>\n"
+                            "<symbolName>sym</symbolName>\n"
+                            "</suppress>\n"
+                            "</suppressions>");
+
+            SuppressionList supprList;
+            ASSERT_EQUALS("", supprList.parseXmlFile(file.path().c_str()));
+            const auto& supprs = supprList.getSuppressions();
+            ASSERT_EQUALS(1, supprs.size());
+            const auto& suppr = *supprs.cbegin();
+            ASSERT_EQUALS("uninitvar", suppr.errorId);
+            ASSERT_EQUALS("file.c", suppr.fileName);
+            ASSERT_EQUALS(10, suppr.lineNumber);
+            ASSERT_EQUALS("sym", suppr.symbolName);
+        }
+
+        // no file specified
+        {
+            SuppressionList supprList;
+            ASSERT_EQUALS("failed to load suppressions XML '' (XML_ERROR_FILE_NOT_FOUND).", supprList.parseXmlFile(""));
+        }
+
+        // missing file
+        {
+            SuppressionList supprList;
+            ASSERT_EQUALS("failed to load suppressions XML 'suppressparsexml.xml' (XML_ERROR_FILE_NOT_FOUND).", supprList.parseXmlFile("suppressparsexml.xml"));
+        }
+
+        // empty file
+        {
+            ScopedFile file("suppressparsexml.xml",
+                            "");
+
+            SuppressionList supprList;
+            ASSERT_EQUALS("failed to load suppressions XML 'suppressparsexml.xml' (XML_ERROR_EMPTY_DOCUMENT).", supprList.parseXmlFile(file.path().c_str()));
+        }
+
+        // wrong root node
+        {
+            ScopedFile file("suppressparsexml.xml",
+                            "<suppress/>\n");
+
+            SuppressionList supprList;
+            ASSERT_EQUALS("", supprList.parseXmlFile(file.path().c_str()));
+        }
+
+        // no root node
+        {
+            ScopedFile file("suppressparsexml.xml",
+                            "<?xml version=\"1.0\"?>\n");
+
+            SuppressionList supprList;
+            ASSERT_EQUALS("failed to load suppressions XML 'suppressparsexml.xml' (no root node found).", supprList.parseXmlFile(file.path().c_str()));
+        }
+
+        // unknown element
+        {
+            ScopedFile file("suppressparsexml.xml",
+                            "<suppressions>\n"
+                            "<suppress>\n"
+                            "<eid>uninitvar</eid>\n"
+                            "</suppress>\n"
+                            "</suppressions>");
+
+            SuppressionList supprList;
+            ASSERT_EQUALS("unknown element 'eid' in suppressions XML 'suppressparsexml.xml', expected id/fileName/lineNumber/symbolName/hash.", supprList.parseXmlFile(file.path().c_str()));
+        }
+    }
 };
 
 REGISTER_TEST(TestSuppressions)