Fixed #12071 (suppressing critical error, no indication to user that analysis of file fails) (cppcheck-opensource#5771)

danmar · web-flow · commit 7c316fb76ddb · 2023-12-17T15:42:17.000+01:00
diff --git a/.github/workflows/CI-unixish.yml b/.github/workflows/CI-unixish.yml
@@ -423,7 +423,8 @@ jobs:
         run: |
           ./cppcheck --error-exitcode=1 --inline-suppr --addon=threadsafety addons/test/threadsafety
           ./cppcheck --error-exitcode=1 --inline-suppr --addon=threadsafety --std=c++03 addons/test/threadsafety
-          ./cppcheck --error-exitcode=1 --inline-suppr --addon=misra addons/test/misra/crash*.c
+          ./cppcheck --error-exitcode=1 --dump addons/test/misra/crash*.c
+          python3 addons/misra.py --cli addons/test/misra/crash*.c.dump > /dev/null
           ./cppcheck --error-exitcode=1 --inline-suppr --addon=misra --enable=information addons/test/misra/config*.c
 
           ./cppcheck --addon=misra --enable=style --inline-suppr --enable=information --error-exitcode=1 addons/test/misra/misra-ctu-*-test.c
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -47,7 +47,7 @@ if(LIBXML2_XMLLINT_EXECUTABLE)
     add_custom_target(errorlist-xml $<TARGET_FILE:cppcheck> --errorlist > ${CMAKE_BINARY_DIR}/errorlist.xml
             DEPENDS cppcheck)
 
-    add_custom_target(example-xml $<TARGET_FILE:cppcheck> --xml --enable=all --inconclusive --max-configs=1 ${CMAKE_SOURCE_DIR}/samples 2> ${CMAKE_BINARY_DIR}/example.xml
+    add_custom_target(example-xml $<TARGET_FILE:cppcheck> --xml --enable=all --inconclusive --max-configs=1 ${CMAKE_SOURCE_DIR}/samples -i${CMAKE_SOURCE_DIR}/samples/syntaxError/bad.c 2> ${CMAKE_BINARY_DIR}/example.xml
             DEPENDS cppcheck)
 
     add_custom_target(createXMLExamples DEPENDS errorlist-xml example-xml)
diff --git a/Makefile b/Makefile
@@ -439,7 +439,7 @@ validatePlatforms: ${PlatformFilesCHECKED}
 /tmp/errorlist.xml: cppcheck
 	./cppcheck --errorlist >$@
 /tmp/example.xml: cppcheck
-	./cppcheck --xml --enable=all --inconclusive --max-configs=1 samples 2>/tmp/example.xml
+	./cppcheck --xml --enable=all --inconclusive --max-configs=1 samples -isamples/syntaxError/bad.c 2>/tmp/example.xml
 createXMLExamples:/tmp/errorlist.xml /tmp/example.xml
 .PHONY: validateXML
 validateXML: createXMLExamples
diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp
@@ -587,6 +587,10 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
                 if (!parseNumberArg(argv[i], 17, mSettings.exitCode))
                     return Result::Fail;
             }
+            // --unsafe-exitcode => for tests, critical errors will not force non-zero exitcode
+            else if (std::strcmp(argv[i], "--unsafe-exitcode") == 0) {
+                mSettings.unsafeExitCode = true;
+            }
 
             // Exception handling inside cppcheck client
             else if (std::strcmp(argv[i], "--exception-handling") == 0) {
diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp
@@ -118,6 +118,10 @@ class CppCheckExecutor::StdLogger : public ErrorLogger
      */
     void writeCheckersReport() const;
 
+    bool hasCriticalErrors() const {
+        return !mCriticalErrors.empty();
+    }
+
 private:
     /**
      * Information about progress is directed here. This should be
@@ -287,9 +291,12 @@ int CppCheckExecutor::check_internal(CppCheck& cppcheck) const
 
     mStdLogger->writeCheckersReport();
 
+    if (!settings.unsafeExitCode && mStdLogger->hasCriticalErrors())
+        return settings.exitCode > 0 ? settings.exitCode : EXIT_FAILURE;
+
     if (returnValue)
         return settings.exitCode;
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 void CppCheckExecutor::StdLogger::writeCheckersReport() const
@@ -398,6 +405,10 @@ void CppCheckExecutor::StdLogger::reportErr(const ErrorMessage &msg)
         if (!mCriticalErrors.empty())
             mCriticalErrors += ", ";
         mCriticalErrors += msg.id;
+        if (msg.severity == Severity::none) {
+            mCriticalErrors += " (suppressed)";
+            return;
+        }
     }
 
     if (mSettings.xml)
diff --git a/gui/resultstree.cpp b/gui/resultstree.cpp
@@ -666,6 +666,11 @@ void ResultsTree::contextMenuEvent(QContextMenuEvent * e)
             menu.addAction(hideallid);
 
             QAction *suppress = new QAction(tr("Suppress selected id(s)"), &menu);
+            {
+                QVariantMap data = mContextItem->data().toMap();
+                const QString messageId = data[ERRORID].toString();
+                suppress->setEnabled(!ErrorLogger::isCriticalErrorId(messageId.toStdString()));
+            }
             menu.addAction(suppress);
             connect(suppress, &QAction::triggered, this, &ResultsTree::suppressSelectedIds);
 
diff --git a/gui/resultsview.cpp b/gui/resultsview.cpp
@@ -168,6 +168,9 @@ void ResultsView::error(const ErrorItem &item)
 
     handleCriticalError(item);
 
+    if (item.severity == Severity::none)
+        return;
+
     if (mUI->mTree->addErrorItem(item)) {
         emit gotResults();
         mStatistics->addItem(item.tool(), ShowTypes::SeverityToShowType(item.severity));
@@ -562,10 +565,14 @@ void ResultsView::handleCriticalError(const ErrorItem &item)
             if (!mCriticalErrors.isEmpty())
                 mCriticalErrors += ",";
             mCriticalErrors += item.errorId;
+            if (item.severity == Severity::none)
+                mCriticalErrors += " (suppressed)";
         }
         QString msg = tr("There was a critical error with id '%1'").arg(item.errorId);
         if (!item.file0.isEmpty())
             msg += ", " + tr("when checking %1").arg(item.file0);
+        else
+            msg += ", " + tr("when checking a file");
         msg += ". " + tr("Analysis was aborted.");
         mUI->mLabelCriticalErrors->setText(msg);
         mUI->mLabelCriticalErrors->setVisible(true);
diff --git a/htmlreport/check.sh b/htmlreport/check.sh
@@ -40,7 +40,7 @@ validate_html "$INDEX_HTML"
 validate_html "$STATS_HTML"
 
 
-../cppcheck ../samples --enable=all --inconclusive --xml-version=2 2> "$GUI_TEST_XML"
+../cppcheck ../samples -i../samples/syntaxError/bad.c --enable=all --inconclusive --xml-version=2 2> "$GUI_TEST_XML"
 xmllint --noout "$GUI_TEST_XML"
 $PYTHON cppcheck-htmlreport --file "$GUI_TEST_XML" --title "xml2 + inconclusive test" --report-dir "$REPORT_DIR"
 echo ""
@@ -49,7 +49,7 @@ validate_html "$INDEX_HTML"
 validate_html "$STATS_HTML"
 
 
-../cppcheck ../samples --enable=all --inconclusive --verbose --xml-version=2 2> "$GUI_TEST_XML"
+../cppcheck ../samples -i../samples/syntaxError/bad.c --enable=all --inconclusive --verbose --xml-version=2 2> "$GUI_TEST_XML"
 xmllint --noout "$GUI_TEST_XML"
 $PYTHON cppcheck-htmlreport --file "$GUI_TEST_XML" --title "xml2 + inconclusive + verbose test" --report-dir "$REPORT_DIR"
 echo -e "\n"
diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp
@@ -1599,6 +1599,19 @@ void CppCheck::reportErr(const ErrorMessage &msg)
     const auto errorMessage = Suppressions::ErrorMessage::fromErrorMessage(msg, macroNames);
 
     if (mSettings.nomsg.isSuppressed(errorMessage, mUseGlobalSuppressions)) {
+        if (ErrorLogger::isCriticalErrorId(msg.id)) {
+            mExitCode = 1;
+
+            if (mSettings.nomsg.isSuppressedExplicitly(errorMessage, mUseGlobalSuppressions)) {
+                ErrorMessage temp;
+                temp.severity = Severity::none;
+                temp.id = msg.id;
+                mErrorLogger.reportErr(temp);
+            } else {
+                // Report critical error that is not explicitly suppressed
+                mErrorLogger.reportErr(msg);
+            }
+        }
         return;
     }
 
diff --git a/lib/settings.h b/lib/settings.h
@@ -358,6 +358,9 @@ class CPPCHECKLIB WARN_UNUSED Settings {
     /** @brief The maximum time in seconds for the typedef simplification */
     std::size_t typedefMaxTime{};
 
+    /** @brief Unsafe exitcode => do not force non-zero exitcode when there are critical errors */
+    bool unsafeExitCode = false;
+
     /** @brief defines given by the user */
     std::string userDefines;
 
diff --git a/lib/suppressions.cpp b/lib/suppressions.cpp
@@ -418,6 +418,19 @@ bool Suppressions::isSuppressed(const Suppressions::ErrorMessage &errmsg, bool g
     return returnValue;
 }
 
+bool Suppressions::isSuppressedExplicitly(const Suppressions::ErrorMessage &errmsg, bool global)
+{
+    for (Suppression &s : mSuppressions) {
+        if (!global && !s.isLocal())
+            continue;
+        if (s.errorId != errmsg.errorId) // Error id must match exactly
+            continue;
+        if (s.isMatch(errmsg))
+            return true;
+    }
+    return false;
+}
+
 bool Suppressions::isSuppressed(const ::ErrorMessage &errmsg, const std::set<std::string>& macroNames)
 {
     if (mSuppressions.empty())
diff --git a/lib/suppressions.h b/lib/suppressions.h
@@ -202,6 +202,14 @@ class CPPCHECKLIB Suppressions {
      */
     bool isSuppressed(const ErrorMessage &errmsg, bool global = true);
 
+    /**
+     * @brief Returns true if this message is "explicitly" suppressed. The suppression "id" must match textually exactly.
+     * @param errmsg error message
+     * @param global use global suppressions
+     * @return true if this error is explicitly suppressed.
+     */
+    bool isSuppressedExplicitly(const ErrorMessage &errmsg, bool global = true);
+
     /**
      * @brief Returns true if this message should not be shown to the user.
      * @param errmsg error message
diff --git a/releasenotes.txt b/releasenotes.txt
@@ -44,3 +44,6 @@ Other:
 - Markup files will now be processed after the regular source files when using multiple threads/processes (some issues remain - see Trac #12167 for details).
 - Added file name to ValueFlow "--debug" output.
 - Fixed build when using "clang-cl" in CMake.
+
+Safety critical fixes:
+- Changed handling when critical errors are suppressed. They can only be suppressed explicitly and the exit code will never be successful after a critical error.
diff --git a/test/cli/test-other.py b/test/cli/test-other.py
@@ -411,7 +411,7 @@ def test_invalid_addon_py(tmpdir):
     args = ['--addon={}'.format(addon_file), '--enable=all', test_file]
 
     exitcode, stdout, stderr = cppcheck(args)
-    assert exitcode == 0  # TODO: needs to be 1
+    assert exitcode == 1
     lines = stdout.splitlines()
     assert lines == [
         'Checking {} ...'.format(test_file)
@@ -435,7 +435,7 @@ def test_invalid_addon_py_verbose(tmpdir):
     args = ['--addon={}'.format(addon_file), '--enable=all', '--verbose', test_file]
 
     exitcode, stdout, stderr = cppcheck(args)
-    assert exitcode == 0  # TODO: needs to be 1
+    assert exitcode == 1
     lines = stdout.splitlines()
     assert lines == [
         'Checking {} ...'.format(test_file),
diff --git a/test/cli/test-suppress-syntaxError.py b/test/cli/test-suppress-syntaxError.py
@@ -13,3 +13,15 @@ def test_j2_suppress():
     assert ret == 0
     assert len(stderr) == 0
 
+def test_suppress_syntax_error_implicitly():
+    ret, stdout, stderr = cppcheck(['--suppress=*', 'proj-suppress-syntaxError'], remove_active_checkers=False)
+    assert ret == 1
+    assert '[syntaxError]' in stderr
+    assert 'Active checkers: There was critical errors' in stdout
+
+def test_suppress_syntax_error_explicitly():
+    ret, stdout, stderr = cppcheck(['--suppress=syntaxError', 'proj-suppress-syntaxError'], remove_active_checkers=False)
+    assert ret == 1
+    assert '[syntaxError]' not in stderr
+    assert 'Active checkers: There was critical errors' in stdout
+
diff --git a/test/cli/testutils.py b/test/cli/testutils.py
@@ -71,7 +71,7 @@ def __lookup_cppcheck_exe():
 
 
 # Run Cppcheck with args
-def cppcheck(args, env=None):
+def cppcheck(args, env=None, remove_active_checkers=True):
     exe = __lookup_cppcheck_exe()
     assert exe is not None, 'no cppcheck binary found'
 
@@ -80,7 +80,7 @@ def cppcheck(args, env=None):
     comm = p.communicate()
     stdout = comm[0].decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
     stderr = comm[1].decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
-    if stdout.find('\nActive checkers:') > 0:
+    if remove_active_checkers and stdout.find('\nActive checkers:') > 0:
         stdout = stdout[:1 + stdout.find('\nActive checkers:')]
     return p.returncode, stdout, stderr
 
diff --git a/test/testsuppressions.cpp b/test/testsuppressions.cpp
@@ -1144,7 +1144,7 @@ class TestSuppressions : public TestFixture {
     void suppressingSyntaxErrors() { // syntaxErrors should be suppressible (#7076)
         const char code[] = "if if\n";
 
-        ASSERT_EQUALS(0, checkSuppression(code, "syntaxError:test.cpp:1"));
+        ASSERT_EQUALS(1, checkSuppression(code, "syntaxError:test.cpp:1"));
         ASSERT_EQUALS("", errout.str());
     }
 
@@ -1159,7 +1159,7 @@ class TestSuppressions : public TestFixture {
                             "   fstp  QWORD PTR result  ; store a double (8 bytes)\n"
                             "   pop   EAX               ; restore EAX\n"
                             "}";
-        ASSERT_EQUALS(0, checkSuppression(code, ""));
+        ASSERT_EQUALS(1, checkSuppression(code, ""));
         ASSERT_EQUALS("", errout.str());
     }
 
@@ -1177,7 +1177,7 @@ class TestSuppressions : public TestFixture {
                             "[!VAR \"BC\" = \"$BC + 1\"!][!//\n"
                             "[!ENDIF!][!//\n"
                             "};";
-        ASSERT_EQUALS(0, checkSuppression(code, "syntaxError:test.cpp:4"));
+        ASSERT_EQUALS(1, checkSuppression(code, "syntaxError:test.cpp:4"));
         ASSERT_EQUALS("", errout.str());
     }
 
@@ -1211,15 +1211,17 @@ class TestSuppressions : public TestFixture {
     void suppressingSyntaxErrorAndExitCode() {
         const char code[] = "fi if;";
 
-        ASSERT_EQUALS(0, checkSuppression(code, "*:test.cpp"));
-        ASSERT_EQUALS("", errout.str());
+        ASSERT_EQUALS(2, checkSuppression(code, "*:test.cpp"));
+        ASSERT_EQUALS("[test.cpp:1]: (error) syntax error\n", errout.str());  // <- critical errors can't be suppressed with globbed id
 
         // multi files, but only suppression one
         std::map<std::string, std::string> mfiles;
         mfiles["test.cpp"] = "fi if;";
         mfiles["test2.cpp"] = "fi if";
-        ASSERT_EQUALS(2, checkSuppression(mfiles, "*:test.cpp"));
-        ASSERT_EQUALS("[test2.cpp:1]: (error) syntax error\n", errout.str());
+        ASSERT_EQUALS(3, checkSuppression(mfiles, "*:test.cpp"));
+        ASSERT_EQUALS("[test.cpp:1]: (error) syntax error\n" // <- critical errors can't be suppressed with globbed id
+                      "[test2.cpp:1]: (error) syntax error\n",
+                      errout.str());
 
         // multi error in file, but only suppression one error
         const char code2[] = "fi fi\n"
diff --git a/tools/dmake.cpp b/tools/dmake.cpp
@@ -740,7 +740,7 @@ int main(int argc, char **argv)
     fout << "/tmp/errorlist.xml: cppcheck\n";
     fout << "\t./cppcheck --errorlist >$@\n";
     fout << "/tmp/example.xml: cppcheck\n";
-    fout << "\t./cppcheck --xml --enable=all --inconclusive --max-configs=1 samples 2>/tmp/example.xml\n";
+    fout << "\t./cppcheck --xml --enable=all --inconclusive --max-configs=1 samples -isamples/syntaxError/bad.c 2>/tmp/example.xml\n";
     fout << "createXMLExamples:/tmp/errorlist.xml /tmp/example.xml\n";
     fout << ".PHONY: validateXML\n";
     fout << "validateXML: createXMLExamples\n";
diff --git a/tools/extracttests.py b/tools/extracttests.py
@@ -94,8 +94,7 @@ def parseFile(self, filename):
         start_code = None
         disable = False
 
-        fin = open(filename, 'r')
-        for line in fin:
+        for line in open(filename, 'r'):
             # testclass starts
             res = re.match('class (' + name + ')', line)
             if res is not None:
@@ -137,6 +136,10 @@ def parseFile(self, filename):
             if code is not None:
                 res = re.match('\\s+' + string, line)
                 if res is not None:
+                    if line.find('",') > line.find('"'):
+                        code = None
+                        continue
+
                     code = code + res.group(1)
                     if res.group(1).find('"') > 0:
                         code = None
@@ -159,10 +162,8 @@ def parseFile(self, filename):
                         'expected': expected}
                 self.nodes.append(node)
                 code = None
-
-        # close test file
-        fin.close()
-
+            elif re.match('\\s+[TOD_]*ASSERT', line) is not None:
+                code = None
 
 def strtoxml(s):
     """Convert string to xml/html format"""
diff --git a/tools/run_more_tests.sh b/tools/run_more_tests.sh
@@ -18,6 +18,8 @@ python3 $DIR/extracttests.py --code=$(pwd)/test1 $1
 
 cd test1
 
+$CPPCHECK -q --template=cppcheck1 --suppress="*" .
+
 $CPPCHECK -q --template=cppcheck1 . 2> 1.txt
 
 echo "(!x) => (x==0)"
