Missing context regions for single-line #2616

michaelcfanning · 2023-02-13T01:48:52Z

…ameter.

…s <512 characters.

src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs

+                foreach (string po in new string[] { null, Environment.NewLine })
+                {
+                    string[] tests =
+                    {
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding,   3)}",
+                        $"{new string(padding,   3)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                        $"{new string(padding,   3)}{pr}{sentinel}{po}{new string(padding,   3)}",
+                        $"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding, ssl)}",
+                        $"{new string(padding,   1)}{pr}{sentinel}{po}{new string(padding, ssl)}",
+                        $"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding,   1)}",
+                        $"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding, ssl)}",
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                        $"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding, bsl)}",
+                        $"{new string(padding,  10)}{pr}{sentinel}{po}{new string(padding, bsl)}",
+                        $"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding,  10)}",
+                        $"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding, bsl)}",
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding,   3)}",
+                        $"{new string(padding,   3)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                        $"{new string(padding,   3)}{pr}{sentinel}{po}{new string(padding,   3)}",
+                        $"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding, ssl)}",
+                        $"{new string(padding,   1)}{pr}{sentinel}{po}{new string(padding, ssl)}",
+                        $"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding,   1)}",
+                        $"{new string(padding, ssl)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding, ssl)}",
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                        $"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding, bsl)}",
+                        $"{new string(padding,  10)}{pr}{sentinel}{po}{new string(padding, bsl)}",
+                        $"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding,  10)}",
+                        $"{new string(padding, bsl)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding, bsl)}",
+                        $"{new string(padding,   0)}{pr}{sentinel}{po}{new string(padding,   0)}",
+                    };
+
+                    var context = new StringBuilder();
+                    int iteration = 0;
+
+                    // DEBUGGING THESE TESTS: these tests do not accumulate all outputs and report 
+                    // them, instead they break on the first failure. A failure will report a 
+                    // message like so:
+                    //
+                    // Expected contextRegion.Snippet not to be <null> because 'baz' snippet exists
+                    // (iteration 12, while processing char-based region type for value 'abaza'). 
+                    //
+                    // Observe the iteration value (in this case 12). Set a conditional breakpoint
+                    // below when the iteration variable equals this value and you can debug the
+                    // relevant failure.
+
+                    foreach (string test in tests)
+                    {
+                        var cache = new FileRegionsCache();
+                        var uri = new Uri(@"c:\temp\DoesNotExist\" + Guid.NewGuid().ToString() + ".cpp");
+
+                        int index = test.IndexOf(sentinel);
+
+                        // The FileRegions code takes two discrete code paths depending on whether
+                        // the input variable is char-offset based or uses the start line convention.
+                        var regions = new Region[]
+                        {
+                    new Region
+                    {
+                        CharOffset = index,
+                        CharLength = sentinel.Length,
+                    },
+                    new Region
+                    {
+                        StartLine = 1,
+                        StartColumn = index + 1,
+                        EndColumn = index + sentinel.Length + 1,
+                    }
+                        };
+
+                        string charBased = "char-based";
+                        string lineBased = "line-based";
+
+                        foreach (Region region in regions)
+                        {
+                            context.Clear();
+                            context.Append($"(iteration {iteration}, while processing {(region.StartLine == 1 ? lineBased : charBased)} region type for value '{test}')");
+
+                            // First, we populate the region and text snippet for the actual test finding.
+                            Region actual = cache.PopulateTextRegionProperties(region, uri, populateSnippet: true, test);
+
+                            actual.Snippet.Should().NotBeNull($"'{sentinel}' snippet exists {context}");
+                            actual.Snippet.Text?.Should().Be($"{sentinel}", $"region snippet did not match {context}");
+
+                            // Now, we attempt to produce a context region.
+                            Region contextRegion = cache.ConstructMultilineContextSnippet(actual, uri, test);
+                            contextRegion.Snippet.Should().NotBeNull($"'{sentinel}' snippet exists {context}");
+                            contextRegion.Snippet.Text.Contains(sentinel).Should().BeTrue($"context region should encapsulate finding {context}");
+                        }
+
+                        iteration++;
+                    }
+                }


src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs

michaelcfanning · 2023-02-13T17:25:12Z

src/Sarif/FileRegionsCache.cs

@@ -142,6 +142,9 @@ private Region PopulateTextRegionProperties(NewLineIndex lineIndex, Region input
            return region;
        }

+        internal const int BIGSNIPPETLENGTH = 512;


BIGSNIPPETLENGTH

Note that I've lifted out these magic values to make them more explicit in test use, and to allow product changes to flow more naturally to test validation.

do we prefer put at the top of the file with other definitions

Yes, will do this in a follow-on change, thanks for the review!!

michaelcfanning · 2023-02-13T17:26:18Z

src/Sarif/FileRegionsCache.cs


            // Generating full inputRegion to prevent issues.
-            Region originalRegion = this.PopulateTextRegionProperties(inputRegion, uri, populateSnippet: true);
+            Region originalRegion = this.PopulateTextRegionProperties(inputRegion, uri, populateSnippet: true, fileText);


fileText

We were not consistent around passing this optional argument around. This likely would break certain in-memory analysis operations that aren't highly exercised/well-tested today.

michaelcfanning · 2023-02-13T17:27:38Z

src/Sarif/FileRegionsCache.cs

-            region.CharOffset = originalRegion.CharOffset < smallSnippetLength
-                ? 0
-                : originalRegion.CharOffset - smallSnippetLength;
+            region.CharOffset = Math.Max(0, originalRegion.CharOffset - SMALLSNIPPETLENGTH);


CharOffset

Any prepended context region must start at the beginning of the file, any value that precedes this is invalid.

michaelcfanning · 2023-02-13T17:28:25Z

src/Sarif/FileRegionsCache.cs

-            region.CharLength = originalRegion.CharLength + region.CharOffset + smallSnippetLength < newLineIndex.Text.Length
-                ? originalRegion.CharLength + smallSnippetLength + Math.Abs(region.CharOffset - originalRegion.CharOffset)
-                : newLineIndex.Text.Length - region.CharOffset;
+            region.CharLength =


CharLength

Similarly, any context region that blows past the end of the actual file is invalid.

michaelcfanning · 2023-02-13T19:04:07Z

    public void FileRegionsCache_IncreasingToLeftAndRight()

This test really should have been built out more. It tests one idiosyncratic case in our idiosyncratic system.

If the case when have a context region that itself exceeds the large snippet size (of 512 chars), our solution will add up to 128 chars of additional text to it, starting with the prepended side. This test shows that only the overflow from the unused 128 chars is appended to the region.

All of this behavior should first be reexamined and then tested in directed tests. This change is a hammer to ensure that all cases, in fact, produce context regions. We should ship this and then later fix this:

#2617

Refers to: src/Test.UnitTests.Sarif/FileRegionsCacheTests.cs:707 in 520eb13. [](commit_id = 520eb13, deletion_comment = False)

shaopeng-gh

michaelcfanning added 12 commits January 31, 2023 15:47

Add rolling has optional data enum value. Remove SarifLogger tool par…

7bd9cb8

…ameter.

Remove unnecessary tool argument from all log construction.

9346d42

Formatting updates.

e596d39

Fine-tune extensions serialization.

0edd2d4

Don't serialize empty guid values.

d4efb0d

Add hash computation helpers.

fe3d138

Update release notes.

bae5a37

Fix compute hash function callback logic.

4369a20

Formatting changes.

fdb2545

Fix condition where context regions are required for single-line file…

f470849

…s <512 characters.

Merge remote-tracking branch 'origin' into missing-context-regions

0926536

All tests pass.

09a417c

github-advanced-security bot found potential problems Feb 13, 2023

View reviewed changes

michaelcfanning marked this pull request as ready for review February 13, 2023 17:17

michaelcfanning requested review from EasyRhinoMSFT, marmegh and cfaucon as code owners February 13, 2023 17:17

Update test comment.

520eb13

michaelcfanning commented Feb 13, 2023

View reviewed changes

michaelcfanning added 3 commits February 13, 2023 09:30

CodeQL advice taken.

c70851b

Fix CodeQL alert.

44f8e58

Restore test and correct comment.

2d48491

shaopeng-gh approved these changes Feb 13, 2023

View reviewed changes

michaelcfanning merged commit 254ab10 into main Feb 14, 2023

michaelcfanning deleted the missing-context-regions branch February 14, 2023 00:54

shaopeng-gh mentioned this pull request Feb 21, 2023

Releasing v3.2.0 #2622

Closed

shaopeng-gh mentioned this pull request Feb 21, 2023

Releasing 4.0.0 #2623

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Missing context regions for single-line #2616

Missing context regions for single-line #2616

michaelcfanning commented Feb 13, 2023 •

edited

Loading

michaelcfanning Feb 13, 2023

shaopeng-gh Feb 13, 2023

michaelcfanning Feb 14, 2023

michaelcfanning Feb 13, 2023

michaelcfanning Feb 13, 2023

michaelcfanning Feb 13, 2023

michaelcfanning commented Feb 13, 2023

shaopeng-gh left a comment

Missing context regions for single-line #2616

Missing context regions for single-line #2616

Conversation

michaelcfanning commented Feb 13, 2023 • edited Loading

michaelcfanning Feb 13, 2023

Choose a reason for hiding this comment

shaopeng-gh Feb 13, 2023

Choose a reason for hiding this comment

michaelcfanning Feb 14, 2023

Choose a reason for hiding this comment

michaelcfanning Feb 13, 2023

Choose a reason for hiding this comment

michaelcfanning Feb 13, 2023

Choose a reason for hiding this comment

michaelcfanning Feb 13, 2023

Choose a reason for hiding this comment

michaelcfanning commented Feb 13, 2023

shaopeng-gh left a comment

Choose a reason for hiding this comment

michaelcfanning commented Feb 13, 2023 •

edited

Loading