fixed #14638 - report errors for invalid suppression lines [skip ci]

Author: firewave

lib/suppressions.cpp

@@ -207,9 +207,8 @@ std::vector<SuppressionList::Suppression> SuppressionList::parseMultiSuppressCom
     return suppressions;
 }
 
-SuppressionList::Suppression SuppressionList::parseLine(const std::string &line)
+SuppressionList::Suppression SuppressionList::parseLine(std::string line)
 {
-    std::istringstream lineStream;
     SuppressionList::Suppression suppression;
 
     // Strip any end of line comments
@@ -218,13 +217,18 @@ SuppressionList::Suppression SuppressionList::parseLine(const std::string &line)
         while (endpos > 0 && std::isspace(line[endpos-1])) {
             endpos--;
         }
-        lineStream.str(line.substr(0, endpos));
-    } else {
-        lineStream.str(line);
+        line = line.substr(0, endpos);
     }
 
-    if (std::getline(lineStream, suppression.errorId, ':')) {
-        if (std::getline(lineStream, suppression.fileName)) {
+    const auto parts = splitString(line, '\n');
+    const std::string& suppr_l = parts[0];
+
+    const std::string::size_type first_sep = suppr_l.find(':');
+    suppression.errorId = suppr_l.substr(0, first_sep);
+    if (first_sep != std::string::npos) {
+        suppression.fileName = suppr_l.substr(first_sep+1);
+        if (!suppression.fileName.empty()) {
+            // TODO: this only works with files which have an extension
             // If there is not a dot after the last colon in "file" then
             // the colon is a separator and the contents after the colon
             // is a line number..
@@ -235,38 +239,38 @@ SuppressionList::Suppression SuppressionList::parseLine(const std::string &line)
             // if a colon is found and there is no dot after it..
             if (pos != std::string::npos &&
                 suppression.fileName.find('.', pos) == std::string::npos) {
-                // Try to parse out the line number
-                try {
-                    std::istringstream istr1(suppression.fileName.substr(pos+1));
-                    istr1 >> suppression.lineNumber;
-                } catch (...) {
-                    suppression.lineNumber = SuppressionList::Suppression::NO_LINE;
-                }
-
-                if (suppression.lineNumber != SuppressionList::Suppression::NO_LINE) {
-                    suppression.fileName.erase(pos);
-                }
-            }
-
-            // when parsing string generated internally by toString() there can be newline
-            std::string extra;
-            while (std::getline(lineStream, extra)) {
-                if (startsWith(extra, "symbol="))
-                    suppression.symbolName = extra.substr(7);
-                else if (extra == "polyspace=1")
-                    suppression.isPolyspace = true;
+                // parse out the line number
+                const std::string line_s = suppression.fileName.substr(pos+1);
+                suppression.lineNumber = strToInt<int>(line_s);
+                suppression.fileName.erase(pos);
             }
+            suppression.fileName = Path::simplifyPath(suppression.fileName);
+        } else {
+            throw std::runtime_error("filename is missing");
         }
     }
 
-    suppression.fileName = Path::simplifyPath(suppression.fileName);
+    // TODO: make this optional - do we even encounter this in production code?
+    // when parsing string generated internally by toString() there can be newline
+    for (std::size_t i = 1; i < parts.size(); ++i) {
+        if (startsWith(parts[i], "symbol="))
+            suppression.symbolName = parts[i].substr(7);
+        else if (parts[i] == "polyspace=1")
+            suppression.isPolyspace = true;
+        else
+            throw std::runtime_error("unexpected extra '" + parts[i] + "'");
+    }
 
     return suppression;
 }
 
 std::string SuppressionList::addSuppressionLine(const std::string &line)
 {
-    return addSuppression(parseLine(line));
+    try {
+        return addSuppression(parseLine(line));
+    } catch (const std::exception& e) {
+        return e.what();
+    }
 }
 
 std::string SuppressionList::addSuppression(SuppressionList::Suppression suppression)

lib/suppressions.h

@@ -197,7 +197,7 @@ class CPPCHECKLIB SuppressionList {
      * @param line The line to parse.
      * @return a suppression object
      */
-    static Suppression parseLine(const std::string &line);
+    static Suppression parseLine(std::string line);
 
     /**
      * @brief Don't show the given error.

test/testsuppressions.cpp

@@ -58,6 +58,7 @@ class TestSuppressions : public TestFixture {
     void run() override {
         mNewTemplate = true;
         TEST_CASE(parseLine);
+        TEST_CASE(parseLineInvalid);
         TEST_CASE(suppressionsBadId1);
         TEST_CASE(suppressionsDosFormat);     // Ticket #1836
         TEST_CASE(suppressionsFileNameWithColon);    // Ticket #1919 - filename includes colon
@@ -171,11 +172,38 @@ class TestSuppressions : public TestFixture {
         ASSERT_EQUALS("bad:test.c:1\nsymbol=x\npolyspace=1", SuppressionList::parseLine("bad:test.c:1\nsymbol=x\npolyspace=1").toString());
 
         // polyspace + symbol
-        ASSERT_EQUALS("bad\npolyspace=1\nsymbol=x", SuppressionList::parseLine("bad\npolyspace=1\nsymbol=x").toString());
+        ASSERT_EQUALS("bad\nsymbol=x\npolyspace=1", SuppressionList::parseLine("bad\npolyspace=1\nsymbol=x").toString());
         ASSERT_EQUALS("bad:test.c\nsymbol=x\npolyspace=1", SuppressionList::parseLine("bad:test.c\npolyspace=1\nsymbol=x").toString());
         ASSERT_EQUALS("bad:test.c:1\nsymbol=x\npolyspace=1", SuppressionList::parseLine("bad:test.c:1\npolyspace=1\nsymbol=x").toString());
     }
 
+    void parseLineInvalid() const {
+        // missing filename
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:"), std::runtime_error, "filename is missing");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:\n"), std::runtime_error, "filename is missing");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:\n1.c"), std::runtime_error, "filename is missing");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:#1.c"), std::runtime_error, "filename is missing"); // TODO: looks like a valid filename
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id://1.c"), std::runtime_error, "filename is missing");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id::"), std::runtime_error, "converting '' to integer failed - not an integer");
+
+        // missing/invalid line
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c:"), std::runtime_error, "converting '' to integer failed - not an integer");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c:\n"), std::runtime_error, "converting '' to integer failed - not an integer");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c:\n1"), std::runtime_error, "converting '' to integer failed - not an integer");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c:#1"), std::runtime_error, "converting '' to integer failed - not an integer"); // TODO: looks like a valid filename
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c://1"), std::runtime_error, "converting '' to integer failed - not an integer");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c:zero"), std::runtime_error, "converting 'zero' to integer failed - not an integer");
+
+        // invalid extras
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c:1\n"), std::runtime_error, "unexpected extra ''");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c:1\nsym=x"), std::runtime_error, "unexpected extra 'sym=x'");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c:1\nsymbol:x"), std::runtime_error, "unexpected extra 'symbol:x'");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c:1\npolyspace=0"), std::runtime_error, "unexpected extra 'polyspace=0'");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c:1\npolyspace:1"), std::runtime_error, "unexpected extra 'polyspace:1'");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id\n:"), std::runtime_error, "unexpected extra ':'");
+        ASSERT_THROW_EQUALS(SuppressionList::parseLine("id:1.c\n:"), std::runtime_error, "unexpected extra ':'");
+    }
+
     void suppressionsBadId1() const {
         SuppressionList suppressions;
         std::istringstream s1("123");