From 5aa1710dd03f1644e9e249a3f858f0debf5f3c71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Mon, 18 Dec 2023 18:26:23 +0100 Subject: [PATCH] Fix #12071 (Add safety mode that makes cppcheck more strict about critical errors) (#5777) --- cli/cmdlineparser.cpp | 4 ++++ cli/cppcheckexecutor.cpp | 28 +++++++++++++++++++-------- gui/resultstree.cpp | 8 ++++++++ gui/resultsview.cpp | 9 ++++++++- gui/showtypes.cpp | 1 + lib/check.cpp | 2 +- lib/cppcheck.cpp | 20 +++++++++++++++++-- lib/errortypes.cpp | 4 ++++ lib/errortypes.h | 8 +++++++- lib/settings.cpp | 9 +++++++++ lib/settings.h | 9 +++++++++ lib/suppressions.cpp | 13 +++++++++++++ lib/suppressions.h | 8 ++++++++ releasenotes.txt | 3 +++ test/cli/test-suppress-syntaxError.py | 12 ++++++++++++ test/cli/testutils.py | 4 ++-- test/fixture.cpp | 2 +- tools/run_more_tests.sh | 3 +++ 18 files changed, 131 insertions(+), 16 deletions(-) diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp index 417c46e5a..82ef08a41 100644 --- 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 index 296a2ad8e..364beef55 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -118,6 +118,10 @@ public: */ 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; } + 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 (ErrorLogger::isCriticalErrorId(msg.id) && mCriticalErrors.find(msg.id) == std::string::npos) { - if (!mCriticalErrors.empty()) - mCriticalErrors += ", "; - mCriticalErrors += msg.id; - } - if (mSettings.xml) reportErr(msg.toXML()); else diff --git a/gui/resultstree.cpp b/gui/resultstree.cpp index e73475cf8..16dbbc628 100644 --- 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 index 2b5baa5fb..b073b6625 100644 --- 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 index 4ece3ae6a..59d19c263 100644 --- 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 index 53b4e5fed..c5e6368f1 100644 --- 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 index 9d5b0537c..9f912697d 100644 --- a/lib/cppcheck.cpp +++ b/lib/cppcheck.cpp @@ -1460,9 +1460,10 @@ void CppCheck::executeAddons(const std::vector& files, const std::s errmsg.setmsg(text); const std::string severity = obj["severity"].get(); 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 index a7028d89b..efdfd21e9 100644 --- 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 index ff83d6a47..3e9a6a414 100644 --- 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 index 54c96f672..a58e75b2c 100644 --- 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()) + return "'safety' is not a bool"; + safety = v.get(); + } + } return ""; } diff --git a/lib/settings.h b/lib/settings.h index 6833d8569..be106ef98 100644 --- a/lib/settings.h +++ b/lib/settings.h @@ -291,6 +291,15 @@ public: std::list 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 index 782d657d1..e5f4e7b47 100644 --- 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& macroNames) { if (mSuppressions.empty()) diff --git a/lib/suppressions.h b/lib/suppressions.h index d859ed02f..e6f6da830 100644 --- a/lib/suppressions.h +++ b/lib/suppressions.h @@ -202,6 +202,14 @@ public: */ 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 index 75d4427d6..41295d325 100644 --- 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 index 00d383eab..b2293bc84 100644 --- 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 index 1dd932a9e..dd529e1b6 100644 --- 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 index 67d022bc3..1a72724a0 100644 --- 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 index 12633c41f..413ff8892 100755 --- 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)"