From 260ae3c38bcba571520e8ae4518a2e80d2eed8c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20St=C3=B6neberg?= Date: Wed, 6 Mar 2024 11:14:37 +0100 Subject: [PATCH] improved handling/testing of XMLs without a a root node / added some TODOs (#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. --- cli/cmdlineparser.cpp | 6 ++- lib/suppressions.cpp | 11 +++--- test/testcmdlineparser.cpp | 59 ++++++++++++++++++++++++++++ test/testerrorlogger.cpp | 2 + test/testlibrary.cpp | 3 ++ test/testplatform.cpp | 15 ++++++++ test/testsuppressions.cpp | 78 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 167 insertions(+), 7 deletions(-) diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp index e0efda90dbe..20163c14fcd 100644 --- 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 index 80e922490ea..46f26550386 100644 --- 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 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(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 index a552b652556..52e293fe7e9 100644 --- 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", ""); + 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", + "\n" + "\n" + "uninitvar\n" + "\n" + ""); + 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", ""); + 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 index 49fc814ffa0..8bba7dc7973 100644 --- a/test/testerrorlogger.cpp +++ b/test/testerrorlogger.cpp @@ -279,6 +279,8 @@ class TestErrorLogger : public TestFixture { ""; 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 index d7fc3ca68d5..b15b73a3b46 100644 --- a/test/testlibrary.cpp +++ b/test/testlibrary.cpp @@ -984,6 +984,9 @@ class TestLibrary : public TestFixture { void loadLibErrors() const { + LOADLIBERROR("", + Library::ErrorCode::BAD_XML); + LOADLIBERROR("\n" "\n" " \n" diff --git a/test/testplatform.cpp b/test/testplatform.cpp index 177ce1ad61a..8e16575e550 100644 --- 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[] = ""; + Platform platform; + ASSERT(!readPlatform(platform, xmldata)); + } + + void wrong_root_node() const { + constexpr char xmldata[] = "\n" + ""; + Platform platform; + ASSERT(!readPlatform(platform, xmldata)); + } }; REGISTER_TEST(TestPlatform) diff --git a/test/testsuppressions.cpp b/test/testsuppressions.cpp index 927d6938d69..c2e1558badc 100644 --- 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", + "\n" + "\n" + "uninitvar\n" + "file.c\n" + "10\n" + "sym\n" + "\n" + ""); + + 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", + "\n"); + + SuppressionList supprList; + ASSERT_EQUALS("", supprList.parseXmlFile(file.path().c_str())); + } + + // no root node + { + ScopedFile file("suppressparsexml.xml", + "\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", + "\n" + "\n" + "uninitvar\n" + "\n" + ""); + + 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)