Enqueue files filter #2599

michaelcfanning · 2022-12-28T20:09:30Z

This is a significant change that touches on some core driver concepts, worth a careful review.

Updates max file size in kb type from int to long.
Marks AnalyzeCommandBase as obsolete, we should delete it soon.
We now emit a warning anytime a file is skipped due to exceeding size threshold.
Updated the file size option to allow setting it to any value 0 or great. The rationale here is that setting the threshold to 0 would allow a tool to collect a log file that documents what files would have been scanned but were not.
Found and fixed at least one bug accounting for the hangs observed in Linux analysis (due to exceptions enumerating files).
Provide an overridable mechanism for updating file filtering logic (being file size). This will allow tools like BinSkim to examine and discard files before the hashing phase (because they aren't Windows PEs or other scan targets). This will resolve the horrible perf slowdowns observed in pipelines that naively apply wildcard specifiers at analysis time.
I've converted the max file size in kb argument to an actual context property. This means it can be configured using exported XML. We should convert all command-line arguments in this way. i.e., you should be able to pass an XML file only to drive analysis.

@shaopeng-gh @LingZhou-gh @EasyRhin0 @suvamM @yongyan-gh

@rtaket @cfaucon, heads up: this SDK will emit skipped file warnings, as requested.

src/Sarif/AnalyzerContextBase.cs

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

+            catch (Exception e)
+            {
+                Errors.LogUnhandledEngineException(_rootContext, e);
+                ThrowExitApplicationException(_rootContext, ExitReason.UnhandledExceptionInEngine);
+            }


michaelcfanning · 2022-12-28T21:56:47Z

src/Sarif/Errors.cs

@@ -46,8 +46,10 @@ public static void LogExceptionLoadingTarget(IAnalysisContext context)
                throw new ArgumentNullException(nameof(context));
            }

+            context.RuntimeErrors |= RuntimeConditions.ExceptionLoadingTargetFile;


context

We now set RuntimeErrors early in every method, so that this data is persisted in the event that an unhandled exception occurs in the remainder of the method (as happened during my current dev/test loop).

Curious what exception you got, guess its thrown by logger? Should we apply the same change to Warnings?

michaelcfanning · 2022-12-28T22:00:43Z

src/Sarif/Warnings.cs

+            }
+
+            // '{0}' was skipped as its size ({1} kilobytes) exceeds the currently configured threshold ({2} kilobytes).
+            context.Logger.LogConfigurationNotification(


context

Produces a console warning at runtime like so. Note that we include the full path name of the skipped file in the output. I produced this message by configuring the max file in kb argument via a config file, demonstrating that this is functional using actual client tool.

BINSKIM : warning WRN997.FileSkippedDueToSize : 'D:\repros\NetperfServiceReleaseFolder\VMAgents\Linux\NetPerfCanaryAppOrchestrator_artifact\NetPerfOrchestrator\resources\scripts\SecondaryNic.sh' was not analyzed as its size (4 kilobytes) exceeds the currently configured threshold (1 kilobytes).

michaelcfanning · 2022-12-28T22:03:43Z

src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs

@@ -12,6 +12,7 @@

 namespace Microsoft.CodeAnalysis.Sarif.Driver
 {
+    [Obsolete("AnalyzeCommandBase will be deprecated entirely soon. Use MultithreadedAnalyzeCommandBase instead.")]


AnalyzeCommandBase

It grows wearisome to maintain this obsolete single-threaded analysis, let's zap it next release.

michaelcfanning · 2022-12-28T22:04:34Z

src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs

@@ -94,15 +95,16 @@
                        ExecutionException = ex;
                        return FAILURE;
                    }
                    catch (Exception ex)


LogUnhandledEngineException

This helper previously returned an error condition to OR into RuntimeErrors. I've updated the helper to be consistent with other utility methods. We now retrieve the RuntimeErrors in the finally block.

michaelcfanning · 2022-12-28T22:14:53Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

@@ -214,7 +213,7 @@

            // 1: First we initiate an asynchronous operation to locate disk files for
            // analysis, as specified in analysis configuration (file names, wildcards).
-            Task<bool> enumerateFilesOnDisk = EnumerateFilesOnDiskAsync(options, rootContext);
+            Task<bool> enumerateFilesOnDisk = EnumerateFilesOnDiskAsync(options);


EnumerateFilesOnDiskAsync

This method already access instance data, so we'll just utilize _rootContext.

michaelcfanning · 2022-12-28T22:16:28Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

@@ -363,107 +367,129 @@
            }


ShouldEnqueue

This diff is all screwed up here (in codeflow). This is the core new feature, we provide an overridable mechanism for filtering files at file enumeration time. This will allow binskim to check to see whether a file can be scanned. Currently, we let all files in and then they all get hashed (and then discarded), resulting in egregious slowdowns.

michaelcfanning · 2022-12-28T22:17:18Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

-
-            readyToHashChannel.Writer.Complete();
+            catch (Exception e)
+            {


OK, so here's the source of some of the hangs that have been reported, if file enumeration raised an unhandled exception, we never closed the write channel. Oops.

michaelcfanning · 2022-12-28T22:18:11Z

src/Test.FunctionalTests.Sarif/Multitool/BaselineOptionTests.cs

@@ -115,7 +115,6 @@ protected override string ConstructTestOutputFromInputResource(string inputResou
                Quiet = true,
                PrettyPrint = true,
                Optimize = true,
-                Kind = new List<ResultKind> { ResultKind.Fail },


Kind

This is now a default in the options ctor.

michaelcfanning · 2022-12-28T22:19:52Z

src/Test.Utilities.Sarif/TestAnalyzeOptions.cs

@@ -9,12 +9,6 @@ namespace Microsoft.CodeAnalysis.Sarif
 {
    public class TestAnalyzeOptions : AnalyzeOptionsBase
    {
-        public TestAnalyzeOptions()


TestAnalyzeOptions

set now in the base ctor.

src/Sarif.Driver/Sdk/AnalyzeOptionsBase.cs

+        {
+            // TODO: these defaults need to be converted to the configuration
+            // property pattern as followed by MaxFileSizeInKilobytes.
+            Traces = new string[] { };


…arif-sdk into enqueue-files-filter

yongyan-gh · 2022-12-28T23:18:19Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

@@ -89,13 +89,14 @@
                        if (!(ex is ExitApplicationException<ExitReason>))
                        {
                            // These exceptions escaped our net and must be logged here
-                            RuntimeErrors |= Errors.LogUnhandledEngineException(_rootContext, ex);
+                            Errors.LogUnhandledEngineException(_rootContext, ex);


Errors.LogUnhandledEngineException

so this call may throw exception, should we add a 'catch' block in the first 'try' block to catch it?

…ze only.

shaopeng-gh · 2022-12-28T23:59:21Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

+                            if (!IsTargetWithinFileSizeLimit(file, _rootContext.MaxFileSizeInKilobytes, out long fileSizeInKb))
+                            { 
+                                _ignoredFilesCount++;
+                            }


question,
we use the ShouldEnqueue() to see if to add to sortedFiles list.
we use the IsTargetWithinFileSizeLimit to see if to add to _ignoredFilesCount.

Should we use the same logic, by just using the else

We can add more logics to ShouldEnqueue in the future.

shaopeng-gh · 2022-12-29T00:21:45Z

src/Sarif.Driver/Sdk/AnalyzeOptionsBase.cs

@@ -118,7 +129,7 @@
        [Option(
            "max-file-size-in-kb",
            HelpText = "The maximum file size (in kilobytes) that will be analyzed.",
-            Default = 1024)]
-        public int MaxFileSizeInKilobytes { get; set; } = 1024;
+            Default = -1)]


-1

the benefit of have 1024 in the CommonOption here is it will be shown to user. with changed to -1, it is actually 1024 as we coded inside c# but just is not as clear to user.

shaopeng-gh

michaelcfanning added 5 commits December 28, 2022 07:27

Add overridable filtering mechanism to driver file enumeration phase.

26a4291

Update max file size type. Obsolete AnalyzeCommandBase.

c67b6dd

Resolve hangs in file enumeration phase.

7703770

Merge from main

c1e8330

Test fixes.

9baabcb

github-advanced-security bot found potential problems Dec 28, 2022

View reviewed changes

michaelcfanning added 2 commits December 28, 2022 13:26

Restore more tests to working order.

24dedbd

All tests now passing.

e50fb2e

michaelcfanning commented Dec 28, 2022

View reviewed changes

michaelcfanning added 2 commits December 28, 2022 14:20

Updates after first code review.

47a0c26

Add new analyze context base file.

de74667

michaelcfanning marked this pull request as ready for review December 28, 2022 22:21

michaelcfanning requested review from EasyRhinoMSFT, marmegh and cfaucon as code owners December 28, 2022 22:21

github-advanced-security bot found potential problems Dec 28, 2022

View reviewed changes

suvamM approved these changes Dec 28, 2022

View reviewed changes

michaelcfanning added 2 commits December 28, 2022 15:07

Make max file size arg overridable.

8877214

Merge branch 'main' into enqueue-files-filter

ee6c142

michaelcfanning enabled auto-merge (squash) December 28, 2022 23:09

michaelcfanning disabled auto-merge December 28, 2022 23:16

Consult context for maf file size not options.

a68651f

Merge branch 'enqueue-files-filter' of https://github.com/microsoft/s…

7af863f

…arif-sdk into enqueue-files-filter

yongyan-gh reviewed Dec 28, 2022

View reviewed changes

Ignored files count should be restricted to those that exceed file si…

0c60205

…ze only.

shaopeng-gh reviewed Dec 28, 2022

View reviewed changes

michaelcfanning added 2 commits December 28, 2022 16:10

File size test fixups due to default arg behavior change.

50642ab

File size test fixups due to default arg behavior change.

6e658bb

shaopeng-gh reviewed Dec 29, 2022

View reviewed changes

shaopeng-gh approved these changes Dec 29, 2022

View reviewed changes

michaelcfanning merged commit 859c2fe into main Dec 29, 2022

michaelcfanning deleted the enqueue-files-filter branch December 29, 2022 00:25

This was referenced Feb 21, 2023

Releasing v3.2.0 #2622

Closed

Releasing 4.0.0 #2623

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enqueue files filter #2599

Enqueue files filter #2599

michaelcfanning commented Dec 28, 2022 •

edited

Loading

michaelcfanning Dec 28, 2022

yongyan-gh Dec 28, 2022

michaelcfanning Dec 28, 2022

michaelcfanning Dec 28, 2022

michaelcfanning Dec 28, 2022

michaelcfanning Dec 28, 2022

michaelcfanning Dec 28, 2022

michaelcfanning Dec 28, 2022

michaelcfanning Dec 28, 2022

michaelcfanning Dec 28, 2022

yongyan-gh Dec 28, 2022

shaopeng-gh Dec 28, 2022

shaopeng-gh Dec 29, 2022

shaopeng-gh left a comment

Enqueue files filter #2599

Enqueue files filter #2599

Conversation

michaelcfanning commented Dec 28, 2022 • edited 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh left a comment

Choose a reason for hiding this comment

michaelcfanning commented Dec 28, 2022 •

edited

Loading