Fix #1530: CS converter doesn't produce threadFlowLocation.location. #1531

ghost · 2019-06-12T17:05:23Z

When the CS converter translates a sequence of propagation-event elements into a threadFlow, it populates each threadFlowLocation.stack from the propagation-event's stack sub-element. The converter also takes the propagation-event's signature sub-element and places it at the top of the stack.

But the converter does not populate threadFlowLocation.location, which confuses the VS Code Viewer, which expects location to be present. Sure, we could "fix" the viewer, but really, signature does specify the location, and we should populate it.

While looking at the code I found a couple more issues:

The WebGoat.xml.sarif test file included run.language = "en-US", which is wrong because it's the default. I removed it. I have a fix in another branch for the test infrastructure bug that failed to detect this.
"Fully qualified logical names" were not being handled consistently. Now, they never include the return type of the method, and the never include the argument signature. As a result, there are hundreds of line changes in WebGoat.xml.sarif test file. The important changes, for purposes of fixing the bug, is that every threadFlowLocation object now has location property in addition to the stack property.

ghost · 2019-06-12T17:09:26Z

@michaelcfanning is added to the review. #Closed

ghost · 2019-06-12T17:09:34Z

@harleenkohli is added to the review. #Closed

ghost · 2019-06-12T17:09:44Z

@rscrivens is added to the review. #Closed

ghost · 2019-06-12T17:10:42Z

src/Sarif.Converters/ContrastSecurityConverter.cs

@@ -1121,14 +1121,6 @@ private PhysicalLocation CreatePhysicalLocation(string uri, Region region = null
            };
        }

-        private static readonly Regex LogicalLocationRegex =


LogicalLocationRegex [](start = 38, length = 20)

Moved down, right before the new method RemoveReturnTypeFrom. #ByDesign

ghost · 2019-06-12T17:11:32Z

src/Sarif.Converters/ContrastSecurityConverter.cs

@@ -1138,14 +1130,9 @@ private static string GetUserCodeLocation(Stack stack)
            foreach (StackFrame frame in stack.Frames)
            {
                string fullyQualifiedLogicalName = frame.Location.LogicalLocation.FullyQualifiedName;
-                Match match = LogicalLocationRegex.Match(fullyQualifiedLogicalName);


fullyQualifiedLogicalName [](start = 57, length = 25)

There's no need to strip off the return type here, in this special case, because now we do it on every stack frame when we create it (see below). #ByDesign

ghost · 2019-06-12T17:12:28Z

src/Sarif.Converters/ContrastSecurityConverter.cs

@@ -1617,6 +1625,8 @@ private static void ReadStack(SparseReader reader, object parent)
            Debug.Assert(context.CurrentThreadFlowLocation == null);
            context.CurrentThreadFlowLocation = new ThreadFlowLocation
            {
+                Location = context.Signature.Location,


Location [](start = 16, length = 8)

After all the fuss and bother, this one-liner is the fix for #1530. The rest was just making sure the fully qualified logical name was in the right form. #Resolved

i don't like what we've done to FQN, we've elided the information that qualifies one method overload over the other. this isn't heplful and will compromise result matching.

In reply to: 293025496 [](ancestors = 293025496)

Same as the other two comments -- I will restore the signature to the FQN.

In reply to: 293604799 [](ancestors = 293604799,293025496)

ghost · 2019-06-12T17:13:36Z

                  "location": {

The result of the fix: every threadFlowLocation object now has a location property. Rusty, note that you only get a logical location. The viewer needs to handle that case. #ByDesign

Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:134 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = False)

ghost · 2019-06-12T17:14:28Z

          "System.Web.HttpRequest.get_Form",

The first of hundreds of places where we stripped off the return type and the method signature from the fully qualified logical name, leaving just the namespace-qualified method name. #Resolved

Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:113 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = False)

michaelcfanning · 2019-06-13T22:43:45Z

                              "fullyQualifiedName": "System.Web.Hosting.PipelineRuntime.ProcessRequestNotification()"

why is this a good change? Removing the parens suggests we aren't providing params data, are we not? #Resolved

Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:3049 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = True)

michaelcfanning · 2019-06-13T22:45:07Z

                        "fullyQualifiedName": "System.String.Concat"

this isn't a fully qualified name, there are many string.concat overloads, so which one is this>? you need to populate this before doing the stack frame stripping, i think #Resolved

Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:12246 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = False)

michaelcfanning

🕐

ghost · 2019-06-13T23:04:15Z

                        "fullyQualifiedName": "System.String.Concat"

It was the same change that stripped off the return type. I'll restore the signature (including empty parens when there are no arguments).

In reply to: 501905176 [](ancestors = 501905176)

Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:12246 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = False)

ghost · 2019-06-13T23:04:55Z

                              "fullyQualifiedName": "System.Web.Hosting.PipelineRuntime.ProcessRequestNotification()"

It's not. I got overaggressive when I stripped off the return type. Will fix.

In reply to: 501904892 [](ancestors = 501904892)

Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:3049 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = True)

ghost · 2019-06-13T23:16:55Z

          "System.Web.HttpRequest.get_Form",

Per feedback, we now strip just the return type, and leave the argument signature.

In reply to: 501370995 [](ancestors = 501370995)

Refers to: src/Test.FunctionalTests.Sarif/v2/ConverterTestData/ContrastSecurity/WebGoat.xml.sarif:113 in 6a4b5d6. [](commit_id = 6a4b5d6, deletion_comment = False)

michaelcfanning

Fix #1530: CS converter doesn't produce threadFlowLocation.location.

6a4b5d6

ghost requested review from michaelcfanning and harleenkohli June 12, 2019 17:05

ghost commented Jun 12, 2019

View reviewed changes

michaelcfanning requested changes Jun 13, 2019

View reviewed changes

Address PR feedback from MF: Don't strip signature.

b2a18a6

michaelcfanning approved these changes Jun 13, 2019

View reviewed changes

ghost merged commit 7f9a558 into master Jun 14, 2019

ghost deleted the users/lgolding/bug-1530 branch June 14, 2019 00:08

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1530: CS converter doesn't produce threadFlowLocation.location. #1531

Fix #1530: CS converter doesn't produce threadFlowLocation.location. #1531

ghost commented Jun 12, 2019 •

edited by ghost

Loading

ghost commented Jun 12, 2019

ghost commented Jun 12, 2019

ghost commented Jun 12, 2019

ghost Jun 12, 2019 •

edited by harleenkohli

Loading

ghost Jun 12, 2019 •

edited by ghost

Loading

ghost Jun 12, 2019 •

edited by ghost

Loading

michaelcfanning Jun 13, 2019

ghost Jun 13, 2019

ghost commented Jun 12, 2019 •

edited by ghost

Loading

ghost commented Jun 12, 2019 •

edited by ghost

Loading

michaelcfanning commented Jun 13, 2019 •

edited by ghost

Loading

michaelcfanning commented Jun 13, 2019 •

edited by ghost

Loading

michaelcfanning left a comment

ghost commented Jun 13, 2019

ghost commented Jun 13, 2019

ghost commented Jun 13, 2019

michaelcfanning left a comment

Fix #1530: CS converter doesn't produce threadFlowLocation.location. #1531

Fix #1530: CS converter doesn't produce threadFlowLocation.location. #1531

Conversation

ghost commented Jun 12, 2019 • edited by ghost Loading

ghost commented Jun 12, 2019

ghost commented Jun 12, 2019

ghost commented Jun 12, 2019

ghost Jun 12, 2019 • edited by harleenkohli Loading

Choose a reason for hiding this comment

ghost Jun 12, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jun 12, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

michaelcfanning Jun 13, 2019

Choose a reason for hiding this comment

ghost Jun 13, 2019

Choose a reason for hiding this comment

ghost commented Jun 12, 2019 • edited by ghost Loading

ghost commented Jun 12, 2019 • edited by ghost Loading

michaelcfanning commented Jun 13, 2019 • edited by ghost Loading

michaelcfanning commented Jun 13, 2019 • edited by ghost Loading

michaelcfanning left a comment

Choose a reason for hiding this comment

ghost commented Jun 13, 2019

ghost commented Jun 13, 2019

ghost commented Jun 13, 2019

michaelcfanning left a comment

Choose a reason for hiding this comment

ghost commented Jun 12, 2019 •

edited by ghost

Loading

ghost Jun 12, 2019 •

edited by harleenkohli

Loading

ghost Jun 12, 2019 •

edited by ghost

Loading

ghost Jun 12, 2019 •

edited by ghost

Loading

ghost commented Jun 12, 2019 •

edited by ghost

Loading

ghost commented Jun 12, 2019 •

edited by ghost

Loading

michaelcfanning commented Jun 13, 2019 •

edited by ghost

Loading

michaelcfanning commented Jun 13, 2019 •

edited by ghost

Loading