Add valgrind parser

[bitrunner]

This is the beginning of a valgrind report parser. It is a work-in-progress based on the recent addition of a valgrind parser to violations-lib so it depends upon #735 and will fail CI until that is merged because I didn't want to conflict the pom.xml changes for that here. The eventual goal of this pull request is to provide valgrind support in warnings-ng as a replacement for the broken, vulnerable, and unmaintained jenkins valgrind-plugin.

There are a few known problems/open questions with this that I could use some advice/help with to move this forward. I'm willing to work this through whatever needs to be done to achieve the goal of providing a warnings-ng alternative to the valgrind-plugin.

1. Valgrind analyzes running programs for memory related problems so it provides a lot of important details about findings in the form of stack traces. I tried to capture these details in the description field of issues as HTML tables similar to how they were reported by the existing valgrind-plugin. I haven't been able to wire this up successfully through warnings-ng to see what this looks like on the warnings-ng Jenkins UI, so this might be a terrible strategy or otherwise need to be revised to fit existing design paradigms.
2. It's difficult to get a parser to identify the right file where the issue root cause is because there are often multiple files to choose from in the stack trace and simple rules would not always identify the correct one. Is there an established pattern for dealing with this kind of ambiguity? Right now, the violations-lib valgrind parser uses the file that is closest to the top of the stack that isn't valgrind's replacement for malloc. That behavior is echoed in analysis-model in this initial pull request. That might not be ideal.
3. There's some text in the suppression part of the description HTML that contains a special character, <, that needs to be escaped. A simple String.replace("<","&lt;") would suffice, but is there a more robust way to escape such strings that may contain special HTML characters? Will this escaping be handled in warnings-ng or further downstream of analysis-model for the description field?
4. Both the origin and category fields are set to "valgrind:[tool]" where [tool] is the actual valgrind tool that detected the problem (ex. memcheck). It seems important to identify these problems as coming from valgrind and its subtool, but I couldn't find a reference elsewhere in the code for similar handling so this may need some refinement.
5. IntelliJ reports static analysis findings that seem in conflict with javac. For example, IntelliJ wants "final" keywords on method parameters but javac complains that is unnecessary and discouraged since Java 8. Is there a preference for what to make happy for these types of things? I've been going with the compiler, but it seems like IntelliJ cleanliness might be preferred.
6. There is some embedded JSON in the Violation structure that can generate some exceptions when parsed in the convertToReport function of the ValgrindAdapter in analysis-model. I resorted to catching and ignoring them because an alternative was unclear to me. Is there a better way to deal with that?
7. Is it best to work on analysis-model in isolation or should it be worked in conjunction with whatever needs to be done to warnings-ng to support this new valgrind tool? I've found it difficult to align all the moving parts to get this to work end-to-end coming at this from a place of complete ignorance. Is there some guidance on adding new tools somewhere?

• [x] Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• [x] Ensure that the pull request title represents the desired changelog entry
• [x] Please describe what you did
• [x] Link to relevant issues in GitHub or Jira
• [x] Link to relevant pull requests, esp. upstream and downstream changes
• [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue

[uhafner]

Thanks for providing this PR! I merged the violations lib PR #735 so you can update your PR with the new pom.xml version.

[uhafner]

1. Valgrind analyzes running programs for memory related problems so it provides a lot of important details about findings in the form of stack traces. I tried to capture these details in the description field of issues as HTML tables similar to how they were reported by the existing valgrind-plugin. I haven't been able to wire this up successfully through warnings-ng to see what this looks like on the warnings-ng Jenkins UI, so this might be a terrible strategy or otherwise need to be revised to fit existing design paradigms.

It is hard to decide without an example. Do you have an HTML snippet that shows such a description?

2. It's difficult to get a parser to identify the right file where the issue root cause is because there are often multiple files to choose from in the stack trace and simple rules would not always identify the correct one. Is there an established pattern for dealing with this kind of ambiguity? Right now, the violations-lib valgrind parser uses the file that is closest to the top of the stack that isn't valgrind's replacement for malloc. That behavior is echoed in analysis-model in this initial pull request. That might not be ideal.

I think this is a good starting point. There is currently no strategy, each parser author decides on his own how to map issues to files.

3. There's some text in the suppression part of the description HTML that contains a special character, <, that needs to be escaped. A simple String.replace("<","<") would suffice, but is there a more robust way to escape such strings that may contain special HTML characters? Will this escaping be handled in warnings-ng or further downstream of analysis-model for the description field?

Currently, the description field is used by a few parsers only: these parsers return an HTML string. I think I need to improve the documentation for this field. Currently no escaping is done, so if you have non-HTML content you need to escape on your own by using StringEscapeUtils.escapeHtml4 (https://issues.jenkins.io/browse/JENKINS-62036). I'm not sure if I can or should add something in the analysis-model. Maybe can you elaborate your problem with an example?

The HTML string is later processed in the warnings plugin. The whole description is piped through an HTML processor (https://github.com/jenkinsci/antisamy-markup-formatter-plugin) to ensure that the created HTML contains no evil tags.

4. Both the origin and category fields are set to "valgrind:[tool]" where [tool] is the actual valgrind tool that detected the problem (ex. memcheck). It seems important to identify these problems as coming from valgrind and its subtool, but I couldn't find a reference elsewhere in the code for similar handling so this may need some refinement.

Do not use the origin, this field should be set from outside. I should make that clear in the JavaDoc as well.

5. IntelliJ reports static analysis findings that seem in conflict with javac. For example, IntelliJ wants "final" keywords on method parameters but javac complains that is unnecessary and discouraged since Java 8.

This was a bug in my configuration. final still should be used. I fixed the ErrorProne analysis configuration.

Is there a preference for what to make happy for these types of things? I've been going with the compiler, but it seems like IntelliJ cleanliness might be preferred.

Actually, the PR checks should be all Green 😄

6. There is some embedded JSON in the Violation structure that can generate some exceptions when parsed in the convertToReport function of the ValgrindAdapter in analysis-model. I resorted to catching and ignoring them because an alternative was unclear to me. Is there a better way to deal with that?

If the exceptions indicate a problem in the source code of the parser, then they should be thrown so we can fix the parser (fail fast). If the exception may occur more often due to a problem in the tool that writes the reports then we should catch and ignore them. I'll look at the code and comment there.

7. Is it best to work on analysis-model in isolation or should it be worked in conjunction with whatever needs to be done to warnings-ng to support this new valgrind tool? I've found it difficult to align all the moving parts to get this to work end-to-end coming at this from a place of complete ignorance. Is there some guidance on adding new tools somewhere?

Normally, it is not required anymore to make any changes in the warnings plugin. A new parser should work out of the box. In order to test your changes you need to wrap your changes into a new https://github.com/jenkinsci/analysis-model-api-plugin Jenkins plugin and deploy it to your Jenkins. When you are using my devenv then the deploy script does that automatically for you. You can then use the analysisModelId (https://github.com/jenkinsci/warnings-ng-plugin/blob/master/doc/Documentation.md#pipeline-step-with-a-generic-symbol)

[bitrunner]

Thanks for the helpful feedback and advice. I think this is sorted out except for the description format and styling.

Here's an example of what it's currently generating (with extra newlines and formatting for readability):

<table>
  <tr><td class="pane-header">Executable</td><td class="pane">./terrible_program</td></tr>
  <tr><td class="pane-header">Unique Id</td><td class="pane">0x1</td></tr>
  <tr><td class="pane-header">Thread Id</td><td class="pane">1</td></tr>
  <tr><td class="pane-header">Auxiliary</td><td class="pane">Address 0x4dd0c90 is 0 bytes after a block of size 16 alloc'd</td></tr>
</table>
<h2>Primary Stack Trace</h2>
<h3>Invalid write of size 4</h3>
<table>
  <tr><td class="pane-header">Object</td><td class="pane">/home/some_user/terrible_program/terrible_program</td></tr>
  <tr><td class="pane-header">Function</td><td class="pane">main</td></tr>
  <tr><td class="pane-header">File</td><td class="pane">/home/some_user/terrible_program/terrible_program.cpp:10</td></tr>
</table>
<h2>Auxiliary Stack Trace</h2>
<h3>Address 0x4dd0c90 is 0 bytes after a block of size 16 alloc'd</h3>
<table>
  <tr><td class="pane-header">Object</td><td class="pane">/usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so</td></tr>
  <tr><td class="pane-header">Function</td><td class="pane">operator new[](unsigned long)</td></tr>
  <tr><td class="pane-header">File</td><td class="pane">./coregrind/m_replacemalloc/vg_replace_malloc.c:640</td></tr>
</table>
<br>
<table>
  <tr><td class="pane-header">Object</td><td class="pane">/home/some_user/terrible_program/terrible_program</td></tr>
  <tr><td class="pane-header">Function</td><td class="pane">main</td></tr>
  <tr><td class="pane-header">File</td><td class="pane">/home/some_user/terrible_program/terrible_program.cpp:3</td></tr>
</table>
<h2>Suppression</h2>
<table>
  <tr><td class="pane"><pre>{
   &lt;insert_a_suppression_name_here&gt;
   Memcheck:Addr4
   fun:main
}</pre></td></tr>
</table>

Here's what that looks like (without warnings-ng CSS):

Executable	./terrible_program
Unique Id	0x1
Thread Id	1
Auxiliary	Address 0x4dd0c90 is 0 bytes after a block of size 16 alloc'd

Primary Stack Trace

Invalid write of size 4

Object	/home/some_user/terrible_program/terrible_program
Function	main
File	/home/some_user/terrible_program/terrible_program.cpp:10

Auxiliary Stack Trace

Address 0x4dd0c90 is 0 bytes after a block of size 16 alloc'd

Object	/usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so
Function	operator new[](unsigned long)
File	./coregrind/m_replacemalloc/vg_replace_malloc.c:640

Object	/home/some_user/terrible_program/terrible_program
Function	main
File	/home/some_user/terrible_program/terrible_program.cpp:3

Suppression

{ <insert_a_suppression_name_here> Memcheck:Addr4 fun:main }

[codecov[bot]]

Codecov Report

Merging #738 (943e039) into master (565bacf) will increase coverage by 0.02%. Report is 13 commits behind head on master. The diff coverage is 94.50%.

@@             Coverage Diff              @@
##             master     #738      +/-   ##
============================================
+ Coverage     92.92%   92.94%   +0.02%     
- Complexity     2338     2368      +30     
============================================
  Files           345      347       +2     
  Lines          6499     6592      +93     
  Branches        672      686      +14     
============================================
+ Hits           6039     6127      +88     
- Misses          262      263       +1     
- Partials        198      202       +4     

Files Changed	Coverage Δ
...du/hm/hafner/analysis/registry/ParserRegistry.java	100.00% <ø> (ø)
...er/analysis/parser/violations/ValgrindAdapter.java	93.82% <93.82%> (ø)
...m/hafner/analysis/registry/ValgrindDescriptor.java	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[bitrunner]

I finally got this to work end-to-end through warnings-ng after manually updating analysis-model-api's pom.xml to use the analysis-model SNAPSHOT (you might consider noting this probably obvious required step in your guide somewhere to keep idiots like me from staring open mouthed at it not working for an embarrassingly long time) using the analysisModelId trick you pointed me at. I moved the big blob of HTML into the message from the description because I couldn't find the description anywhere on the warnings-ng UI. It looks like this horrible mess:

I obviously have no UI skills, so advice on how to make this look less ridiculous is welcome. strong tags seem to be automatically inserted into the supplied HTML and those aren't helping make it look less terrible. Maybe this whole idea is dumb because it's so far outside the norm for warnings-ng. At this point, I'm not sure. Without all the stack information, it will be impossible for anyone to figure out what needs fixing so it seems like it has to be available to the user somehow if this is going to be useful.

Thoughts?

[uhafner]

Yes, I need to update the documentation and my development environment (https://github.com/uhafner/warnings-ng-plugin-devenv) description. Since I switched to incremental builds the procedure has been changed and now the version is not a SNAPSHOT version anymore. (That helps to use Dependabot creating automatic PRs when a new release appears.).

Currently the description is used by a few parsers only (and there the description is obtained from external files). But normally only the message is wrapped into the <strong> tags, the description should be shown as such (HTML). Note that some tags might get removed by our sanitizer to avoid security problems.

I think I need to install your PR in my local instance and play around with it a little bit. I'm also not a UI designer but during the development of the plugin I improved my skills 😁

Are there other tools that produce nice Valgrind reports where we can get an inspiration?

[uhafner]

I just installed it and started with some experiments. I think it does not make sense to use fonts larger than the content. I activated some styling using the predefined Bootstrap CSS table styling but I am not sure if that is enough. Maybe we need to provide some new CSS classes for such use cases.

[uhafner]

You can use basically anything from https://getbootstrap.com/ if that helps? (Collapsible sections, etc.)

[mchatt]

Hello @bitrunner, I am looking forward using your new valgrind parser. Any idea when this PR can be merged ? Thank you

[uhafner]

Maybe we can use the current styling in improve it later in another PR? I haven't yet looked at the code because there where these open UI design questions but I can add a review this weekend if it makes sense.

[mchatt]

Maybe we can use the current styling in improve it later in another PR? I haven't yet looked at the code because there where these open UI design questions but I can add a review this weekend if it makes sense.

I have almost zero knowledge about UI development. I can't be of much help to you. But as a potential user of this parser, I would say yes, let merge it as it is, and improve the design in a different PR.

[bitrunner]

It seems like it might be time to finally finish this up. I like your bootstrap suggestions, style hints, and think your example screenshot is plenty good enough @uhafner. I tried to add some CSS to make it look nicer, but all the CSS seems to get stripped from the message and description fields so no CSS classes can be applied. Were you able to get some through to make that screenshot or were you just adding the styling in the browser? I can make it look nice by editing stuff in my browser, but I can't seem to get enough wizardry through the plugin to make it look reasonable.

[uhafner]

Good to see that you have some time to finish it. I also have some time to help right now!

Actually I forgot whether I changed the HTML output in the browser or in the code 🙈

You can use HTML in the description but not in the message. If that does not work it might get stripped somewhere by accident.

[bitrunner]

It doesn't seem to work in the description either. Here's the best I've been able to make it look without any CSS. It seems usable like that, but it's not great. Only the first line is the message, everything else is in the description.

I rebased and pushed the latest formatting stuff so what's currently in the pull request should make output like the screenshot.

[uhafner]

Sorry, seems that I forced pushed to your repository :-(

Can you please force push your chagnes again? I wasn't aware that my branch is not set to track my repository. Sorry about that...

[bitrunner]

No problem. Done. Feel free to add whatever you like.

[uhafner]

Actually I just wanted to see the results in my Jenkins instance. From your code it also looks that HTML Is working in the description, doesn't it?

[uhafner]

Currently, your headers are smaller than the text, maybe you need to tweak these a little bit.

[uhafner]

BTW: in Java it might make more sense to use j2html to generate the HTML output (and not a StringBuilder).

Example: https://github.com/jenkinsci/analysis-model/blob/565bacf88d429a00860e44cf4c8345acd97b5c92/src/main/java/edu/hm/hafner/analysis/parser/VeraCodePipelineScannerParser.java#L150

[KalleOlaviNiemitalo]

If you use j2html, be aware that its TagCreator.join method "removes spaces before periods and commas", which may be an undesirable change on text coming from a tool such as valgrind.

[bitrunner]

The HTML tags I've tried seem to go through fine, but the CSS is stripped. For example, trying to add striping to the tables like this results in a bare table tag (the class="..." part is stripped) in the output HTML:

<table class="table table-striped">

As for j2html, if you'd prefer it be done that way, I'll switch to it. I'm too clueless to have a preference. @KalleOlaviNiemitalo's comment causes some concern but I can't think of anything specific potentially losing some spaces would mess up that HTML doesn't already do anyway.

[bitrunner]

Another problem I noticed is that the suppression part of the Violation's specifics is getting lost somehow. It works in the ValgrindAdapter unit test, but Violation.getSpecifics().get("suppression") always returns null when running in Jenkins. Some debugging indicates there does not appear to ever be a suppression key/value pair in the specifics Map. The stack traces are also stored in the specifics Map and they are working fine. Processing the same valgrind output XML in the unit test and on Jenkins shows the suppression part in the unit test but not on Jenkins. The suppression uses CDATA in XML and has \n newlines, curly braces {}, and angle brackets <> in its text so it might have something funky that is making something upset. I don't see anything of interest in the jenkins log. Any idea how this might be possible?

[KalleOlaviNiemitalo]

@bitrunner the j2html behaviour did previously cause JENKINS-64051. I don't remember whether valgrind output can include any application-specified text that j2html could corrupt in a similar fashion.

[bitrunner]

I updated this to use j2html and thanks to @KalleOlaviNiemitalo, avoided problems like JENKINS-64051 by borrowing @uhafner's strategy. In the process, I changed from h5 to h4 to make the headers bigger than the text per @uhafner's note above.

I also managed to trick the very strict static analysis into accepting some methods that return null using a somewhat ridiculous obfuscation. null returns appears to be a common idiom in j2html to handle optional things. If my trickery is not acceptable, I'll refactor yet again to find some other middle ground between j2html and the static analysis rules or just go back to StringBuilder where null returns aren't helpful.

I also put the CSS in to show the problem of it being stripped.

It seems like this might be good enough as is although it has not-so-pretty HTML output and missing suppressions which doesn't seem critical.

Shall we squash and merge or pursue further enhancements?

[uhafner]

The HTML tags I've tried seem to go through fine, but the CSS is stripped. For example, trying to add striping to the tables like this results in a bare table tag (the class="..." part is stripped) in the output HTML:

You are right, the https://github.com/jenkinsci/antisamy-markup-formatter-plugin actually removes potentially unsafe HTML tags and classes. I have no idea why classes are considered a security problem.

[uhafner]

Another problem I noticed is that the suppression part of the Violation's specifics is getting lost somehow. It works in the ValgrindAdapter unit test, but Violation.getSpecifics().get("suppression") always returns null when running in Jenkins. Some debugging indicates there does not appear to ever be a suppression key/value pair in the specifics Map. The stack traces are also stored in the specifics Map and they are working fine. Processing the same valgrind output XML in the unit test and on Jenkins shows the suppression part in the unit test but not on Jenkins. The suppression uses CDATA in XML and has \n newlines, curly braces {}, and angle brackets <> in its text so it might have something funky that is making something upset. I don't see anything of interest in the jenkins log. Any idea how this might be possible?

The only thing what you need to be aware: Jenkins might read those values (you will seem them in the serialized XML reports in Jenkins build folder) but will not render them due to the OWASP markup formatter. All tags will be checked against a whitelist to see whether only safe elements are contained.

[bitrunner]

You are right, the https://github.com/jenkinsci/antisamy-markup-formatter-plugin actually removes potentially unsafe HTML tags and classes. I have no idea why classes are considered a security problem.

Can the stuff that is produced by the plugin be separated from the stuff that comes from the tool report file for the purpose of sanitization? That is, can we allow the plugin to generate HTML that is not subject to the user safety checks and only run the stuff that comes directly from the report files/valgrind tool through the safety check?

This is not possible yet (and I don't know if its worth the effort, currently we simply filter everything). This would also imply that we need to sanitize the HTML already here.

The only thing what you need to be aware: Jenkins might read those values (you will seem them in the serialized XML reports in Jenkins build folder) but will not render them due to the OWASP markup formatter. All tags will be checked against a whitelist to see whether only safe elements are contained.

From what I can tell from debugging, the suppression content from the valgrind.xml report is not being read into the Violation when running in Jenkins but it is read when running the unit test. It might be dropped by the XML reader, I'm not sure. I am sure this is not a problem where the tags are stripped on the output. This information is lost on the input side.

Do you have an example in the unit test? Then I can try to debug locally.

[bitrunner]

Do you have an example in the unit test? Then I can try to debug locally.

Yes. 4 of the 5 test errors in the test valgrind.xml have suppressions. In valgrind.xml, the suppression is stored in CDATA sections within the <rawtext> tags like this one: https://github.com/jenkinsci/analysis-model/blob/817d21c8fdd0a860f6fd3ad0e8f0c92383756c57/src/test/resources/edu/hm/hafner/analysis/parser/violations/valgrind.xml#L78-L86

The insert_a_suppression_name_here part of this line verifies that the suppressions are in the output within the unit test: https://github.com/jenkinsci/analysis-model/blob/817d21c8fdd0a860f6fd3ad0e8f0c92383756c57/src/test/java/edu/hm/hafner/analysis/parser/violations/ValgrindAdapterTest.java#L66

While running in Jenkins, specifics.get("suppression") is always null here: https://github.com/jenkinsci/analysis-model/blob/817d21c8fdd0a860f6fd3ad0e8f0c92383756c57/src/main/java/edu/hm/hafner/analysis/parser/violations/ValgrindAdapter.java#L65

If you like, you can add links to the icon and URL to the descriptor as well:

Sorry, I misunderstood what you wanted me to do. I think I manged to do it right in the latest push.

[uhafner]

Do you have an example in the unit test? Then I can try to debug locally.

Yes. 4 of the 5 test errors in the test valgrind.xml have suppressions. In valgrind.xml, the suppression is stored in CDATA sections within the <rawtext> tags like this one:

https://github.com/jenkinsci/analysis-model/blob/817d21c8fdd0a860f6fd3ad0e8f0c92383756c57/src/test/resources/edu/hm/hafner/analysis/parser/violations/valgrind.xml#L78-L86

The insert_a_suppression_name_here part of this line verifies that the suppressions are in the output within the unit test:

https://github.com/jenkinsci/analysis-model/blob/817d21c8fdd0a860f6fd3ad0e8f0c92383756c57/src/test/java/edu/hm/hafner/analysis/parser/violations/ValgrindAdapterTest.java#L66

While running in Jenkins, specifics.get("suppression") is always null here:

https://github.com/jenkinsci/analysis-model/blob/817d21c8fdd0a860f6fd3ad0e8f0c92383756c57/src/main/java/edu/hm/hafner/analysis/parser/violations/ValgrindAdapter.java#L65

I found the problem. In my unit tests the default XML input stream parser of Xerces XMLStreamReaderImpl is used while during runtime a ValidatingStreamReader of the Jackson library is used. This parsers seems to replace the standard parser in Jenkins. I have no idea if we can (or should) remove that association.

[uhafner]

I think we can postpone the problem with the parser and create a new release right now.