Fix #1571: FxCop converter doesn't emit run.logicalLocations + add validation rule #1570

ghost · 2019-07-11T18:35:58Z

@harleenkohli discovered an FxCop SARIF file where theResult.locations[0].logicalLocations[0].index was populated, but theRun.logicalLocations was absent. This is invalid by §3.33.3:

If thisObject is an element of theRun.logicalLocations, then index MAY be present. ...

Otherwise, if theRun.logicalLocations is absent, or if it does not contain a cached object for thisObject, then index SHALL NOT be present.

The reason is that the FxCop converter accumulates the list of logical locations but then never writes them out. It's a one-line fix.

Since this is an in-the-wild occurrence of invalid SARIF that we did not detect, I added a validation rule. The rule is complicated because there are many places we need to do similar array index validations.

Notes:

The validation rule needs to know what run it's looking at, so I added SarifValidationContext.CurrentRun.

Also:

If there is neither physical nor logical location available information, don't emit result.locations. This happens, for example, on an engine exception (CA0001) or an assembly load error (CA0055).
To aid debugging, implement Region.ToString (returns, for example, "(24,10)").
Update SarifValidationSkimmerBase.HelpUri to always point to the latest spec.
Prevent the validator from emitting empty rule help messages.

…st version.

…ndex

ghost · 2019-07-11T18:38:57Z

@michaelcfanning is added to the review. #Closed

ghost · 2019-07-11T18:39:06Z

@harleenkohli is added to the review. #Closed

michaelcfanning · 2019-07-15T17:36:11Z

src/Sarif/RuleUtilities.cs

@@ -32,7 +33,7 @@ public static Result BuildResult(FailureLevel level, IAnalysisContext context, R
            return BuildResult(level, kind, context, region, ruleMessageId, arguments);
        }

-        public static Result BuildResult(FailureLevel level, ResultKind kind, IAnalysisContext context, Region region, string ruleMessageId, params string[] arguments)
+        public static Result BuildResult(FailureLevel level, ResultKind kind, IAnalysisContext context, Region region, string ruleMessageId, params object[] arguments)


object [](start = 148, length = 6)

Not sure I like this change. it's preferable to require callers to convert to strings, so that they can apply culture insensitive/sensitive transformations during ToString. we can't make an appropriate choice from the object.ToString() helper. #Resolved

michaelcfanning · 2019-07-15T17:39:07Z

src/Sarif.Multitool/Rules/RuleId.cs

@@ -16,5 +16,6 @@ public static class RuleId
        public const string UriBaseIdRequiresRelativeUri = "SARIF1014";
        public const string UriMustBeAbsolute = "SARIF1015";
        public const string ContextRegionRequiresRegion = "SARIF1016";
+        public const string InvalidLogicalLocationIndex = "SARIF1017";


InvalidLogicalLocationIndex [](start = 28, length = 27)

As always, I'm concerned this rule is overly granular, create one rule for any invalid index. #Resolved

Working on it. There are a total of 15 array-index-valued properties, so it will take a couple of hours:

logicalLocation.index

logicalLocation.parentIndex

artifactLocation.index

artifact.parentIndex

result.ruleIndex

address.index

address.parentIndex

threadFlowLocation.index

graphTraversal.resultGraphIndex

graphTraversal.runGraphIndex

webRequest.index

webResponse.index

resultProvenance.invocationIndex

reportingDescriptorReference.index

toolComponentReference.index

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

…ndex

…ndex.

michaelcfanning · 2019-07-19T16:08:19Z

src/Sarif.Converters/FxCopConverter.cs

@@ -138,6 +140,11 @@ internal Result CreateResult(FxCopLogReader.Context context)
                sourceFile = string.IsNullOrWhiteSpace(sourceFile) ? targetFile : sourceFile;
            }

+            // Don't emit a location if neither physical location nor logical location information
+            // is present. This is the case for CA0001, (unexpected error in analysis tool).


, [](start = 54, length = 1)

remove this comma? #Resolved

michaelcfanning · 2019-07-19T16:17:05Z

src/Sarif.Multitool/Rules/SARIF1017.InvalidIndex.cs

+                $"runs[{Context.CurrentRunIndex}].results[{Context.CurrentResultIndex}].graphTraversals");
+        }
+
+        protected override void Analyze(LogicalLocation logicalLocation, string logicalLocationPointer)


LogicalLocation [](start = 40, length = 15)

where's the check for logicalLocation.parentIndex? #Resolved

I have not finished all the cases yet. And some of the cases have sub-cases: for instance, you have to validate ReportingDescriptorReference.Index, but ReportingDescriptorReferences occur in several different contexts -- and you need tests for all of them to ensure you've got error message arguments right in each case.

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

Having said that, I just looked at my checklist (I pasted it into another comment), and I see that I put an x in the logicalLocation.parentIndex box -- and that was a mistake. Good catch! I'll uncheck it and implement it (and then re-check it ;-)).

In reply to: 305525258 [](ancestors = 305525258,305428389)

michaelcfanning · 2019-07-19T16:19:43Z

src/Sarif.Multitool/Rules/SARIF1017.InvalidIndex.cs

+
+        protected override void Analyze(GraphTraversal graphTraversal, string graphTraversalPointer)
+        {
+            ValidateArrayIndex(


ValidateArrayIndex [](start = 12, length = 18)

please double check you've covered all 15 identified cases. Am I correct that I find only 13 here? logicalLocations.parentIndex certainly seems missing, can't find the other, assuming there is another. #Resolved

Absolutely. I have a list of cases to cover. Here's the current status (which I think includes work done after you did your review):

[ ] One rule for invalid index [x] logicalLocation.parentIndex [x] artifactLocation.index [x] artifact.parentIndex [x] result.ruleIndex [x] address.index [x] address.parentIndex [x] threadFlowLocation.index [x] graphTraversal.resultGraphIndex [x] graphTraversal.runGraphIndex [x] webRequest.index [x] result.webRequest [x] threadFlowLocation.webRequest [x] webResponse.index [x] result.webResponse [x] threadFlowLocation.webResponse [x] resultProvenance.invocationIndex [ ] reportingDescriptorReference.index [x] result.rule [ ] Cover the case where there is a toolComponent reference that affects the array this points into. [x] configurationOverride.descriptor [x] invocation.ruleConfigurationOverrides [x] invocation.notificationConfigurationOverrides [ ] reportingDescriptorRelationship.target [ ] notification.descriptor [ ] notification.associatedRule [ ] Use in Taxa [ ] Use in ReportingDescriptorRelationship [ ] toolComponentReference.index [ ] toolComponent.associatedComponent [ ] reportingDescriptorReference.toolComponent

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

Great. Resetting my review to 'reviewing' pending your completion of the work. Carry on! Good stuff.

In reply to: 305525771 [](ancestors = 305525771,305429284)

michaelcfanning · 2019-07-19T16:21:36Z

src/Sarif.Multitool/Rules/SarifValidationSkimmerBase.cs

+        protected virtual void Analyze(GraphTraversal graphTraversal, string graphTraversalPointer)
+        {
+        }
+
        protected virtual void Analyze(Invocation invocation, string invocationPointer)


Analyze [](start = 31, length = 7)

If we were ninja, there would be a test that reflects over this type and parses the schema and attempts to ensure that we find an Analyze method for all types. Looks like graphTraversal was missing for example. #WontFix

Format is complete now, so not sure it's worth the time to stop. If we touch this file again, I'll do it.:)

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

michaelcfanning · 2019-07-19T16:24:01Z

src/Sarif.Multitool/Rules/SarifValidationSkimmerBase.cs

@@ -186,7 +214,7 @@ internal static string JsonPointerToJavaScript(string pointerString)
            var pointer = new JsonPointer(pointerString);
            foreach (string token in pointer.ReferenceTokens)
            {
-                if (int.TryParse(token, out int index))
+                if (int.TryParse(token, out _))


_ [](start = 44, length = 1)

NIT can you use 'unusedIndex' or something rather than an underscore? i find it jarring to see this where I expect a variable name. I understand you're trying to indicate it is not consumed. #ByDesign

Actually this is a language feature. "_" is special -- note how I didn't have to specify the type of the output parameter.

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

aah, beautiful!! TIL. I always found it annoying thing about TryParses! most of the times we dont care about the out param, so this makes me very happy! :)

In reply to: 305525416 [](ancestors = 305525416,305430798)

michaelcfanning · 2019-07-19T16:26:09Z

src/Sarif.Multitool/Rules/SarifValidationSkimmerBase.cs

+            }
+        }
+
+        private void Visit(ReportingDescriptorReference reportingDescriptorReference, string reportingDescriptorReferencePointer)


Visit [](start = 21, length = 5)

this code never should have reauthored a comprehensive visitor in the SDK but should have used our rewriting visitor. all this churn emphasizes the point to me. #WontFix

I don't remember how we got to this state. I think I discovered at some point that the generated RewritingVisitor wasn't fit for this purpose, so I started work in a branch (which I still have hanging around called) users/lgolding/generate-visitor, where I was going to generate this visitor. But I never got time to finish it, and it turns out there are aspects of this code that are hard to auto-generate. I'm not opposed to taking another look, but boy, I can't imagine this rising to the top of my stack.

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

There are places in this code where I have to set context to establish whether I'm looking at a notification descriptor, or a taxon descriptor, in order to produce the correct error message. (See, for example, the method Visit(Notification, string) in this class, and in SARIF1017.InvalidIndex.cs, see the overload of Analyze that takes a ReportingDescriptorReference.)

That would make it hard to auto-generate this class.

In reply to: 305526382 [](ancestors = 305526382,305431532)

michaelcfanning

revoking review

…ndex

harleenkohli · 2019-08-09T18:39:20Z

src/ReleaseHistory.md

+## **v2.1.9** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.1.9) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.1.9) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.1.9) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.1.9)
+* BUGFIX: FxCop converter produced logicalLocation.index but did not produce the run.logicalLocations array. https://github.com/microsoft/sarif-sdk/issues/1571
+* FEATURE: Add validation rule to ensure that `logicalLocation.index` is not present unless `run.logicalLocations` is present.


logicalLocation.index is not present unless run.logicalLocations [](start = 46, length = 68)

this text can be changed to cover all index based validations #Resolved

Thanks, fixed.

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

harleenkohli

…ndex

Larry Golding added 6 commits July 10, 2019 10:54

Define rule id and add tests; no rule implementation yet.

a04aa00

Implement rule.

e60baa9

To aid debugging, implement Region.ToString.

e6b19a0

Update SarifValidationSkimmerBase.HelpUri to always point to the late…

d2af119

…st version.

Prevent validator from emitting empty rule help messages.

d5b0257

Merge branch 'master' into users/lgolding/validate-logical-location-i…

dd97a7a

…ndex

ghost requested review from michaelcfanning and harleenkohli July 11, 2019 18:35

Larry Golding added 3 commits July 11, 2019 11:42

Bump version number to 2.1.9.

64120ea

Shorten rule name to avoid "path name to long" test breaks.

c90577d

Fix #1571: FxCop converter doesn't emit logical locations.

c3c7586

ghost changed the title ~~Add validation rule for invalid logicalLocation.index~~ Fix #1571: FxCop converter doesn't emit logical locations + add validation rule Jul 11, 2019

ghost changed the title ~~Fix #1571: FxCop converter doesn't emit logical locations + add validation rule~~ Fix #1571: FxCop converter doesn't emit run.logicalLocations + add validation rule Jul 12, 2019

Don't emit empty locations.

6f59135

This was referenced Jul 12, 2019

FxCop SarifSdkTest is disabled, and fails when enabled. #1575

Closed

Provide a results caching mechanism in cases where hashing is enabled. #1574

Merged

michaelcfanning reviewed Jul 15, 2019

View reviewed changes

Larry Golding added 10 commits July 15, 2019 13:11

Merge branch 'master' into users/lgolding/validate-logical-location-i…

d7fa53d

…ndex

PR feedback: Don't use object[] for message args.

240efb8

PR feedback: One rule for all indices: renames and message fixes.

83a3103

Factor out index validation logic.

7da454e

Validate artifactLocation.index.

b26133c

DRY out validation logic.

046d33f

Validate artifact.parentIndex.

a6cc5a4

Validate result.ruleIndex.

d0cc2c4

Validate address.index.

4a9c09d

Validate address.parentIndex.

a848fd7

Larry Golding added 7 commits July 17, 2019 15:31

Improve jpointer to offending array.

408468e

Validate web{request,response}.index.

99aa70c

Validate web{Request,Response}.index in threadFlowLocation.

dbb552f

Validate resultProvenance.invocationIndex.

4b5773a

Validate invocation.ruleConfigurationOverrides[].descriptor.index

1949f57

Validate invocation.notificationConfigurationOverrides[].descriptor.i…

8ea8d6f

…ndex.

Add files omitted from previous commit

06e6504

michaelcfanning reviewed Jul 19, 2019

View reviewed changes

michaelcfanning previously approved these changes Jul 19, 2019

View reviewed changes

Larry Golding added 10 commits July 20, 2019 10:32

Merge branch 'master' into users/lgolding/validate-logical-location-i…

1d51085

…ndex

Validate logicalLocation.parentIndex.

b7b16a6

Validate notification.{descriptor,associatedRule}.index.

3894245

Capitalize "SARIF" in test output.

88b9c22

Validate reportingDescriptorRelationship.target.index.

a8098f7

Validate result.taxa[i].index.

4976b3f

Validate threadFlowLocation.taxa[].index.

d2b94c4

Validate reportingDescriptorReference.index for extensions.

373d0cc

Validate toolComponentReference.index.

d21f3df

Validate toolComponent.associatedComponent.index.

2261d1a

harleenkohli reviewed Aug 9, 2019

View reviewed changes

harleenkohli approved these changes Aug 9, 2019

View reviewed changes

Merge branch 'master' into users/lgolding/validate-logical-location-i…

39489e0

…ndex

ghost merged commit 4ac33df into master Aug 9, 2019

ghost deleted the users/lgolding/validate-logical-location-index branch August 9, 2019 21:15

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1571: FxCop converter doesn't emit run.logicalLocations + add validation rule #1570

Fix #1571: FxCop converter doesn't emit run.logicalLocations + add validation rule #1570

ghost commented Jul 11, 2019 •

edited by ghost

Loading

ghost commented Jul 11, 2019

ghost commented Jul 11, 2019

michaelcfanning Jul 15, 2019 •

edited by ghost

Loading

michaelcfanning Jul 15, 2019 •

edited by ghost

Loading

ghost Jul 15, 2019

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

ghost Jul 19, 2019

ghost Jul 19, 2019

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

ghost Jul 19, 2019

michaelcfanning Jul 19, 2019

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

michaelcfanning Jul 19, 2019

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

ghost Jul 19, 2019

harleenkohli Aug 9, 2019

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

ghost Jul 19, 2019

ghost Jul 21, 2019

michaelcfanning left a comment

harleenkohli Aug 9, 2019 •

edited by ghost

Loading

ghost Aug 9, 2019

harleenkohli left a comment

Fix #1571: FxCop converter doesn't emit run.logicalLocations + add validation rule #1570

Fix #1571: FxCop converter doesn't emit run.logicalLocations + add validation rule #1570

Conversation

ghost commented Jul 11, 2019 • edited by ghost Loading

ghost commented Jul 11, 2019

ghost commented Jul 11, 2019

michaelcfanning Jul 15, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

michaelcfanning Jul 15, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 19, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

michaelcfanning Jul 19, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 19, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 19, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 19, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 19, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

harleenkohli Aug 9, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harleenkohli left a comment

Choose a reason for hiding this comment

ghost commented Jul 11, 2019 •

edited by ghost

Loading

michaelcfanning Jul 15, 2019 •

edited by ghost

Loading

michaelcfanning Jul 15, 2019 •

edited by ghost

Loading

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

michaelcfanning Jul 19, 2019 •

edited by ghost

Loading

harleenkohli Aug 9, 2019 •

edited by ghost

Loading