Resolve unhandled exceptions when analyzing folders that contain large files #2494

marmegh · 2022-07-01T02:28:34Z

OutOfMemory and NullReferenceException exceptions are being thrown when analyzing folders containing large files. This change adopts a file size limit in kilobytes and defaults to 1024.

Added unit testing to cover combinations of file sizes and file size limits.

This is related to #621.

…ove logic comparing file size to limit to base class, and udpated accordingly.

…leSizeInKilobytes if passed a negative number.

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs

eddynaka · 2022-07-01T15:10:07Z

src/Sarif/IAnalysisContext.cs

+        /// Gets or sets the maximum file size (in kilobytes) that will be analyzed.
+        /// If not set, it will analyze all sizes.
+        /// </summary>
+        int FileSizeInKilobytes { get; set; }


FileSizeInKilobytes

since we are here, can we update this to MaxFileSizeInKilobytes or something that actually explains why is this needed?
#WontFix

This was mentioned in the other PR, it would be a breaking change for SPAM. Is that acceptable?

I think, if that would make things clearer and we tell people, yes.

What are your thoughts?

Another strategy could be the following:

we rename the C# property

we keep the exposed argument.

So, internally, we know the good name. and we tell that in the next release we will rename the argument :)

Please do not take this change now. A breaking change of this kind needs to come with a thoughtful implementation, this is new work that is slowing down the immediate change.

So, go ahead and file an issue. We have other patterns of deprecating command-line args (like --hashes is support to be obsoleted!!). We do this in a manner such as Eddy describes, retain the old and mark it obsolete and/or update the code to properly transform old command-lines to new.

So, Ed, be sure to capture your excellent thoughts in the issue that you file. :)

…ative.

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

eddynaka · 2022-07-01T18:09:58Z

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

+        [Fact]
+        public void MultithreadedAnalyzeCommandBase_TargetFileSizeTestCases()
+        {
+            Random random = new Random();


Random random = new Random();

you should preserve the seed, otherwise, if this test fails, you won't be aware of what was the seed used and won't be able to easily reproduce it. #Resolved

Looking closer, you just need to use the TestRule.s_seed

Please do step through other tests that show the output that we expect for any random use. Eddy, be sure to explain this sort of thing in detail, it's important for others to understand completely (you may have done this already offline). Basically, any test that uses a random value generator MUST emit the seed used to initialize the PRNG in its output. For tests that fail only due to specific randomized values, this seed can be hard-coded on the dev box to ensure reproducibility of the problem.

By the way! I recently fixed a test bug of this kind in an MS service. I was co-developing a change and my build failed, test rerun succeeded. When I looked at history, I could clearly see this was a flaky test, but it only failed once every several hundred pipeline builds. :) The issue was a random value generator that only provoked a negative condition in a very small % of cases. Ouch! That bug had lived in this code base for over a year...

eddynaka · 2022-07-01T18:10:05Z

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

+            int randomMaxFileSize = random.Next(1, int.MaxValue - 1);
+            long randomFileSize = (long)random.Next(2, int.MaxValue - 1);
+
+


remove #Resolved

I would so dearly love to not see this kind of feedback in code reviews, i.e., for all style suggestions to mostly be resolved by an automated code formatter. Is there not an autofix for this in a Roslyn analyzer or command-line tool? It's important that we stay clean and detail-oriented, so I appreciate all of us pointing this sort of thing out, to be clear. Just encouraging us to remove as many issues of this kind from ever occurring, wherever we can.

We usually have a step that removes these extra lines as part of buildAndTest.cmd for most of our repos.

eddynaka · 2022-07-01T18:30:57Z

src/Sarif.Driver/Sdk/PluginDriverCommand.cs

+        {
+            long fileSize = FileSystem.GetFileSize(path) / 1024;
+
+            return (maxFileSize == -1 || fileSize < maxFileSize);


fileSize < maxFileSize

I think a few cases are missing:
maxfile = -1 vs -1/0/1/big
maxfile = file = 10 (a reasonable number) #Resolved

Updated test cases and added some explicit follow up confirming this method separately.

eddynaka · 2022-07-01T18:32:05Z

src/Sarif.Multitool.Library/SarifValidationContext.cs

@@ -96,6 +96,13 @@ public Uri TargetUri

        public DefaultTraces Traces { get; set; }

+


remove empty line #Resolved

michaelcfanning · 2022-07-01T20:05:24Z

src/Sarif.Driver/Sdk/PluginDriverCommand.cs

@@ -122,6 +129,11 @@ internal bool ValidateOutputFileCanBeCreated(IAnalysisContext context, string ou
            return succeeded;
        }

+        internal bool ValidateFileSizeInKilobytes(int fileSizeInKilobytes)


ValidateFileSizeInKilobytes

This factoring looks questionable to be. Your method name is as long as its implementation. :)

In general, a utility method only has value if it contains some specialized logic that you don't want to maintain in multiple places. A simple 'greater than 0' comparison doesn't seem to meet that test.

There is another subtle issue 'ValidateFileSizeInKilobytes' is suggestive, it suggests more validation is done. You encode 'kilobytes' in the name and 'file size' and yet the implementation of this helper seems to be 'VerifyIsGreaterThanZero', and that's it.

Having said all this, I don't even see a caller for this method. :) So maybe I'm making a whole lot of pointing out you left some intermediate code in the file. :)

Final point, though, just to keep beating this to death, this helper doesn't access instance data and, if it were to survive, could be static.

#Resolved

michaelcfanning · 2022-07-01T20:10:01Z

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

+                new {
+                    expectedExitReason = ExitReason.None,
+                    fileSize = (long)ulong.MinValue,
+                    maxFileSize = randomMaxFileSize


randomMaxFileSize

Don't do this. :) there really isn't a good use of a random value in this test pattern, in my opinion. Why is that? Because the test matrix of use values is so clear. You need all boundaries (max & min), you need zero, -1, and then a couple values within the interior range.

Using random to generate something just won't buy you much, and meantime you've opted into a more complex test pattern which, for example, leads others to require you to follow specific pattern. Don't do that unless you really think you derive value from it.

#Resolved

michaelcfanning · 2022-07-01T20:14:31Z

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

+
+                var options = new TestAnalyzeOptions
+                {
+                    Threads = 10,


Threads = 10

Why are you opting into this behavior, and enabling hashes, etc? This is a nice test, but it's doing a lot, why is that? Can't we make this a bit more focused on the matter at hand, file size validation?

When you start adding things, you increase performance costs, but you also risk introducing non-obvious breaks in tests. So, let's say we had a problem introduced by our hashing argment to --insert. This test may fail, and we'd all be confused why the file size test broken.

This is just an example. But the principle here is that we want targeted unit tests that directly focus on some test work, as far as possible

It isn't always possible to create a clean union. But for this example, I definitely don't see why we'd explicitly set any args, would prefer defaults and minimal options otherwise. #Resolved

Hashes, etc were part of the original customer escalation. It's fairly conclusive that with and without these the repro and fix are the same. I've removed.

michaelcfanning · 2022-07-06T21:14:18Z

src/ReleaseHistory.md

@@ -1,5 +1,9 @@
 # SARIF Package Release History (SDK, Driver, Converters, and Multitool)

+## Unreleased
+
+* BUGFIX: Resolve OutofMemoryException and NullReferenceException' failures by adding `--file-size-in-kb` argument to specify a max file size to analyze or default to 1024KB. [#2494](https://github.com/microsoft/sarif-sdk/pull/2494)


OutofMemoryException

enquote this and the other exception kind.

michaelcfanning · 2022-07-06T21:18:42Z

src/Sarif/FileSystem.cs

+        /// <returns>
+        /// A long representing the size of the file in bytes.
+        /// </returns>
+        public long GetFileSize(string path)


GetFileSize

Let's call this FileInfoLength. #Pending

michaelcfanning · 2022-07-06T21:25:51Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

-                                                                               filter,
-                                                                               SearchOption.TopDirectoryOnly))
+                    foreach (string file in FileSystem.DirectoryEnumerateFiles(directory, filter, SearchOption.TopDirectoryOnly)
+                            .Where(file => IsTargetWithinFileSizeLimit(file, _rootContext.FileSizeInKilobytes)))


Where

Please put your predicate/handling within the collection, the rule is to avoid chained collection traversal. #Pending

michaelcfanning · 2022-07-06T21:29:36Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

-                                                                               filter,
-                                                                               SearchOption.TopDirectoryOnly))
+                    foreach (string file in FileSystem.DirectoryEnumerateFiles(directory, filter, SearchOption.TopDirectoryOnly)
+                            .Where(file => IsTargetWithinFileSizeLimit(file, _rootContext.FileSizeInKilobytes)))


Where

put this filter predicate into the code block. #Pending

michaelcfanning · 2022-07-06T21:31:24Z

src/Sarif.Multitool.Library/SarifValidationContext.cs

+        /// Gets or sets the maximum file size (in kilobytes) that will be analyzed.
+        /// If not set, it will analyze all sizes.
+        /// </summary>
+        public int FileSizeInKilobytes { get; set; } = -1;


-1

remove this #Pending

michaelcfanning · 2022-07-06T21:36:24Z

src/Sarif/IAnalysisContext.cs

+        /// Gets or sets the maximum file size (in kilobytes) that will be analyzed.
+        /// If not set, it will analyze all sizes.
+        /// </summary>
+        int FileSizeInKilobytes { get; set; }


FileSizeInKilobytes

Change this to MaxFileInKilobytes. Do not accept this change into SPAM without making sure SPAM supports its old argument as well for now.

You can look at how we handled deprecating --hashes in favor of --insert Hashes in the BinSkim code base for an example. #Pending

michaelcfanning · 2022-07-06T21:38:38Z

src/Sarif.Multitool/AnalyzeTestContext.cs

@@ -30,6 +30,8 @@ public class AnalyzeTestContext : IAnalysisContext

        public DefaultTraces Traces { get; set; }

+        public int FileSizeInKilobytes { get; set; } = -1;


-1;

remove this. only the attribute should set the default value. #Pending

michaelcfanning · 2022-07-06T21:39:49Z

src/Sarif.Multitool.Library/SarifValidationContext.cs

+        /// Gets or sets the maximum file size (in kilobytes) that will be analyzed.
+        /// If not set, it will analyze all sizes.
+        /// </summary>
+        public int FileSizeInKilobytes { get; set; } = -1;


-1

Remove this. Did you author a test for the validation command that this is honored? #Pending

michaelcfanning · 2022-07-06T21:40:15Z

src/Test.UnitTests.Sarif.Driver/Sdk/PluginDriverCommandTests.cs

@@ -9,6 +9,7 @@

 using Microsoft.CodeAnalysis.Sarif;
 using Microsoft.CodeAnalysis.Sarif.Driver;
+using Microsoft.VisualBasic;


Microsoft

why is this is here? #Pending

michaelcfanning · 2022-07-06T21:40:45Z

src/Test.Utilities.Sarif/TestAnalysisContext.cs

+        /// Gets or sets the maximum file size (in kilobytes) that will be analyzed.
+        /// If not set, it will analyze all sizes.
+        /// </summary>
+        public int FileSizeInKilobytes { get; set; } = -1;


-1

Remove this. #Pending

michaelcfanning · 2022-07-06T21:44:57Z

public class AnalyzeTestContext : IAnalysisContext

Why is this in here? See if you can delete it. I don't understand why a test file is in the production code, looks like it might be mistake.

In reply to: 1176781298

Refers to: src/Sarif.Multitool/AnalyzeTestContext.cs:9 in eb91a3a. [](commit_id = eb91a3a, deletion_comment = False)

michaelcfanning

michaelcfanning · 2022-07-06T21:45:55Z

src/ReleaseHistory.md

@@ -1,5 +1,9 @@
 # SARIF Package Release History (SDK, Driver, Converters, and Multitool)

+## Unreleased
+
+* BUGFIX: Resolve OutofMemoryException and NullReferenceException' failures by adding `--file-size-in-kb` argument to specify a max file size to analyze or default to 1024KB. [#2494](https://github.com/microsoft/sarif-sdk/pull/2494)


BUGFIX

FEATURE: Add file-size-in-kb argument that filters allows filtering scan targets by file size. #Pending

marmegh · 2022-07-06T23:47:14Z

public class AnalyzeTestContext : IAnalysisContext

Eddy provided the PR for historical context. Not part of this change, but this and related files should eventually be moved to test project.

In reply to: 1176781298

Refers to: src/Sarif.Multitool/AnalyzeTestContext.cs:9 in eb91a3a. [](commit_id = eb91a3a, deletion_comment = False)

eddynaka · 2022-07-07T16:02:51Z

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

+                    MaxFileSizeInKilobytes = testCase.maxFileSize
+                };
+
+                var command = new TestMultithreadedAnalyzeCommand(mockFileSystem.Object);


TestMultithreadedAnalyzeCommand

You are only testing the multithreaded command base. If you take a look, we have some tests that has a boolean and that decides if it is single threaded or multithreaded since input/output is exactly the same besides the actual command.
#Resolved

…ingle threaded code paths from test.

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

+            if (exitReason != ExitReason.None)
+            {
+                var exception = command.ExecutionException as ExitApplicationException<ExitReason>;
+                exception.Should().NotBeNull();


src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

+                bool expectedToBeWithinLimits = testCase.maxFileSize == -1 ||
+                    testCase.fileSize / 1024 < testCase.maxFileSize;


eddynaka

marmegh added 6 commits June 27, 2022 09:47

file size fix

f2a06e7

Update CreateTargetsSet to exclude too large files.

882676b

Fix build errors.

91feaf9

Move logic to retrieve file size from FileInfo to FileSystem class, m…

c9317ba

…ove logic comparing file size to limit to base class, and udpated accordingly.

Update unit tests with mock FileSystem

36408d8

Testcases covering file limits edge cases and reset to default max Fi…

cc17ae0

…leSizeInKilobytes if passed a negative number.

marmegh requested a review from michaelcfanning as a code owner July 1, 2022 02:28

github-advanced-security bot found potential problems Jul 1, 2022

View reviewed changes

eddynaka reviewed Jul 1, 2022

View reviewed changes

Throw and exit for InvalidCommandLineOption if file size limit is neg…

ad44f1d

…ative.

github-advanced-security bot found potential problems Jul 1, 2022

View reviewed changes

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs Fixed Show resolved Hide resolved

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs Fixed Show fixed Hide fixed

CodeQL changes

b02d14e

eddynaka reviewed Jul 1, 2022

View reviewed changes

michaelcfanning reviewed Jul 1, 2022

View reviewed changes

PR feedback

d6b1c02

marmegh requested a review from michaelcfanning July 6, 2022 05:53

Updated a comment.

eb91a3a

michaelcfanning reviewed Jul 6, 2022

View reviewed changes

michaelcfanning approved these changes Jul 6, 2022

View reviewed changes

michaelcfanning reviewed Jul 6, 2022

View reviewed changes

marmegh added 2 commits July 6, 2022 16:52

Addressing PR feedback.

29dd946

Fixed typo

18946a5

eddynaka reviewed Jul 7, 2022

View reviewed changes

Correct typo and use existing helper methods to test both multi and s…

5b13312

…ingle threaded code paths from test.

github-advanced-security bot found potential problems Jul 8, 2022

View reviewed changes

marmegh added 2 commits July 8, 2022 10:37

Merge branch 'main' into spamICM

4217bfd

Fix flaky test.

e2a213e

eddynaka approved these changes Jul 8, 2022

View reviewed changes

marmegh merged commit ce8c5cb into main Jul 8, 2022

marmegh deleted the spamICM branch July 8, 2022 20:13

This was referenced Jul 15, 2022

Update to Newtonsoft 13.0.1 #2504

Merged

Creating release v.2.4.16 #2507

Merged

marmegh mentioned this pull request Aug 4, 2022

Newtonsoft and Layout Changes microsoft/sarif-website#142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve unhandled exceptions when analyzing folders that contain large files #2494

Resolve unhandled exceptions when analyzing folders that contain large files #2494

marmegh commented Jul 1, 2022

eddynaka Jul 1, 2022 •

edited by marmegh

Loading

marmegh Jul 1, 2022

eddynaka Jul 1, 2022

eddynaka Jul 1, 2022

michaelcfanning Jul 1, 2022

eddynaka Jul 1, 2022 •

edited by marmegh

Loading

eddynaka Jul 1, 2022

michaelcfanning Jul 1, 2022

eddynaka Jul 1, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 1, 2022

marmegh Jul 1, 2022

eddynaka Jul 1, 2022 •

edited by marmegh

Loading

marmegh Jul 1, 2022

eddynaka Jul 1, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 1, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 1, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 1, 2022 •

edited by marmegh

Loading

marmegh Jul 1, 2022

michaelcfanning Jul 6, 2022

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning commented Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning left a comment

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

marmegh commented Jul 6, 2022

eddynaka Jul 7, 2022 •

edited by marmegh

Loading

eddynaka left a comment

		int randomMaxFileSize = random.Next(1, int.MaxValue - 1);
		long randomFileSize = (long)random.Next(2, int.MaxValue - 1);

		@@ -96,6 +96,13 @@ public Uri TargetUri

		public DefaultTraces Traces { get; set; }

		@@ -30,6 +30,8 @@ public class AnalyzeTestContext : IAnalysisContext

		public DefaultTraces Traces { get; set; }

		public int FileSizeInKilobytes { get; set; } = -1;

		bool expectedToBeWithinLimits = testCase.maxFileSize == -1 \|\|
		testCase.fileSize / 1024 < testCase.maxFileSize;

Resolve unhandled exceptions when analyzing folders that contain large files #2494

Resolve unhandled exceptions when analyzing folders that contain large files #2494

Conversation

marmegh commented Jul 1, 2022

eddynaka Jul 1, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jul 1, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jul 1, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jul 1, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jul 1, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning Jul 1, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning Jul 1, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning Jul 1, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 6, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning Jul 6, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning Jul 6, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning Jul 6, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning Jul 6, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning Jul 6, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning Jul 6, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning Jul 6, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning Jul 6, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

michaelcfanning commented Jul 6, 2022 • edited by marmegh Loading

michaelcfanning left a comment

Choose a reason for hiding this comment

michaelcfanning Jul 6, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

marmegh commented Jul 6, 2022

eddynaka Jul 7, 2022 • edited by marmegh Loading

Choose a reason for hiding this comment

eddynaka left a comment

Choose a reason for hiding this comment

eddynaka Jul 1, 2022 •

edited by marmegh

Loading

eddynaka Jul 1, 2022 •

edited by marmegh

Loading

eddynaka Jul 1, 2022 •

edited by marmegh

Loading

eddynaka Jul 1, 2022 •

edited by marmegh

Loading

eddynaka Jul 1, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 1, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 1, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 1, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning commented Jul 6, 2022 •

edited by marmegh

Loading

michaelcfanning Jul 6, 2022 •

edited by marmegh

Loading

eddynaka Jul 7, 2022 •

edited by marmegh

Loading