Fix #12071 (Add safety mode that makes cppcheck more strict about critical errors) (#5777)

danmar · web-flow · commit 5aa1710dd03f · 2023-12-18T18:26:23.000+01:00
diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp
@@ -1066,6 +1066,10 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
 #endif
             }
 
+            // Safety certified behavior
+            else if (std::strcmp(argv[i], "--safety") == 0)
+                mSettings.safety = true;
+
             // show timing information..
             else if (std::strncmp(argv[i], "--showtime=", 11) == 0) {
                 const std::string showtimeMode = argv[i] + 11;
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
@@ -288,9 +292,12 @@ int CppCheckExecutor::check_internal(CppCheck& cppcheck) const
 
     mStdLogger->writeCheckersReport();
 
+    if (settings.safety && mStdLogger->hasCriticalErrors())
+        return EXIT_FAILURE;
+
     if (returnValue)
         return settings.exitCode;
-    return 0;
+    return EXIT_SUCCESS;
 }
 
 void CppCheckExecutor::StdLogger::writeCheckersReport() const
@@ -385,24 +392,29 @@ void CppCheckExecutor::StdLogger::reportProgress(const std::string &filename, co
 
 void CppCheckExecutor::StdLogger::reportErr(const ErrorMessage &msg)
 {
-    if (msg.severity == Severity::none && (msg.id == "logChecker" || endsWith(msg.id, "-logChecker"))) {
+    if (msg.severity == Severity::internal && (msg.id == "logChecker" || endsWith(msg.id, "-logChecker"))) {
         const std::string& checker = msg.shortMessage();
         mActiveCheckers.emplace(checker);
         return;
     }
 
-    // TODO: we generate a different message here then we log below
-    // TODO: there should be no need for verbose and default messages here
-    // Alert only about unique errors
-    if (!mShownErrors.insert(msg.toString(mSettings.verbose)).second)
-        return;
-
     if (ErrorLogger::isCriticalErrorId(msg.id) && mCriticalErrors.find(msg.id) == std::string::npos) {
         if (!mCriticalErrors.empty())
             mCriticalErrors += ", ";
         mCriticalErrors += msg.id;
+        if (msg.severity == Severity::internal)
+            mCriticalErrors += " (suppressed)";
     }
 
+    if (msg.severity == Severity::internal)
+        return;
+
+    // TODO: we generate a different message here then we log below
+    // TODO: there should be no need for verbose and default messages here
+    // Alert only about unique errors
+    if (!mShownErrors.insert(msg.toString(mSettings.verbose)).second)
+        return;
+
     if (mSettings.xml)
         reportErr(msg.toXML());
     else
diff --git a/gui/resultstree.cpp b/gui/resultstree.cpp
@@ -343,6 +343,9 @@ QString ResultsTree::severityToTranslatedString(Severity severity)
     case Severity::debug:
         return tr("debug");
 
+    case Severity::internal:
+        return tr("internal");
+
     case Severity::none:
     default:
         return QString();
@@ -666,6 +669,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
@@ -161,13 +161,16 @@ void ResultsView::progress(int value, const QString& description)
 
 void ResultsView::error(const ErrorItem &item)
 {
-    if (item.severity == Severity::none && (item.errorId == "logChecker" || item.errorId.endsWith("-logChecker"))) {
+    if (item.severity == Severity::internal && (item.errorId == "logChecker" || item.errorId.endsWith("-logChecker"))) {
         mStatistics->addChecker(item.message);
         return;
     }
 
     handleCriticalError(item);
 
+    if (item.severity == Severity::internal)
+        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::internal)
+                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/gui/showtypes.cpp b/gui/showtypes.cpp
@@ -37,6 +37,7 @@ ShowTypes::ShowType ShowTypes::SeverityToShowType(Severity severity)
 {
     switch (severity) {
     case Severity::none:
+    case Severity::internal:
         return ShowTypes::ShowNone;
     case Severity::error:
         return ShowTypes::ShowErrors;
diff --git a/lib/check.cpp b/lib/check.cpp
@@ -128,6 +128,6 @@ ErrorPath Check::getErrorPath(const Token* errtok, const ValueFlow::Value* value
 
 void Check::logChecker(const char id[])
 {
-    reportError(nullptr, Severity::none, "logChecker", id);
+    reportError(nullptr, Severity::internal, "logChecker", id);
 }
 
diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp
@@ -1460,9 +1460,10 @@ void CppCheck::executeAddons(const std::vector<std::string>& files, const std::s
             errmsg.setmsg(text);
             const std::string severity = obj["severity"].get<std::string>();
             errmsg.severity = severityFromString(severity);
-            if (errmsg.severity == Severity::none) {
+            if (errmsg.severity == Severity::none || errmsg.severity == Severity::internal) {
                 if (!endsWith(errmsg.id, "-logChecker"))
                     continue;
+                errmsg.severity = Severity::internal;
             }
             else if (!mSettings.severity.isEnabled(errmsg.severity))
                 continue;
@@ -1568,7 +1569,7 @@ void CppCheck::purgedConfigurationMessage(const std::string &file, const std::st
 // TODO: part of this logic is duplicated in Executor::hasToLog()
 void CppCheck::reportErr(const ErrorMessage &msg)
 {
-    if (msg.severity == Severity::none && (msg.id == "logChecker" || endsWith(msg.id, "-logChecker"))) {
+    if (msg.severity == Severity::internal) {
         mErrorLogger.reportErr(msg);
         return;
     }
@@ -1589,6 +1590,21 @@ void CppCheck::reportErr(const ErrorMessage &msg)
     const auto errorMessage = Suppressions::ErrorMessage::fromErrorMessage(msg, macroNames);
 
     if (mSettings.nomsg.isSuppressed(errorMessage, mUseGlobalSuppressions)) {
+        // Safety: Report critical errors to ErrorLogger
+        if (mSettings.safety && ErrorLogger::isCriticalErrorId(msg.id)) {
+            mExitCode = 1;
+
+            if (mSettings.nomsg.isSuppressedExplicitly(errorMessage, mUseGlobalSuppressions)) {
+                // Report with internal severity to signal that there is this critical error but
+                // it is suppressed
+                ErrorMessage temp(msg);
+                temp.severity = Severity::internal;
+                mErrorLogger.reportErr(temp);
+            } else {
+                // Report critical error that is not explicitly suppressed
+                mErrorLogger.reportErr(msg);
+            }
+        }
         return;
     }
 
diff --git a/lib/errortypes.cpp b/lib/errortypes.cpp
@@ -66,6 +66,8 @@ std::string severityToString(Severity severity)
         return "information";
     case Severity::debug:
         return "debug";
+    case Severity::internal:
+        return "internal";
     }
     throw InternalError(nullptr, "Unknown severity");
 }
@@ -90,5 +92,7 @@ Severity severityFromString(const std::string& severity)
         return Severity::information;
     if (severity == "debug")
         return Severity::debug;
+    if (severity == "internal")
+        return Severity::internal;
     return Severity::none;
 }
diff --git a/lib/errortypes.h b/lib/errortypes.h
@@ -109,7 +109,13 @@ enum class Severity {
      * Debug message.
      * Debug-mode message useful for the developers.
      */
-    debug
+    debug,
+    /**
+     * Internal message.
+     * Message will not be shown to the user.
+     * Tracking what checkers is executed, tracking suppressed critical errors, etc.
+     */
+    internal
 };
 
 CPPCHECKLIB std::string severityToString(Severity severity);
diff --git a/lib/settings.cpp b/lib/settings.cpp
@@ -119,6 +119,15 @@ std::string Settings::loadCppcheckCfg()
             }
         }
     }
+    {
+        const picojson::object::const_iterator it = obj.find("safety");
+        if (it != obj.cend()) {
+            const auto& v = it->second;
+            if (!v.is<bool>())
+                return "'safety' is not a bool";
+            safety = v.get<bool>();
+        }
+    }
 
     return "";
 }
diff --git a/lib/settings.h b/lib/settings.h
@@ -291,6 +291,15 @@ class CPPCHECKLIB WARN_UNUSED Settings {
     std::list<Rule> rules;
 #endif
 
+    /**
+     * @brief Safety certified behavior
+     * Show checkers report when Cppcheck finishes
+     * Make cppcheck checking more strict about critical errors
+     * - returns nonzero if there is critical errors
+     * - a critical error id is not suppressed (by mistake?) with glob pattern
+     */
+    bool safety = false;
+
     /** Do not only check how interface is used. Also check that interface is safe. */
     struct CPPCHECKLIB SafeChecks {
 
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:
+- Added "--safety" option. It makes Cppcheck more strict about critical errors.
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_safety_suppress_syntax_error_implicitly(tmpdir):
+    ret, stdout, stderr = cppcheck(['--safety', '--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_safety_suppress_syntax_error_explicitly():
+    ret, stdout, stderr = cppcheck(['--safety', '--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/fixture.cpp b/test/fixture.cpp
@@ -400,7 +400,7 @@ void TestFixture::reportOut(const std::string & outmsg, Color /*c*/)
 
 void TestFixture::reportErr(const ErrorMessage &msg)
 {
-    if (msg.severity == Severity::none && msg.id == "logChecker")
+    if (msg.severity == Severity::internal)
         return;
     const std::string errormessage(msg.toString(mVerbose, mTemplateFormat, mTemplateLocation));
     // TODO: remove the unique error handling?
diff --git a/tools/run_more_tests.sh b/tools/run_more_tests.sh
@@ -18,6 +18,9 @@ python3 $DIR/extracttests.py --code=$(pwd)/test1 $1
 
 cd test1
 
+# ensure there are not critical errors in extracted tests
+$CPPCHECK -q --safety --suppress="*" .
+
 $CPPCHECK -q --template=cppcheck1 . 2> 1.txt
 
 echo "(!x) => (x==0)"

Original file line number	Diff line number	Diff line change
`@@ -37,6 +37,7 @@ ShowTypes::ShowType ShowTypes::SeverityToShowType(Severity severity)`
`37`	`37`	`{`
`38`	`38`	`switch (severity) {`
`39`	`39`	`case Severity::none:`
	`40`	`+ case Severity::internal:`
`40`	`41`	`return ShowTypes::ShowNone;`
`41`	`42`	`case Severity::error:`
`42`	`43`	`return ShowTypes::ShowErrors;`
Original file line number	Diff line number	Diff line change
`@@ -128,6 +128,6 @@ ErrorPath Check::getErrorPath(const Token* errtok, const ValueFlow::Value* value`
`128`	`128`
`129`	`129`	`void Check::logChecker(const char id[])`
`130`	`130`	`{`
`131`		`- reportError(nullptr, Severity::none, "logChecker", id);`
	`131`	`+ reportError(nullptr, Severity::internal, "logChecker", id);`
`132`	`132`	`}`
`133`	`133`
Original file line number	Diff line number	Diff line change
`@@ -66,6 +66,8 @@ std::string severityToString(Severity severity)`
`66`	`66`	`return "information";`
`67`	`67`	`case Severity::debug:`
`68`	`68`	`return "debug";`
	`69`	`+ case Severity::internal:`
	`70`	`+ return "internal";`
`69`	`71`	`}`
`70`	`72`	`throw InternalError(nullptr, "Unknown severity");`
`71`	`73`	`}`
`@@ -90,5 +92,7 @@ Severity severityFromString(const std::string& severity)`
`90`	`92`	`return Severity::information;`
`91`	`93`	`if (severity == "debug")`
`92`	`94`	`return Severity::debug;`
	`95`	`+ if (severity == "internal")`
	`96`	`+ return Severity::internal;`
`93`	`97`	`return Severity::none;`
`94`	`98`	`}`
Original file line number	Diff line number	Diff line change
`@@ -119,6 +119,15 @@ std::string Settings::loadCppcheckCfg()`
`119`	`119`	`}`
`120`	`120`	`}`
`121`	`121`	`}`
	`122`	`+ {`
	`123`	`+ const picojson::object::const_iterator it = obj.find("safety");`
	`124`	`+ if (it != obj.cend()) {`
	`125`	`+ const auto& v = it->second;`
	`126`	`+ if (!v.is<bool>())`
	`127`	`+ return "'safety' is not a bool";`
	`128`	`+ safety = v.get<bool>();`
	`129`	`+ }`
	`130`	`+ }`
`122`	`131`
`123`	`132`	`return "";`
`124`	`133`	`}`