Add SARIF output support. #4651

mario-campos · 2022-12-16T22:27:06Z

This has been on my TODO list for a while! I've been wanting to integrate CppCheck's results into GitHub since Code Scanning came out. To do that, however, the tool needs to spit out SARIF.

To test it out, run cppcheck --output-format=sarif --quiet foo.c

Now, this is only an initial stab, and it's probably a bit ugly. I'm not quite done just yet (hence why it's a draft PR). For one, it's not truly outputting the correct line/column numbers. So, if you have any suggestions or comments, please let me know.

Also, I initially tried to work SARIF output into the original design by adding a ErrorMessage::toSARIF method (ala ErrorMessage::toXML), but I quickly realized that this wouldn't work very well, since the SARIF document has to be "constructed", not "streamed" the way the XML document is. Which meant that I had to rework the way that findings ("errors") were collected and aggregated, since the SARIF document kinda needs to see everything all at once.

cli/cppcheckexecutor.cpp

firewave · 2022-12-17T18:47:42Z

Thanks for your contribution.

Without actually looking at what it does - please add some unit and/or system tests for it.

danmar · 2022-12-18T15:22:48Z

I haven't looked into your implementation but I really like this. 👍

mario-campos · 2022-12-20T17:01:09Z

@firewave, any idea how to include certain .cpp files into the Visual Studio build? I don't have any experience with Windows development.

cppcheckexecutor.obj : error LNK2019: unresolved external symbol "public: __cdecl XMLAnalysisReport::XMLAnalysisReport(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)" (??0XMLAnalysisReport@@QEAA@AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Z) referenced in function "protected: bool __cdecl CppCheckExecutor::parseFromArgs(class CppCheck *,int,char const * const * const)" (?parseFromArgs@CppCheckExecutor@@IEAA_NPEAVCppCheck@@HQEBQEBD@Z) [D:\a\cppcheck\cppcheck\test\testrunner.vcxproj]

cli/clianalysisreport.cpp

cli/sarifanalysisreport.cpp

lib/settings.h

Currently, for XML output, the XML generation is tightly-coupled with the `ErrorMessage` class, which makes it harder to implement other formats (e.g. SARIF) that may need to see all of the errors/findings at once. Furthermore, implementing the serialization in the `ErrorMessage` class as individual methods (e.g. `toXML`) does not scale very well when using multiple formats. This approach uses the `AnalysisReport` abstract class to sub-class the different serialization formats.

This treats the CLI output as just another format (default) for the findings/report.

This should look and feel like the rest of the CppCheck code.

This is a good start, but it still needs some work. In particular, I need to fix the startLine/endLine/startColumn/endColumn part. Right now, I am using placeholder values ("1") instead of the real line/column numbers.

This matches the other AnalysisReport classes, and it's what clang-tidy recommends.

SARIF's precision property maps to Cppcheck's certainty, which is essentially the confidence level.

firewave · 2022-12-28T00:29:26Z

Sorry for the late reply but I currently out sick. So still no code review...

@firewave, any idea how to include certain .cpp files into the Visual Studio build? I don't have any experience with Windows development.

cppcheckexecutor.obj : error LNK2019: unresolved external symbol "public: __cdecl XMLAnalysisReport::XMLAnalysisReport(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)" (??0XMLAnalysisReport@@QEAA@AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Z) referenced in function "protected: bool __cdecl CppCheckExecutor::parseFromArgs(class CppCheck *,int,char const * const * const)" (?parseFromArgs@CppCheckExecutor@@IEAA_NPEAVCppCheck@@HQEBQEBD@Z) [D:\a\cppcheck\cppcheck\test\testrunner.vcxproj]

If #4652 is merged (could happen any day), dmake will take care of that. Until then you need to do it manually in the *.vcxproj files.

firewave · 2022-12-28T00:52:00Z

Since GitHub actions apparently support SARIF output could also also adjust the selfcheck workflow to use SARIF output instead of relying on the exitcode?

Also if SARIF is used should the exitcode be forced to 0? I have no idea how other tools implement this.

firewave

Also run dmake to actually update the Makefile.

firewave · 2022-12-28T00:30:52Z

cli/analysisreport.h

+#ifndef ANALYSIS_REPORT_H
+#define ANALYSIS_REPORT_H
+
+#include "errorlogger.h"


No need for the include. Just forward declare ErrorMessage.

Are you sure? I tried forward-declaring ErrorMessage as such:

class ErrorMessage;

But it then failed to build.

yes because you pass ErrorMessage by value to addFinding. It should be passed by const reference.. then I believe a forward declaration will be enough.

cli/cmdlineparser.cpp

firewave · 2022-12-28T00:36:35Z

cli/cppcheckexecutor.h

@@ -19,6 +19,7 @@
 #ifndef CPPCHECKEXECUTOR_H
 #define CPPCHECKEXECUTOR_H

+#include "analysisreport.h"


I always forgot if std::unique_ptr can work with incomplete types...could be forward decl then.

cli/sarifanalysisreport.cpp

firewave · 2022-12-28T00:41:53Z

cli/sarifanalysisreport.cpp

+
+    // https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317507
+    run["results"] = picojson::value(results);
+    runs.emplace_back(run);


std::move()? same for the other emplace_back() calls above.

firewave · 2022-12-28T00:44:15Z

cli/sarifanalysisreport.cpp

+}
+
+static std::map<std::string, picojson::value> text(const std::string& s) {
+    std::map<std::string, picojson::value> m = {{ "text", picojson::value(s) }};


We could enable PICOJSON_USE_RVALUE_REFERENCE (or is it enabled via compile-time detection?) and pass byval and move into picojson::value().

That define is based on preprocessor checks. It should be available in all compiler although I am not 100% sure if the Visual Studio check already covers VS2012.

Okay, I think I've got it, but you should probably double-check my work.

cli/cmdlineparser.cpp

cli/xmlanalysisreport.cpp

cli/cppcheckexecutor.cpp

cli/sarifanalysisreport.cpp

mario-campos · 2023-01-10T21:45:50Z

@firewave, unfortunately dmake does not create the Makefile target with the picojson include -isystem externals/picojson:

cli/sarifanalysisreport.o: cli/sarifanalysisreport.cpp cli/analysisreport.h cli/sarifanalysisreport.h externals/picojson/picojson.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/suppressions.h
	$(CXX) ${INCLUDE_FOR_CLI} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ cli/sarifanalysisreport.cpp

See https://github.com/mario-campos/cppcheck/actions/runs/3887481113/jobs/6633822871. Which means that, without tampering with the Makefile, dmake will generate a Makefile that fails to build cppcheck. Any suggestions?

…lue. We will only pass std::string by value if it can be std::move()d. Otherwise, it will get passed as a constant reference.

danmar · 2023-01-18T16:48:07Z

cli/analysisreport.h

+#ifndef ANALYSIS_REPORT_H
+#define ANALYSIS_REPORT_H
+
+#include "errorlogger.h"


yes because you pass ErrorMessage by value to addFinding. It should be passed by const reference.. then I believe a forward declaration will be enough.

danmar · 2023-01-18T17:04:33Z

cli/clianalysisreport.h

+    std::string serialize() override;
+
+private:
+    bool mVerbose;


I have the feeling these members can be const.

danmar · 2023-01-18T17:05:38Z

cli/cmdlineparser.cpp

@@ -941,8 +941,10 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
            }

            // Write results in results.xml
-            else if (std::strcmp(argv[i], "--xml") == 0)
+            else if (std::strcmp(argv[i], "--xml") == 0) {
+                printMessage("option --xml is deprecated; use --output-format=xml instead.");
                mSettings->xml = true;


I suggest we remove variable xml in the Settings. And create enum outputFormat instead.

danmar · 2023-01-18T17:11:18Z

cli/analysisreport.h

+    /**
+     * Submit a CppCheck result for inclusion into the report.
+     */
+    virtual void addFinding(ErrorMessage msg) = 0;


Change parameter type to const ErrorMessage& msg

I believe it used to be const ErrorMessage&, but @firewave suggested passing by value so it could be std::moved.

hmm.. ok but then I think you can't use the forward declaration.. maybe firewave did not see that. Could you double check if the forward declaration is possible if a const reference is used instead.

danmar · 2023-01-18T17:13:27Z

cli/analysisreport.h

+ * by others classes that will contain the results of a CppCheck analysis, and
+ * output those results in a particular format.
+ */
+class AnalysisReport {


Maybe the constructor for AnalysisReport could take a output stream argument:

std::ostream *outputStream

Then you pass the ofstream/std::cout/std::cerr as argument to the report.

danmar · 2023-01-18T18:59:23Z

I think this looks really interesting. There are some refactorings I would like to see.. but maybe instead of writing lots of nits it's quicker to just take over the finishing touches..

danmar · 2023-01-18T19:16:59Z

unfortunately dmake does not create the Makefile target with the picojson include -isystem externals/picojson:

I have the feeling that you can adjust dmake.cpp here:
https://github.com/danmar/cppcheck/blob/main/tools/dmake.cpp#L73

I guess it should say something like:

    else if (filename.compare(0, 4, "cli/") == 0) {
        getDeps("lib" + filename.substr(filename.find('/')), depfiles);
        for (const std::string & external : externalfolders)
            getDeps(external + filename.substr(filename.find('/')), depfiles);
    }

danmar · 2023-01-21T14:51:40Z

unfortunately dmake does not create the Makefile target with the picojson include -isystem externals/picojson:

I have made some fixes in this branch:

https://github.com/danmar/cppcheck/tree/4651

The fixes I made are here: https://github.com/danmar/cppcheck/commit/acca98d8f33e8f1f4bb5e06c4fb88277970830a2.diff
Feel free to apply those fixes locally. In particular I think the changes in dmake.cpp will help you.

There are still some tests that fail. You can run those tests with these commands:

 cd cppcheck/test/cli
 python3 -m pytest test-helloworld.py

If the tests are fixed I have the feeling that we can merge this PR.

mario-campos · 2023-01-24T15:38:29Z

I think this looks really interesting. There are some refactorings I would like to see.. but maybe instead of writing lots of nits it's quicker to just take over the finishing touches..

That would be helpful. Unfortunately, now that the holiday season is over, I don't have as much time for this PR as I did before. But, I will still continue to address the remaining problems as much as I can.

firewave · 2023-01-30T00:19:54Z

FYI there's already an PR which adds that output format command-line switch: #3365.

github-advanced-security bot found potential problems Dec 17, 2022

View reviewed changes

cli/cppcheckexecutor.cpp Fixed Show resolved Hide resolved

mario-campos force-pushed the mario-campos/sarif branch 2 times, most recently from 2e352ed to 3395a1b Compare December 17, 2022 18:39

mario-campos force-pushed the mario-campos/sarif branch from c75210c to 8c84999 Compare December 19, 2022 19:38

mario-campos marked this pull request as ready for review December 19, 2022 22:10

danmar reviewed Dec 20, 2022

View reviewed changes

cli/clianalysisreport.cpp Outdated Show resolved Hide resolved

cli/sarifanalysisreport.cpp Outdated Show resolved Hide resolved

lib/settings.h Outdated Show resolved Hide resolved

mario-campos force-pushed the mario-campos/sarif branch from 8ce527a to 8fdaa05 Compare December 22, 2022 15:41

mario-campos added 20 commits December 22, 2022 09:41

Refactor out CLI error messages into the new class CLIAnalysisReport.

6f0b5f0

This treats the CLI output as just another format (default) for the findings/report.

Change snake-cased add_finding to camelCased addFinding.

e736e59

This should look and feel like the rest of the CppCheck code.

Add SARIF output format with the new SARIFAnalysisReport class.

963267b

This is a good start, but it still needs some work. In particular, I need to fix the startLine/endLine/startColumn/endColumn part. Right now, I am using placeholder values ("1") instead of the real line/column numbers.

Reformat files to fix warnings.

b495ccd

Include externals/picojson for building cppcheck CLI with cmake.

44ea258

Abstract classes must implement a destructor, even if its virtual.

918d9cf

Add a parameter name to the XMLAnalysisReport method.

f92e1fd

This matches the other AnalysisReport classes, and it's what clang-tidy recommends.

Use std::unique_ptr instead of new/free.

a5298d3

Output severity in SARIF report.

a1f442d

Output precision in SARIF report.

349dc0b

SARIF's precision property maps to Cppcheck's certainty, which is essentially the confidence level.

Output line/column numbers in SARIF report.

c501cdd

Unify spacing and alignment in std::map literal.

ca7214f

Unify spacing around std::map literals.

b81988e

Change static methods to static functions.

df790fd

Fix SARIF report by making runs property an array.

b1b7239

Include Cppcheck homepage URL to SARIF report.

6835c6a

Add SARIFAnalysisReport documentation as comment.

d92b72b

Add missing <memory> include to use std::unique_ptr.

551f7ed

Fix switch/case alignment to please uncrustify.

5f13e12

mario-campos requested review from firewave and danmar and removed request for firewave December 24, 2022 18:06

firewave requested changes Dec 28, 2022

View reviewed changes

danmar reviewed Dec 30, 2022

View reviewed changes

cli/cmdlineparser.cpp Outdated Show resolved Hide resolved

cli/xmlanalysisreport.cpp Show resolved Hide resolved

cli/cppcheckexecutor.cpp Outdated Show resolved Hide resolved

cli/cppcheckexecutor.cpp Show resolved Hide resolved

cli/cppcheckexecutor.cpp Outdated Show resolved Hide resolved

danmar reviewed Dec 30, 2022

View reviewed changes

cli/sarifanalysisreport.cpp Outdated Show resolved Hide resolved

mario-campos added 5 commits January 10, 2023 12:03

std::move more objects in sarifanalysisreport.cpp.

13ce02e

Use range-based for loop for SARIF generation.

03d91ac

Use constant reference.

e14ff71

Improve error message to be more helpful.

74950dc

Add CLIAnalysisReport class for outputting findings to CLI.

3abccfc

mario-campos added 8 commits January 10, 2023 15:47

Indent the member initialization for uncrusty.

1cc3516

Fix the extra new-line character that was added.

21eba3b

Use --output-format instead of --sarif.

b341465

Use move semantics for SARIF generation if PicoJSON supports it.

1b32554

Fix tests by using --output-format=xml instead of --xml.

eacd087

Forgot to include windows.h for Windows OSes.

9995bb1

Fix selfcheck and clang-tidy warnings about passing std::string by va…

e53984e

…lue. We will only pass std::string by value if it can be std::move()d. Otherwise, it will get passed as a constant reference.

Need to link the *analysisreport.o object files for testrunner.

3b2fda2

danmar reviewed Jan 18, 2023

View reviewed changes

mario-campos closed this May 18, 2023

firewave mentioned this pull request Oct 5, 2024

Fix #9972 (Add support for SARIF output format) #6863

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SARIF output support. #4651

Add SARIF output support. #4651

mario-campos commented Dec 16, 2022 •

edited

Loading

firewave commented Dec 17, 2022

danmar commented Dec 18, 2022

mario-campos commented Dec 20, 2022

firewave commented Dec 28, 2022

firewave commented Dec 28, 2022

firewave left a comment

firewave Dec 28, 2022

mario-campos Jan 11, 2023

danmar Jan 18, 2023

firewave Dec 28, 2022

firewave Dec 28, 2022

mario-campos Jan 10, 2023

firewave Dec 28, 2022

firewave Jan 10, 2023

mario-campos Jan 11, 2023

mario-campos commented Jan 10, 2023 •

edited

Loading

danmar Jan 18, 2023

danmar Jan 18, 2023

danmar Jan 18, 2023

danmar Jan 18, 2023

mario-campos Jan 18, 2023

danmar Jan 18, 2023

danmar Jan 18, 2023

danmar commented Jan 18, 2023

danmar commented Jan 18, 2023

danmar commented Jan 21, 2023 •

edited

Loading

mario-campos commented Jan 24, 2023

firewave commented Jan 30, 2023

Add SARIF output support. #4651

Add SARIF output support. #4651

Conversation

mario-campos commented Dec 16, 2022 • edited Loading

firewave commented Dec 17, 2022

danmar commented Dec 18, 2022

mario-campos commented Dec 20, 2022

firewave commented Dec 28, 2022

firewave commented Dec 28, 2022

firewave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mario-campos commented Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmar commented Jan 18, 2023

danmar commented Jan 18, 2023

danmar commented Jan 21, 2023 • edited Loading

mario-campos commented Jan 24, 2023

firewave commented Jan 30, 2023

mario-campos commented Dec 16, 2022 •

edited

Loading

mario-campos commented Jan 10, 2023 •

edited

Loading

danmar commented Jan 21, 2023 •

edited

Loading