-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enqueue files filter #2599
Merged
Merged
Enqueue files filter #2599
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
26a4291
Add overridable filtering mechanism to driver file enumeration phase.
michaelcfanning c67b6dd
Update max file size type. Obsolete AnalyzeCommandBase.
michaelcfanning 7703770
Resolve hangs in file enumeration phase.
michaelcfanning c1e8330
Merge from main
michaelcfanning 9baabcb
Test fixes.
michaelcfanning 24dedbd
Restore more tests to working order.
michaelcfanning e50fb2e
All tests now passing.
michaelcfanning 47a0c26
Updates after first code review.
michaelcfanning de74667
Add new analyze context base file.
michaelcfanning 8877214
Make max file size arg overridable.
michaelcfanning ee6c142
Merge branch 'main' into enqueue-files-filter
michaelcfanning a68651f
Consult context for maf file size not options.
michaelcfanning 7af863f
Merge branch 'enqueue-files-filter' of https://github.com/microsoft/s…
michaelcfanning 0c60205
Ignored files count should be restricted to those that exceed file si…
michaelcfanning 50642ab
File size test fixups due to default arg behavior change.
michaelcfanning 6e658bb
File size test fixups due to default arg behavior change.
michaelcfanning File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
|
||
namespace Microsoft.CodeAnalysis.Sarif.Driver | ||
{ | ||
[Obsolete("AnalyzeCommandBase will be deprecated entirely soon. Use MultithreadedAnalyzeCommandBase instead.")] | ||
public abstract class AnalyzeCommandBase<TContext, TOptions> : PluginDriverCommand<TOptions> | ||
where TContext : IAnalysisContext, new() | ||
where TOptions : AnalyzeOptionsBase | ||
|
@@ -97,12 +98,13 @@ public override int Run(TOptions options) | |
catch (Exception ex) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
// These exceptions escaped our net and must be logged here | ||
RuntimeErrors |= Errors.LogUnhandledEngineException(_rootContext, ex); | ||
Errors.LogUnhandledEngineException(_rootContext, ex); | ||
ExecutionException = ex; | ||
return FAILURE; | ||
} | ||
finally | ||
{ | ||
RuntimeErrors |= _rootContext.RuntimeErrors; | ||
logger.AnalysisStopped(RuntimeErrors); | ||
} | ||
} | ||
|
@@ -207,7 +209,7 @@ protected virtual void ValidateOptions(TContext context, TOptions analyzeOptions | |
succeeded &= ValidateFile(context, analyzeOptions.OutputFilePath, DefaultPolicyName, shouldExist: null); | ||
succeeded &= ValidateInvocationPropertiesToLog(context, analyzeOptions.InvocationPropertiesToLog); | ||
succeeded &= analyzeOptions.ValidateOutputOptions(context); | ||
succeeded &= analyzeOptions.MaxFileSizeInKilobytes > 0; | ||
succeeded &= context.MaxFileSizeInKilobytes >= 0; | ||
|
||
if (!succeeded) | ||
{ | ||
|
@@ -217,6 +219,18 @@ protected virtual void ValidateOptions(TContext context, TOptions analyzeOptions | |
} | ||
} | ||
|
||
protected virtual bool ShouldEnqueue(string file, TContext context) | ||
{ | ||
bool shouldEnqueue = IsTargetWithinFileSizeLimit(file, context.MaxFileSizeInKilobytes, out long fileSizeInKb); | ||
|
||
if (!shouldEnqueue) | ||
{ | ||
Warnings.LogFileSkippedDueToSize(context, file, fileSizeInKb); | ||
} | ||
|
||
return shouldEnqueue; | ||
} | ||
|
||
internal AggregatingLogger InitializeLogger(AnalyzeOptionsBase analyzeOptions) | ||
{ | ||
_tool = Tool.CreateFromAssemblyData(); | ||
|
@@ -257,7 +271,7 @@ private ISet<string> CreateTargetsSet(TOptions analyzeOptions) | |
foreach (string file in fileSpecifier.Files) | ||
{ | ||
// Only include files that are below the max size limit. | ||
if (IsTargetWithinFileSizeLimit(file, _rootContext.MaxFileSizeInKilobytes)) | ||
if (ShouldEnqueue(file, _rootContext)) | ||
{ | ||
targets.Add(file); | ||
} | ||
|
@@ -288,10 +302,13 @@ protected virtual TContext CreateContext( | |
{ | ||
Logger = logger, | ||
RuntimeErrors = runtimeErrors, | ||
Policy = policy | ||
Policy = policy ?? new PropertiesDictionary() | ||
}; | ||
|
||
context.MaxFileSizeInKilobytes = options.MaxFileSizeInKilobytes; | ||
context.MaxFileSizeInKilobytes = | ||
options.MaxFileSizeInKilobytes >= 0 | ||
? options.MaxFileSizeInKilobytes | ||
: AnalyzeContextBase.MaxFileSizeInKilobytesDefaultValue; | ||
|
||
if (filePath != null) | ||
{ | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,17 @@ namespace Microsoft.CodeAnalysis.Sarif.Driver | |
[Verb("analyze", HelpText = "Analyze one or more binary files for security and correctness issues.")] | ||
public abstract class AnalyzeOptionsBase : CommonOptionsBase | ||
{ | ||
public AnalyzeOptionsBase() | ||
{ | ||
// TODO: these defaults need to be converted to the configuration | ||
// property pattern as followed by MaxFileSizeInKilobytes. | ||
Traces = new string[] { }; | ||
Check warning Code scanning / CodeQL Virtual call in constructor or destructor
Avoid virtual calls in a constructor or destructor.
|
||
Kind = new List<ResultKind> { ResultKind.Fail }; | ||
Level = new List<FailureLevel> { FailureLevel.Warning, FailureLevel.Error }; | ||
|
||
MaxFileSizeInKilobytes = AnalyzeContextBase.MaxFileSizeInKilobytesDefaultValue; | ||
} | ||
|
||
[Value(0, | ||
HelpText = "One or more specifiers to a file, directory, or filter pattern that resolves to one or more binaries to analyze.")] | ||
public IEnumerable<string> TargetFileSpecifiers { get; set; } | ||
|
@@ -118,7 +129,7 @@ public abstract class AnalyzeOptionsBase : CommonOptionsBase | |
[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)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
public long MaxFileSizeInKilobytes { get; set; } | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It grows wearisome to maintain this obsolete single-threaded analysis, let's zap it next release.