Handle multiple "stack" nodes for errors in Valgrind XML report files

[jhoussay]

Hi,

For some Valgrind errors (such as "InvalidWrite"), the corresponding XML node contains two distinct "stack" nodes, one corresponding to the allocation stack and the other one corresponding to the memory misuse stack.

Unfortunately, sonar cxx only presents the content of one "stack'" node found in the "error" node. In my case, this is pretty annoying as the stack available in Sonar is the allocation stack and does not allow easy identication of the memory misuse.

It seems to come from the parseErrorTag method in ValgrindReportParser.java. When multiple "stack" nodes are contained in the "error" node, only the last one is kept:

String kind = null;
String text = null;
ValgrindStack stack = null;
while (child.getNext() != null) {
   String tagName = child.getLocalName();
    if ("kind".equalsIgnoreCase(tagName)) {
      kind = child.getElemStringValue();
    } else if ("xwhat".equalsIgnoreCase(tagName)) {
      text = child.childElementCursor("text").advance().getElemStringValue();
    } else if ("what".equalsIgnoreCase(tagName)) {
      text = child.getElemStringValue();
    } else if ("stack".equalsIgnoreCase(tagName)) {
      stack = parseStackTag(child);
    }
}

[...]

return new ValgrindError(kind, text, stack);

Handling multliple stacks at this point would require to do so in ValgrindError.java as well and modify the getLastOwnFrame method in order to provide the "best" stack origin...

Julien

[guwirth]

@jhoussay could you

• provide an XML report sample please.
• Is this one message with two locations or two messages? Please have also a look to fix CppcheckParserV2; allow multiple NewIssueLocations #1436. Similar to this?

[jhoussay]

Hi @guwirth ,

Here is an example of report : valgrind_output.xml.txt.

The invalid write "error" node starts line 37. It contains two "stack" nodes, the first one (line 42) corresponding to the invalid write itself (2) and the second one (line 141) corresponding to the memory allocation (1).

1. float *pToto = new float[10];
2. pToto[10] = 10.f

As I said in the first post, only the last stack is kept and, in SonarQube, I have the following behaviour, only pointing to the allocation place, which does not really help to spot the invalid memory access.

So, to my mind, we need one message with the two locations provided in the "error" node.

I had a look to #1436, I would say it corresponds to the same issue but for cppcheck output files. As far as I understand sonar cxx, maybe this pull request could be used for valgrind report parsing as well :

• The ValgrindError.java class could be modified in order to store one or more locations,

• The parseErrorTag method in ValgrindReportParser.java should handle multple "stack" nodes and provide them as multiple locations to the ValgrindError instance returned by the method,

• In CxxValgrindSensor.java, the new saveUniqueLocation provided by the PR could be used.

[guwirth]

@jhoussay maybe we can solve this with #1447

[ivangalkin]

@guwirth @jhoussay #1447 has to address stack frames of Valgrind finding: all frames, which belong to the analyzed project will be referenced as secondary locations.

An other issue I can recognize from the screenshot is the terrible formatting of the stack trace. I guess it should be enough to insert it as a code block. Markdown is supported in comments, not sure about issues. It must be tested.

diff --git a/cxx-sensors/src/main/java/org/sonar/cxx/sensors/valgrind/CxxValgrindSensor.java b/cxx-sensors/src/main/java/org/sonar/cxx/sensors/valgrind/CxxValgrindSensor.java
index 160dcfc..33d5916 100644
--- a/cxx-sensors/src/main/java/org/sonar/cxx/sensors/valgrind/CxxValgrindSensor.java
+++ b/cxx-sensors/src/main/java/org/sonar/cxx/sensors/valgrind/CxxValgrindSensor.java
@@ -73,7 +73,7 @@ public class CxxValgrindSensor extends CxxReportSensor {
       ValgrindFrame frame = error.getLastOwnFrame(context.fileSystem().baseDir().getPath());
       if (frame != null) {
         saveUniqueViolation(context, CxxValgrindRuleRepository.KEY,
-          frame.getPath(), frame.getLine(), error.getKind(), error.toString());
+          frame.getPath(), frame.getLine(), error.getKind(), error.toMarkdown());
       } else {
         LOG.warn("Cannot find a project file to assign the valgrind error '{}' to", error);
       }
diff --git a/cxx-sensors/src/main/java/org/sonar/cxx/sensors/valgrind/ValgrindError.java b/cxx-sensors/src/main/java/org/sonar/cxx/sensors/valgrind/ValgrindError.java
index e38acb9..b9f890f 100644
--- a/cxx-sensors/src/main/java/org/sonar/cxx/sensors/valgrind/ValgrindError.java
+++ b/cxx-sensors/src/main/java/org/sonar/cxx/sensors/valgrind/ValgrindError.java
@@ -49,6 +49,10 @@ class ValgrindError {
     return text + "\n\n" + stack;
   }
 
+  public String toMarkdown() {
+    return text + "\n\n```\n" + stack + "\n```\n";
+  }
+
   @Override
   public boolean equals(Object o) {
     if (this == o) {

[ivangalkin]

FYI: the patch is wrong; neither HTML nor markdown are supported inside of message
For more info please see http://javadocs.sonarsource.org/7.0/apidocs/org/sonar/api/batch/sensor/issue/NewIssueLocation.html#message-java.lang.String-

[jhoussay]

Hi @guwirth, @ivangalkin,

I just relaunched the analysis with #1447.
I saw a little difference between the line number and the statement : a "1" is displayed, but I still was not able to find the real location of my invalid memory access.

I have the feeling that multiple code locations are enabled (as seen at the bottom of the following screenshot) but are not handled in the valgrind report.

PS: @ivangalkin, I clearly agree that a better formatting for valgrind call stacks would be very nice :-)

[ivangalkin]

Hi @jhoussay! Thank you for testing and commenting!
#1447 is based on existing code and altogether it works like that

error = parser.get_error()
last_frame = error.get_last_own_frame()
issue.create_primarly_location(last_frame.full_source_path, last_frame.line_nr, parser.get_stack().to_string());

stack = parser.get_stack()
for frame in stack:
    if frame.full_source_path in analyzed_project:
        msg = frame.nr + frame.address + frame.signature + frame.filename
        issue.add_secondary_location(frame.full_source_path, frame.line_nr, msg)

So there are multiple reasons for wrong visualization:

1. Implement support for multiple locations in CxxReportSensor #1447 implements the visualization of the single frames by means of secondary locations. However SonarQube is able to reference [secondary] locations only if they are part of the analyzed project. The attached XML contains the stack, which is spread over the following paths:

      <dir>/home/jho/dev/Project/code/externals/ThirdParty/cppunit/sources/src/cppunit</dir>
      <dir>/home/jho/dev/Project/code/externals/ThirdParty/include/cppunit</dir>
      <dir>/home/jho/dev/Project/code/modules/MO_SURMOD/src/MO_SURMOD</dir>
      <dir>/home/jho/dev/Project/code/tu/MO_SURMOD_TU/include/MO_SURMOD_TU</dir>

Are all of them part of the SonarQube project? If the test directory (MO_SURMOD_TU?) or (externals are not part of the project, there is no way to display/reference them properly.

@jhoussay Please double-check the properties of your SonarQube analysis.

I think the introduction of dummy secondary locations (if source file was not found in project, see below) would make the visualization more consistent. Such location could be additionally grouped into a "flow" (see NewIssue API)

2. Your XML file contains 3 errors (0, 1, 2), thereby the error nr. 1 has 2 stacks. I double-check the source code and can confirm, that multiple stacks are not supported yet. Implement support for multiple locations in CxxReportSensor #1447 allows the visualization of frames inside of one stack, but it doesn't change the parser/import of the XML itself.

3. SonarQube is not able to format the multi-line message properly. Unfortunately it doesn't support HTML or markdown as a text of an issue. This fact makes a long stacktrace with many details to be displayed as a single line. It's terrible to read.
   There are some workarounds I can think of. E.g.
   a. Adding a dummy secondary location even if source file is not part of a project. This will make a complete stack hierarchy visible, which is a good think in any way. However...

• such location would not reference any meaningful place in the code
• such locations are not displayed by default. User would have to click on them in order to expand the frame one by one.
  b. We could try to create a comment with a valgrind stack. Comments allow multi-line messages and can even contain code blocks.

@guwirth I confirm the issue w.r.t. the insufficient parsing (see nr. 2). If nobody insists on fixing this issue by himself/herself I will provide a patch soon (it is related to my #1447, but should be made as separate pull request).

Regarding the nr. 3 - its not a bug in my opinion, but rather a limitation of SonarQube. I can try to experiment with comments API, but I'm not 100% sure if is technically possible.

Best regards,
Ivan

[jhoussay]

Hi @ivangalkin,

Thanks a lot for your detailed answer !

It made me realize I totally misunderstood #1447 goal : I was considering a "location" as a full stack while it was a frame within the stack.
Actually, this PR is extremely useful to navigate within a stack without losing the error context. But, of course, it does not correspond to my issue with Valgrind !

1. You are totally right : the secondary locations of this stack are not part of the analyzed project (they belong to a unit test program and cppunit), which is fine in this particular example.
2. This is indeed my issue.
3. I think that, thanks to your PR, this limitation is not so bad : visualization isn't great, for sure, but you can navitage through the stack without having to mentally parse one-line stack.

[guwirth]

solved with #1461