From c1aad915d7db2bc4b4d27eda761ef5b02c6106c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sat, 5 Oct 2024 15:40:14 +0200 Subject: [PATCH 01/10] Fix #9972 (Add support for SARIF output format) --- Makefile | 4 +- cli/CMakeLists.txt | 1 + cli/cli.vcxproj | 8 +- cli/cmdlineparser.cpp | 24 ++++- cli/cppcheckexecutor.cpp | 124 ++++++++++++++++++++++++- lib/settings.h | 3 + releasenotes.txt | 2 + samples/incorrectLogicOperator/bad.c | 6 ++ samples/incorrectLogicOperator/good.c | 6 ++ samples/incorrectLogicOperator/out.txt | 3 + test/cli/helloworld_test.py | 15 +++ test/testcmdlineparser.cpp | 40 ++++++++ test/testrunner.vcxproj | 8 +- tools/dmake/dmake.cpp | 2 +- 14 files changed, 233 insertions(+), 13 deletions(-) create mode 100644 samples/incorrectLogicOperator/bad.c create mode 100644 samples/incorrectLogicOperator/good.c create mode 100644 samples/incorrectLogicOperator/out.txt diff --git a/Makefile b/Makefile index 96c3557ac1c..c06377cb79a 100644 --- a/Makefile +++ b/Makefile @@ -173,7 +173,7 @@ ifndef INCLUDE_FOR_LIB endif ifndef INCLUDE_FOR_CLI - INCLUDE_FOR_CLI=-Ilib -isystem externals/simplecpp -isystem externals/tinyxml2 + INCLUDE_FOR_CLI=-Ilib -isystem externals/picojson -isystem externals/simplecpp -isystem externals/tinyxml2 endif ifndef INCLUDE_FOR_TEST @@ -762,7 +762,7 @@ $(libcppdir)/vfvalue.o: lib/vfvalue.cpp lib/config.h lib/errortypes.h lib/mathli cli/cmdlineparser.o: cli/cmdlineparser.cpp cli/cmdlinelogger.h cli/cmdlineparser.h cli/filelister.h externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/filesettings.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/pathmatch.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/timer.h lib/utils.h lib/xml.h $(CXX) ${INCLUDE_FOR_CLI} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ cli/cmdlineparser.cpp -cli/cppcheckexecutor.o: cli/cppcheckexecutor.cpp cli/cmdlinelogger.h cli/cmdlineparser.h cli/cppcheckexecutor.h cli/cppcheckexecutorseh.h cli/executor.h cli/processexecutor.h cli/signalhandler.h cli/singleexecutor.h cli/threadexecutor.h lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/checkersreport.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/filesettings.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h +cli/cppcheckexecutor.o: cli/cppcheckexecutor.cpp cli/cmdlinelogger.h cli/cmdlineparser.h cli/cppcheckexecutor.h cli/cppcheckexecutorseh.h cli/executor.h cli/processexecutor.h cli/signalhandler.h cli/singleexecutor.h cli/threadexecutor.h externals/picojson/picojson.h lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/checkersreport.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/filesettings.h lib/json.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h $(CXX) ${INCLUDE_FOR_CLI} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ cli/cppcheckexecutor.cpp cli/cppcheckexecutorseh.o: cli/cppcheckexecutorseh.cpp cli/cppcheckexecutor.h cli/cppcheckexecutorseh.h lib/config.h lib/filesettings.h lib/path.h lib/platform.h lib/standards.h lib/utils.h diff --git a/cli/CMakeLists.txt b/cli/CMakeLists.txt index 51e9aee6718..c05743becb3 100644 --- a/cli/CMakeLists.txt +++ b/cli/CMakeLists.txt @@ -12,6 +12,7 @@ if (BUILD_CLI) else() target_include_directories(cli_objs SYSTEM PRIVATE ${tinyxml2_INCLUDE_DIRS}) endif() + target_externals_include_directories(cli_objs PRIVATE ${PROJECT_SOURCE_DIR}/externals/picojson/) target_externals_include_directories(cli_objs PRIVATE ${PROJECT_SOURCE_DIR}/externals/simplecpp/) if (NOT CMAKE_DISABLE_PRECOMPILE_HEADERS) target_precompile_headers(cli_objs PRIVATE precompiled.h) diff --git a/cli/cli.vcxproj b/cli/cli.vcxproj index f532fa46e7d..a508d3bf55b 100644 --- a/cli/cli.vcxproj +++ b/cli/cli.vcxproj @@ -85,7 +85,7 @@ - ..\lib;..\externals;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) + ..\lib;..\externals;..\externals\picojson;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) true ProgramDatabase Disabled @@ -114,7 +114,7 @@ - ..\lib;..\externals;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) + ..\lib;..\externals;..\externals\picojson;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) true ProgramDatabase Disabled @@ -143,7 +143,7 @@ - ..\lib;..\externals;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) + ..\lib;..\externals;..\externals\picojson;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) false MaxSpeed CPPCHECKLIB_IMPORT;TINYXML2_IMPORT;NDEBUG;WIN32;_CRT_SECURE_NO_WARNINGS;WIN32_LEAN_AND_MEAN;_WIN64;%(PreprocessorDefinitions) @@ -181,7 +181,7 @@ - ..\lib;..\externals;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) + ..\lib;..\externals;..\externals\picojson;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) false MaxSpeed CPPCHECKLIB_IMPORT;TINYXML2_IMPORT;NDEBUG;WIN32;HAVE_RULES;_CRT_SECURE_NO_WARNINGS;WIN32_LEAN_AND_MEAN;_WIN64;%(PreprocessorDefinitions) diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp index 11923440ea6..5ec8faafad1 100644 --- a/cli/cmdlineparser.cpp +++ b/cli/cmdlineparser.cpp @@ -915,6 +915,20 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a else if (std::strncmp(argv[i], "--output-file=", 14) == 0) mSettings.outputFile = Path::simplifyPath(argv[i] + 14); + else if (std::strncmp(argv[i], "--output-format=", 16) == 0) { + const std::string format = argv[i] + 16; + if (format == "sarif") + mSettings.outputFormat = Settings::OutputFormat::sarif; + else if (format == "xml") + mSettings.outputFormat = Settings::OutputFormat::xml; + else { + mLogger.printError("argument to '--output-format=' must be 'sarif' or 'xml'."); + return Result::Fail; + } + mSettings.xml = (mSettings.outputFormat == Settings::OutputFormat::xml); + } + + // Experimental: limit execution time for extended valueflow analysis. basic valueflow analysis // is always executed. else if (std::strncmp(argv[i], "--performance-valueflow-max-time=", 33) == 0) { @@ -949,6 +963,7 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a // Write results in results.plist else if (std::strncmp(argv[i], "--plist-output=", 15) == 0) { + mSettings.outputFormat = Settings::OutputFormat::plist; mSettings.plistOutput = Path::simplifyPath(argv[i] + 15); if (mSettings.plistOutput.empty()) mSettings.plistOutput = "."; @@ -1361,8 +1376,10 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a mSettings.verbose = true; // Write results in results.xml - else if (std::strcmp(argv[i], "--xml") == 0) + else if (std::strcmp(argv[i], "--xml") == 0) { mSettings.xml = true; + mSettings.outputFormat = Settings::OutputFormat::xml; + } // Define the XML file version (and enable XML output) else if (std::strncmp(argv[i], "--xml-version=", 14) == 0) { @@ -1378,6 +1395,7 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a mSettings.xml_version = tmp; // Enable also XML if version is set mSettings.xml = true; + mSettings.outputFormat = Settings::OutputFormat::xml; } else { @@ -1623,6 +1641,10 @@ void CmdLineParser::printHelp() const " is 2. A larger value will mean more errors can be found\n" " but also means the analysis will be slower.\n" " --output-file= Write results to file, rather than standard error.\n" + " --output-format=\n" + " Specify the output format. The available formats are:\n" + " * sarif\n" + " * xml\n" " --platform=, --platform=\n" " Specifies platform specific types and sizes. The\n" " available builtin platforms are:\n" diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index 2080d9e2f7f..7c98a3f993f 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -28,6 +28,7 @@ #include "errorlogger.h" #include "errortypes.h" #include "filesettings.h" +#include "json.h" #include "settings.h" #include "singleexecutor.h" #include "suppressions.h" @@ -71,6 +72,117 @@ #endif namespace { + class SarifReport { + public: + void addFinding(ErrorMessage msg) { + mFindings.push_back(std::move(msg)); + } + + picojson::array serializeRules() const { + picojson::array ret; + std::set ruleIds; + for (const auto& finding : mFindings) { + if (ruleIds.insert(finding.id).second) { + picojson::object rule; + rule["id"] = picojson::value(finding.id); + picojson::object shortDescription; + shortDescription["text"] = picojson::value(finding.shortMessage()); + rule["shortDescription"] = picojson::value(shortDescription); + ret.emplace_back(rule); + } + } + return ret; + } + + static picojson::array serializeLocations(const ErrorMessage& finding) { + picojson::array ret; + for (const auto& location : finding.callStack) { + picojson::object physicalLocation; + picojson::object artifactLocation; + artifactLocation["uri"] = picojson::value(location.getOrigFile(false)); + physicalLocation["artifactLocation"] = picojson::value(artifactLocation); + picojson::object region; + region["startLine"] = picojson::value(static_cast(location.line)); + region["startColumn"] = picojson::value(static_cast(location.column)); + physicalLocation["region"] = picojson::value(region); + picojson::object loc; + loc["physicalLocation"] = picojson::value(physicalLocation); + ret.emplace_back(loc); + } + return ret; + } + + picojson::array serializeResults() const { + picojson::array results; + for (const auto& finding : mFindings) { + picojson::object res; + res["level"] = picojson::value(sarifLevel(finding.severity)); + if (!finding.callStack.empty()) + res["locations"] = picojson::value(serializeLocations(finding)); + picojson::object message; + message["text"] = picojson::value(finding.shortMessage()); + res["message"] = picojson::value(message); + res["ruleId"] = picojson::value(finding.id); + results.emplace_back(res); + } + return results; + } + + picojson::value serializeRuns(const std::string& productName, const std::string& version) const { + picojson::object driver; + driver["name"] = picojson::value(productName); + driver["version"] = picojson::value(version); + driver["informationUri"] = picojson::value("https://cppcheck.sourceforge.io"); + driver["rules"] = picojson::value(serializeRules()); + picojson::object tool; + tool["driver"] = picojson::value(driver); + picojson::object run; + run["tool"] = picojson::value(tool); + run["results"] = picojson::value(serializeResults()); + picojson::array runs{picojson::value(run)}; + return picojson::value(runs); + } + + std::string serialize(std::string productName) const { + const auto nameAndVersion = Settings::getNameAndVersion(productName); + productName = nameAndVersion.first.empty() ? "Cppcheck" : nameAndVersion.first; + std::string version = nameAndVersion.first.empty() ? CppCheck::version() : nameAndVersion.second; + if (version.find(' ') != std::string::npos) + version.erase(version.find(' '), std::string::npos); + + picojson::object doc; + doc["version"] = picojson::value("2.1.0"); + doc["$schema"] = picojson::value("https://docs.oasis-open.org/sarif/sarif/v2.1.0/errata01/os/schemas/sarif-schema-2.1.0.json"); + doc["runs"] = serializeRuns(productName, version); + + return picojson::value(doc).serialize(true); + } + private: + + + static std::string sarifLevel(Severity severity) { + switch (severity) { + case Severity::error: + return "error"; + case Severity::warning: + case Severity::style: + case Severity::portability: + case Severity::performance: + return "warning"; + case Severity::information: + case Severity::internal: + case Severity::debug: + case Severity::none: + return "note"; + } + return "note"; + } + + + + std::vector mFindings; + }; + class CmdLineLoggerStd : public CmdLineLogger { public: @@ -104,6 +216,9 @@ namespace { } ~StdLogger() override { + if (mSettings.outputFormat == Settings::OutputFormat::sarif) { + reportErr(mSarifReport.serialize(mSettings.cppcheckCfgProductName)); + } delete mErrorOutput; } @@ -182,6 +297,11 @@ namespace { * CTU information */ std::string mCtuInfo; + + /** + * SARIF report generator + */ + SarifReport mSarifReport; }; } @@ -455,7 +575,9 @@ void StdLogger::reportErr(const ErrorMessage &msg) if (!mShownErrors.insert(msg.toString(mSettings.verbose)).second) return; - if (mSettings.xml) + if (mSettings.outputFormat == Settings::OutputFormat::sarif) + mSarifReport.addFinding(msg); + else if (mSettings.xml) reportErr(msg.toXML()); else reportErr(msg.toString(mSettings.verbose, mSettings.templateFormat, mSettings.templateLocation)); diff --git a/lib/settings.h b/lib/settings.h index d886cab0a40..daf1a8d6d43 100644 --- a/lib/settings.h +++ b/lib/settings.h @@ -273,6 +273,9 @@ class CPPCHECKLIB WARN_UNUSED Settings { /** @brief write results (--output-file=<file>) */ std::string outputFile; + enum class OutputFormat : std::uint8_t {text, plist, sarif, xml}; + OutputFormat outputFormat = OutputFormat::text; + Platform platform; /** @brief pid of cppcheck. Intention is that this is set in the main process. */ diff --git a/releasenotes.txt b/releasenotes.txt index 8c0edafd07f..ffee14f39fd 100644 --- a/releasenotes.txt +++ b/releasenotes.txt @@ -10,6 +10,8 @@ GUI: - Changed interface: +- SARIF output. Use --output-format=sarif to activate this. +- Add option --output-format=. Allowed formats are sarif and xml. - Deprecations: diff --git a/samples/incorrectLogicOperator/bad.c b/samples/incorrectLogicOperator/bad.c new file mode 100644 index 00000000000..11daaf4e4ce --- /dev/null +++ b/samples/incorrectLogicOperator/bad.c @@ -0,0 +1,6 @@ + +void foo(int x) { + if (x >= 0 || x <= 10) {} +} + +dummy=foo(); diff --git a/samples/incorrectLogicOperator/good.c b/samples/incorrectLogicOperator/good.c new file mode 100644 index 00000000000..a2d122fb128 --- /dev/null +++ b/samples/incorrectLogicOperator/good.c @@ -0,0 +1,6 @@ + +void foo(int x) { + if (x >= 0 && x <= 10) {} +} + +dummy=foo(); diff --git a/samples/incorrectLogicOperator/out.txt b/samples/incorrectLogicOperator/out.txt new file mode 100644 index 00000000000..e6cf2c5f76f --- /dev/null +++ b/samples/incorrectLogicOperator/out.txt @@ -0,0 +1,3 @@ +samples\incorrectLogicOperator\bad.c:3:16: warning: Logical disjunction always evaluates to true: x >= 0 || x <= 10. [incorrectLogicOperator] + if (x >= 0 || x <= 10) {} + ^ diff --git a/test/cli/helloworld_test.py b/test/cli/helloworld_test.py index 9a5056f17dc..663170c5fb6 100644 --- a/test/cli/helloworld_test.py +++ b/test/cli/helloworld_test.py @@ -4,6 +4,7 @@ import os import re import glob +import json from testutils import create_gui_project_file, cppcheck @@ -319,3 +320,17 @@ def test_missing_include_system(): # #11283 _, _, stderr = cppcheck(args, cwd=__script_dir) assert stderr.replace('\\', '/') == 'helloworld/main.c:1:0: information: Include file: not found. Please note: Cppcheck does not need standard library headers to get proper results. [missingIncludeSystem]\n' + + +def test_sarif(): + args = [ + '--output-format=sarif', + 'helloworld' + ] + ret, stdout, stderr = cppcheck(args, cwd=__script_dir) + assert ret == 0, stdout + res = json.loads(stderr) + assert res['version'] == '2.1.0' + assert res['runs'][0]['results'][0]['locations'][0]['physicalLocation']['artifactLocation']['uri'] == 'helloworld/main.c' + assert res['runs'][0]['results'][0]['ruleId'] == 'zerodiv' + assert res['runs'][0]['results'][0]['message']['text'] == 'Division by zero.' diff --git a/test/testcmdlineparser.cpp b/test/testcmdlineparser.cpp index 4e262b8edb7..0ffa3198f3a 100644 --- a/test/testcmdlineparser.cpp +++ b/test/testcmdlineparser.cpp @@ -207,6 +207,11 @@ class TestCmdlineParser : public TestFixture { TEST_CASE(maxConfigsMissingCount); TEST_CASE(maxConfigsInvalid); TEST_CASE(maxConfigsTooSmall); + TEST_CASE(outputFormatSarif); + TEST_CASE(outputFormatXml); + TEST_CASE(outputFormatOther); + TEST_CASE(outputFormatImplicitPlist); + TEST_CASE(outputFormatImplicitXml); TEST_CASE(premiumOptions1); TEST_CASE(premiumOptions2); TEST_CASE(premiumOptions3); @@ -1246,6 +1251,41 @@ class TestCmdlineParser : public TestFixture { ASSERT_EQUALS("cppcheck: error: argument to '--max-configs=' must be greater than 0.\n", logger->str()); } + void outputFormatSarif() { + REDIRECT; + const char * const argv[] = {"cppcheck", "--output-format=sarif", "file.cpp"}; + ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv)); + ASSERT_EQUALS_ENUM(Settings::OutputFormat::sarif, settings->outputFormat); + } + + void outputFormatXml() { + REDIRECT; + const char * const argv[] = {"cppcheck", "--output-format=xml", "file.cpp"}; + ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv)); + ASSERT_EQUALS_ENUM(Settings::OutputFormat::xml, settings->outputFormat); + } + + void outputFormatOther() { + REDIRECT; + const char * const argv[] = {"cppcheck", "--output-format=text", "file.cpp"}; + ASSERT_EQUALS_ENUM(CmdLineParser::Result::Fail, parser->parseFromArgs(3, argv)); + ASSERT_EQUALS("cppcheck: error: argument to '--output-format=' must be 'sarif' or 'xml'.\n", logger->str()); + } + + void outputFormatImplicitPlist() { + REDIRECT; + const char * const argv[] = {"cppcheck", "--plist-output=.", "file.cpp"}; + ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv)); + ASSERT_EQUALS_ENUM(Settings::OutputFormat::plist, settings->outputFormat); + } + + void outputFormatImplicitXml() { + REDIRECT; + const char * const argv[] = {"cppcheck", "--xml", "file.cpp"}; + ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv)); + ASSERT_EQUALS_ENUM(Settings::OutputFormat::xml, settings->outputFormat); + } + void premiumOptions1() { REDIRECT; asPremium(); diff --git a/test/testrunner.vcxproj b/test/testrunner.vcxproj index 5951a2770ba..89ab1d43a4c 100755 --- a/test/testrunner.vcxproj +++ b/test/testrunner.vcxproj @@ -197,7 +197,7 @@ - ..\cli;..\lib;..\externals;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) + ..\cli;..\lib;..\externals;..\externals\picojson;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) true ProgramDatabase Disabled @@ -226,7 +226,7 @@ - ..\cli;..\lib;..\externals;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) + ..\cli;..\lib;..\externals;..\externals\picojson;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) true ProgramDatabase Disabled @@ -255,7 +255,7 @@ - ..\cli;..\lib;..\externals;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) + ..\cli;..\lib;..\externals;..\externals\picojson;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) false MaxSpeed CPPCHECKLIB_IMPORT;SIMPLECPP_IMPORT;NDEBUG;WIN32;_CRT_SECURE_NO_WARNINGS;WIN32_LEAN_AND_MEAN;_WIN64;%(PreprocessorDefinitions) @@ -295,7 +295,7 @@ - ..\cli;..\lib;..\externals;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) + ..\cli;..\lib;..\externals;..\externals\picojson;..\externals\simplecpp;..\externals\tinyxml2;%(AdditionalIncludeDirectories) false MaxSpeed CPPCHECKLIB_IMPORT;SIMPLECPP_IMPORT;NDEBUG;WIN32;HAVE_RULES;_CRT_SECURE_NO_WARNINGS;WIN32_LEAN_AND_MEAN;_WIN64;%(PreprocessorDefinitions) diff --git a/tools/dmake/dmake.cpp b/tools/dmake/dmake.cpp index 3b50040779b..6e11eff7caa 100644 --- a/tools/dmake/dmake.cpp +++ b/tools/dmake/dmake.cpp @@ -754,7 +754,7 @@ int main(int argc, char **argv) makeConditionalVariable(fout, "PREFIX", "/usr"); makeConditionalVariable(fout, "INCLUDE_FOR_LIB", "-Ilib -isystem externals -isystem externals/picojson -isystem externals/simplecpp -isystem externals/tinyxml2"); - makeConditionalVariable(fout, "INCLUDE_FOR_CLI", "-Ilib -isystem externals/simplecpp -isystem externals/tinyxml2"); + makeConditionalVariable(fout, "INCLUDE_FOR_CLI", "-Ilib -isystem externals/picojson -isystem externals/simplecpp -isystem externals/tinyxml2"); makeConditionalVariable(fout, "INCLUDE_FOR_TEST", "-Ilib -Icli -isystem externals/simplecpp -isystem externals/tinyxml2"); fout << "BIN=$(DESTDIR)$(PREFIX)/bin\n\n"; From 84d128f02af60ed3e91dcaf16494e94c6240ec6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Wed, 9 Oct 2024 15:05:28 +0200 Subject: [PATCH 02/10] review --- cli/cppcheckexecutor.cpp | 54 ++++++++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index 7c98a3f993f..e066fcc4ec4 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -82,12 +82,32 @@ namespace { picojson::array ret; std::set ruleIds; for (const auto& finding : mFindings) { + // github only supports findings with locations + if (finding.callStack.empty()) + continue; if (ruleIds.insert(finding.id).second) { picojson::object rule; rule["id"] = picojson::value(finding.id); + // rule.shortDescription.text picojson::object shortDescription; shortDescription["text"] = picojson::value(finding.shortMessage()); rule["shortDescription"] = picojson::value(shortDescription); + // rule.fullDescription.text + picojson::object fullDescription; + fullDescription["text"] = picojson::value(finding.verboseMessage()); + rule["fullDescription"] = picojson::value(fullDescription); + // rule.help.text + picojson::object help; + help["text"] = picojson::value(finding.verboseMessage()); // FIXME provide proper help text + rule["help"] = picojson::value(help); + // rule.properties.precision, rule.properties.problem.severity + picojson::object properties; + properties["precision"] = picojson::value(sarifPrecision(finding)); + picojson::object properties_problem; + properties_problem["severity"] = picojson::value(sarifSeverity(finding)); + properties["problem"] = picojson::value(properties_problem); + rule["properties"] = picojson::value(properties); + ret.emplace_back(rule); } } @@ -104,6 +124,8 @@ namespace { picojson::object region; region["startLine"] = picojson::value(static_cast(location.line)); region["startColumn"] = picojson::value(static_cast(location.column)); + region["endLine"] = region["startLine"]; + region["endColumn"] = region["startColumn"]; physicalLocation["region"] = picojson::value(region); picojson::object loc; loc["physicalLocation"] = picojson::value(physicalLocation); @@ -115,14 +137,20 @@ namespace { picojson::array serializeResults() const { picojson::array results; for (const auto& finding : mFindings) { + // github only supports findings with locations + if (finding.callStack.empty()) + continue; picojson::object res; - res["level"] = picojson::value(sarifLevel(finding.severity)); - if (!finding.callStack.empty()) - res["locations"] = picojson::value(serializeLocations(finding)); + res["level"] = picojson::value(sarifSeverity(finding)); + res["locations"] = picojson::value(serializeLocations(finding)); picojson::object message; message["text"] = picojson::value(finding.shortMessage()); res["message"] = picojson::value(message); res["ruleId"] = picojson::value(finding.id); + // partialFingerprints.hash + picojson::object partialFingerprints; + partialFingerprints["hash"] = picojson::value(getHash(finding)); + res["partialFingerprints"] = picojson::value(partialFingerprints); results.emplace_back(res); } return results; @@ -159,11 +187,11 @@ namespace { } private: - - static std::string sarifLevel(Severity severity) { - switch (severity) { - case Severity::error: + static std::string sarifSeverity(const ErrorMessage& errmsg) { + if (ErrorLogger::isCriticalErrorId(errmsg.id)) return "error"; + switch (errmsg.severity) { + case Severity::error: case Severity::warning: case Severity::style: case Severity::portability: @@ -178,7 +206,19 @@ namespace { return "note"; } + static std::string sarifPrecision(const ErrorMessage& errmsg) { + if (errmsg.certainty == Certainty::inconclusive) + return "normal"; + return "high"; + } + std::string getHash(const ErrorMessage& errmsg) const { + const std::string s = errmsg.toString(false, "{file}:{line}:{column}: {message} {id} {code}", "{file}:{line}:{column} {info} {code}"); + std::ostringstream os; + //std::cout << s << std::endl; + os << std::hex << std::hash {}(s); + return os.str(); + } std::vector mFindings; }; From 7a80e05af2f180317423fdf300427516e2d7fd06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Wed, 9 Oct 2024 15:10:47 +0200 Subject: [PATCH 03/10] relative filename --- cli/cppcheckexecutor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index e066fcc4ec4..482a8e4ace3 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -119,7 +119,7 @@ namespace { for (const auto& location : finding.callStack) { picojson::object physicalLocation; picojson::object artifactLocation; - artifactLocation["uri"] = picojson::value(location.getOrigFile(false)); + artifactLocation["uri"] = picojson::value(location.getfile(false)); physicalLocation["artifactLocation"] = picojson::value(artifactLocation); picojson::object region; region["startLine"] = picojson::value(static_cast(location.line)); From 8680b3aa5229b6e5fbb8490020fb1da23aca88c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Wed, 9 Oct 2024 16:16:55 +0200 Subject: [PATCH 04/10] fix warning --- cli/cppcheckexecutor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index 482a8e4ace3..cca07d0095b 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -212,7 +212,7 @@ namespace { return "high"; } - std::string getHash(const ErrorMessage& errmsg) const { + static std::string getHash(const ErrorMessage& errmsg) { const std::string s = errmsg.toString(false, "{file}:{line}:{column}: {message} {id} {code}", "{file}:{line}:{column} {info} {code}"); std::ostringstream os; //std::cout << s << std::endl; From 9e1db25383f25c7dd0ae404389ad54ac5fa23b0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Wed, 9 Oct 2024 21:42:02 +0200 Subject: [PATCH 05/10] Update cli/cppcheckexecutor.cpp Co-authored-by: Mario Campos --- cli/cppcheckexecutor.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index cca07d0095b..595088bbe03 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -103,9 +103,7 @@ namespace { // rule.properties.precision, rule.properties.problem.severity picojson::object properties; properties["precision"] = picojson::value(sarifPrecision(finding)); - picojson::object properties_problem; - properties_problem["severity"] = picojson::value(sarifSeverity(finding)); - properties["problem"] = picojson::value(properties_problem); + properties["problem.severity"] = picojson::value(sarifSeverity(finding)); rule["properties"] = picojson::value(properties); ret.emplace_back(rule); From 5d76a944e03a9aa0a8a9f9324b83633352df0946 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Wed, 9 Oct 2024 21:46:29 +0200 Subject: [PATCH 06/10] Update cli/cppcheckexecutor.cpp Co-authored-by: Mario Campos --- cli/cppcheckexecutor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index 595088bbe03..ea9ea273bee 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -157,7 +157,7 @@ namespace { picojson::value serializeRuns(const std::string& productName, const std::string& version) const { picojson::object driver; driver["name"] = picojson::value(productName); - driver["version"] = picojson::value(version); + driver["semanticVersion"] = picojson::value(version); driver["informationUri"] = picojson::value("https://cppcheck.sourceforge.io"); driver["rules"] = picojson::value(serializeRules()); picojson::object tool; From fb9ca692acc37f5dad6f2e6a9d0a163b6cb35f36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Wed, 9 Oct 2024 22:23:15 +0200 Subject: [PATCH 07/10] review comments --- cli/cppcheckexecutor.cpp | 24 +++++++++++------------- test/cli/helloworld_test.py | 7 ++++++- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index ea9ea273bee..19794b63a57 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -104,6 +104,16 @@ namespace { picojson::object properties; properties["precision"] = picojson::value(sarifPrecision(finding)); properties["problem.severity"] = picojson::value(sarifSeverity(finding)); + double securitySeverity = 0; + if (finding.severity == Severity::error && !ErrorLogger::isCriticalErrorId(finding.id)) + securitySeverity = 9.9; // We see undefined behavior + //else if (finding.severity == Severity::warning) + // securitySeverity = 5.1; // We see potential undefined behavior + if (securitySeverity > 0) { + properties["security-severity"] = picojson::value(securitySeverity); + const picojson::array tags{picojson::value("security")}; + properties["tags"] = picojson::value(tags); + } rule["properties"] = picojson::value(properties); ret.emplace_back(rule); @@ -145,10 +155,6 @@ namespace { message["text"] = picojson::value(finding.shortMessage()); res["message"] = picojson::value(message); res["ruleId"] = picojson::value(finding.id); - // partialFingerprints.hash - picojson::object partialFingerprints; - partialFingerprints["hash"] = picojson::value(getHash(finding)); - res["partialFingerprints"] = picojson::value(partialFingerprints); results.emplace_back(res); } return results; @@ -206,18 +212,10 @@ namespace { static std::string sarifPrecision(const ErrorMessage& errmsg) { if (errmsg.certainty == Certainty::inconclusive) - return "normal"; + return "medium"; return "high"; } - static std::string getHash(const ErrorMessage& errmsg) { - const std::string s = errmsg.toString(false, "{file}:{line}:{column}: {message} {id} {code}", "{file}:{line}:{column} {info} {code}"); - std::ostringstream os; - //std::cout << s << std::endl; - os << std::hex << std::hash {}(s); - return os.str(); - } - std::vector mFindings; }; diff --git a/test/cli/helloworld_test.py b/test/cli/helloworld_test.py index 663170c5fb6..48aec331126 100644 --- a/test/cli/helloworld_test.py +++ b/test/cli/helloworld_test.py @@ -333,4 +333,9 @@ def test_sarif(): assert res['version'] == '2.1.0' assert res['runs'][0]['results'][0]['locations'][0]['physicalLocation']['artifactLocation']['uri'] == 'helloworld/main.c' assert res['runs'][0]['results'][0]['ruleId'] == 'zerodiv' - assert res['runs'][0]['results'][0]['message']['text'] == 'Division by zero.' + assert res['runs'][0]['tool']['driver']['rules'][0]['id'] == 'zerodiv' + assert res['runs'][0]['tool']['driver']['rules'][0]['properties']['precision'] == 'high' + assert res['runs'][0]['tool']['driver']['rules'][0]['properties']['problem.severity'] == 'warning' + assert res['runs'][0]['tool']['driver']['rules'][0]['properties']['security-severity'] > 9.5 + assert 'security' in res['runs'][0]['tool']['driver']['rules'][0]['properties']['tags'] + assert re.match(r'[0-9]+(.[0-9]+)+', res['runs'][0]['tool']['driver']['semanticVersion']) From df6d289da6fe1e8a86ffb4cfb2276e4967894040 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Wed, 9 Oct 2024 22:29:40 +0200 Subject: [PATCH 08/10] fix --- cli/cppcheckexecutor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index 19794b63a57..b3adc60b256 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -109,7 +109,7 @@ namespace { securitySeverity = 9.9; // We see undefined behavior //else if (finding.severity == Severity::warning) // securitySeverity = 5.1; // We see potential undefined behavior - if (securitySeverity > 0) { + if (securitySeverity > 0.5) { properties["security-severity"] = picojson::value(securitySeverity); const picojson::array tags{picojson::value("security")}; properties["tags"] = picojson::value(tags); From 816542c84b39afb25c91414f1ee0d74324d49894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 10 Oct 2024 20:22:52 +0200 Subject: [PATCH 09/10] Update cli/cppcheckexecutor.cpp Co-authored-by: Mario Campos --- cli/cppcheckexecutor.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index b3adc60b256..81253b020d2 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -103,7 +103,6 @@ namespace { // rule.properties.precision, rule.properties.problem.severity picojson::object properties; properties["precision"] = picojson::value(sarifPrecision(finding)); - properties["problem.severity"] = picojson::value(sarifSeverity(finding)); double securitySeverity = 0; if (finding.severity == Severity::error && !ErrorLogger::isCriticalErrorId(finding.id)) securitySeverity = 9.9; // We see undefined behavior From 26ca959eed020b5db175bdb05991163094d8418e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 10 Oct 2024 20:26:26 +0200 Subject: [PATCH 10/10] test --- test/cli/helloworld_test.py | 1 - 1 file changed, 1 deletion(-) diff --git a/test/cli/helloworld_test.py b/test/cli/helloworld_test.py index 48aec331126..6ee09396c40 100644 --- a/test/cli/helloworld_test.py +++ b/test/cli/helloworld_test.py @@ -335,7 +335,6 @@ def test_sarif(): assert res['runs'][0]['results'][0]['ruleId'] == 'zerodiv' assert res['runs'][0]['tool']['driver']['rules'][0]['id'] == 'zerodiv' assert res['runs'][0]['tool']['driver']['rules'][0]['properties']['precision'] == 'high' - assert res['runs'][0]['tool']['driver']['rules'][0]['properties']['problem.severity'] == 'warning' assert res['runs'][0]['tool']['driver']['rules'][0]['properties']['security-severity'] > 9.5 assert 'security' in res['runs'][0]['tool']['driver']['rules'][0]['properties']['tags'] assert re.match(r'[0-9]+(.[0-9]+)+', res['runs'][0]['tool']['driver']['semanticVersion'])