Skip to content

Commit

Permalink
Fix InvalidOperationException when --hashes is enabled and using …
Browse files Browse the repository at this point in the history 
…`MultithreadedAnalyzeCommandBase`. (#2447)

* Simplifying multithreaded analysis when hashes are the same

* Test ideas

* Removing unused field and fixing test

* Fixing all tests based on testrule errorcount, improving hashutilities to use FileSystem

* Addressing PR feedback

* Reducing to 50 iterations

* Updating release history and updating style

* Addressing PR feedback

Co-authored-by: Michael C. Fanning <mikefan@microsoft.com>
  • Loading branch information
@eddynaka @michaelcfanning
eddynaka and michaelcfanning authored Feb 14, 2022
1 parent 041400b commit 0177c03
Show file tree
Hide file tree
Showing 6 changed files with 106 additions and 40 deletions.
1 change: 1 addition & 0 deletions src/ReleaseHistory.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* BREAKING: Fix `InvalidOperationException` when using PropertiesDictionary in a multithreaded application, and remove `[Serializable]` from it. Now use of BinaryFormatter on it will result in `SerializationException`: Type `PropertiesDictionary` is not marked as serializable. [#2415](https://github.com/microsoft/sarif-sdk/pull/2415)
* BREAKING: `SarifLogger` now emits an artifacts table entry if `artifactLocation` is not null for tool configuration and tool execution notifications. [#2437](https://github.com/microsoft/sarif-sdk/pull/2437)
* BUGFIX: Fix `ArgumentException` when `--recurse` is enabled and two file target specifiers generates the same file path. [#2438](https://github.com/microsoft/sarif-sdk/pull/2438)
* BUGFIX: Fix 'InvalidOperationException' with message `Collection was modified; enumeration operation may not execute` in `MultithreadedAnalyzeCommandBase`, which is raised when analyzing with the `--hashes` switch. [#2447](https://github.com/microsoft/sarif-sdk/pull/2447)

## **v2.4.12** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.12) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.12) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.12) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.12) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.12)

Expand Down
34 changes: 11 additions & 23 deletions src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ public abstract class MultithreadedAnalyzeCommandBase<TContext, TOptions> : Plug
private OptionallyEmittedData _dataToInsert;
private Channel<int> _resultsWritingChannel;
private Channel<int> _fileEnumerationChannel;
private Dictionary<string, List<string>> _hashToFilesMap;
private IDictionary<string, HashData> _pathToHashDataMap;
private ConcurrentDictionary<int, TContext> _fileContexts;

Expand Down Expand Up @@ -186,9 +185,9 @@ private void MultithreadedAnalyzeTargets(TOptions options,
IEnumerable<Skimmer<TContext>> skimmers,
ISet<string> disabledSkimmers)
{
options.Threads = options.Threads > 0 ?
options.Threads :
(Debugger.IsAttached) ? 1 : Environment.ProcessorCount;
options.Threads = options.Threads > 0
? options.Threads
: (Debugger.IsAttached) ? 1 : Environment.ProcessorCount;

var channelOptions = new BoundedChannelOptions(2000)
{
Expand Down Expand Up @@ -470,23 +469,11 @@ private async Task<bool> HashAsync()
{
if (_computeHashes)
{
if (_hashToFilesMap == null)
{
_hashToFilesMap = new Dictionary<string, List<string>>();
}

TContext context = _fileContexts[index];
string localPath = context.TargetUri.LocalPath;

HashData hashData = HashUtilities.ComputeHashes(localPath);

if (!_hashToFilesMap.TryGetValue(hashData.Sha256, out List<string> paths))
{
paths = new List<string>();
_hashToFilesMap[hashData.Sha256] = paths;
}
HashData hashData = HashUtilities.ComputeHashes(localPath, FileSystem);

paths.Add(localPath);
context.Hashes = hashData;

if (_pathToHashDataMap != null && !_pathToHashDataMap.ContainsKey(localPath))
Expand Down Expand Up @@ -592,7 +579,7 @@ protected virtual TContext CreateContext(TOptions options,

if (filePath != null)
{
context.TargetUri = new Uri(filePath);
context.TargetUri = new Uri(filePath, UriKind.RelativeOrAbsolute);
}

return context;
Expand Down Expand Up @@ -873,21 +860,22 @@ protected virtual TContext DetermineApplicabilityAndAnalyze(

IAnalysisLogger logger = context.Logger;

int numberOfFiles = 1;
if (_computeHashes)
{
numberOfFiles = _hashToFilesMap[context.Hashes.Sha256].Count;
if (numberOfFiles > 1 && _analysisLoggerCache.ContainsKey(context.Hashes.Sha256))
if (_analysisLoggerCache.ContainsKey(context.Hashes.Sha256))
{
return context;
}
}

context.Logger.AnalyzingTarget(context);

if (_computeHashes && numberOfFiles > 1)
if (_computeHashes)
{
_analysisLoggerCache[context.Hashes.Sha256] = logger;
if (!_analysisLoggerCache.TryAdd(context.Hashes.Sha256, logger))
{
return context;
}
}

IEnumerable<Skimmer<TContext>> applicableSkimmers = DetermineApplicabilityForTarget(context, skimmers, disabledSkimmers);
Expand Down
3 changes: 2 additions & 1 deletion src/Sarif.Driver/Sdk/PluginDriverCommand.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ internal bool ValidateOutputFileCanBeCreated(IAnalysisContext context, string ou
{
bool succeeded = true;

if (!DriverUtilities.CanCreateOutputFile(outputFilePath, force, FileSystem))
if (!string.IsNullOrWhiteSpace(outputFilePath) &&
!DriverUtilities.CanCreateOutputFile(outputFilePath, force, FileSystem))
{
Errors.LogOutputFileAlreadyExists(context, outputFilePath);
succeeded = false;
Expand Down
6 changes: 4 additions & 2 deletions src/Sarif/HashUtilities.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,13 @@ public static IDictionary<string, HashData> MultithreadedComputeTargetFileHashes

[SuppressMessage("Microsoft.Security.Cryptography", "CA5354:SHA1CannotBeUsed")]
[SuppressMessage("Microsoft.Security.Cryptography", "CA5350:MD5CannotBeUsed")]
public static HashData ComputeHashes(string fileName)
public static HashData ComputeHashes(string fileName, IFileSystem fileSystem = null)
{
fileSystem ??= FileSystem;

try
{
using (Stream stream = FileSystem.FileOpenRead(fileName))
using (Stream stream = fileSystem.FileOpenRead(fileName))
{
using (var bufferedStream = new BufferedStream(stream, 1024 * 32))
{
Expand Down
71 changes: 65 additions & 6 deletions src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ public void AnalyzeCommandBase_DefaultEndToEndAnalysis()

// As configured by injected TestRuleBehaviors, we should
// see an error per scan target (one file in this case).
resultCount.Should().Be(1);
resultCount.Should().Be((int)TestRule.ErrorsCount.DefaultValue());
run.Results[0].Kind.Should().Be(ResultKind.Fail);

toolNotificationCount.Should().Be(0);
Expand All @@ -676,6 +676,65 @@ public void AnalyzeCommandBase_EndToEndAnalysisWithPostUri()
PostUriTestHelper(@"https://host.does.not.exist", TestAnalyzeCommand.FAILURE, RuntimeConditions.ExceptionPostingLogFile);
}

[Fact]
public void MultithreadedAnalyzeCommandBase_EndToEndMultithreadedAnalysis()
{
string specifier = "*.xyz";

int filesCount = 10;
var files = new List<string>();
for (int i = 0; i < filesCount; i++)
{
files.Add(Path.GetFullPath($@".\File{i}.txt"));
}

var propertiesDictionary = new PropertiesDictionary();
propertiesDictionary.SetProperty(TestRule.ErrorsCount, (uint)15);
propertiesDictionary.SetProperty(TestRule.Behaviors, TestRuleBehaviors.LogError);

using var tempFile = new TempFile(".xml");
propertiesDictionary.SaveToXml(tempFile.Name);

var mockStream = new Mock<Stream>();
mockStream.Setup(m => m.CanRead).Returns(true);
mockStream.Setup(m => m.CanSeek).Returns(true);
mockStream.Setup(m => m.ReadByte()).Returns('a');

var mockFileSystem = new Mock<IFileSystem>();
mockFileSystem.Setup(x => x.DirectoryExists(It.IsAny<string>())).Returns(true);
mockFileSystem.Setup(x => x.DirectoryGetFiles(It.IsAny<string>(), specifier)).Returns(files);
mockFileSystem.Setup(x => x.FileExists(It.Is<string>(s => s.EndsWith(specifier)))).Returns(true);
mockFileSystem.Setup(x => x.DirectoryEnumerateFiles(It.IsAny<string>(),
It.IsAny<string>(),
It.IsAny<SearchOption>())).Returns(files);
mockFileSystem.Setup(x => x.FileOpenRead(It.IsAny<string>())).Returns(mockStream.Object);
mockFileSystem.Setup(x => x.FileExists(tempFile.Name)).Returns(true);

Output.WriteLine($"The seed that will be used is: {TestRule.s_seed}");

for (int i = 0; i < 50; i++)
{
var options = new TestAnalyzeOptions
{
Threads = 10,
TargetFileSpecifiers = new[] { specifier },
SarifOutputVersion = SarifVersion.Current,
TestRuleBehaviors = TestRuleBehaviors.LogError,
DataToInsert = new[] { OptionallyEmittedData.Hashes },
ConfigurationFilePath = tempFile.Name
};

var command = new TestMultithreadedAnalyzeCommand(mockFileSystem.Object);
command.DefaultPluginAssemblies = new Assembly[] { this.GetType().Assembly };

int result = command.Run(options);

command.ExecutionException?.InnerException.Should().BeNull();

result.Should().Be(CommandBase.SUCCESS, $"Iteration: {i}, Seed: {TestRule.s_seed}");
}
}

[Fact]
public void AnalyzeCommandBase_PersistsSarifOneZeroZero()
{
Expand Down Expand Up @@ -739,7 +798,7 @@ public void AnalyzeCommandBase_RunDefaultRules()

// As configured by the inject TestRuleBehaviors value, we should see
// an error for every scan target (of which there is one file in this test).
resultCount.Should().Be(1);
resultCount.Should().Be((int)TestRule.ErrorsCount.DefaultValue());
run.Results[0].Level.Should().Be(FailureLevel.Error);

toolNotificationCount.Should().Be(0);
Expand Down Expand Up @@ -774,8 +833,8 @@ public void AnalyzeCommandBase_FireAllRules()
(configurationNotification) => { configurationNotificationCount++; });

// As configured by context, we should see a single error raised.
resultCount.Should().Be(1);
run.Results.Count((result) => result.Level == FailureLevel.Error).Should().Be(1);
resultCount.Should().Be((int)TestRule.ErrorsCount.DefaultValue());
run.Results.Count((result) => result.Level == FailureLevel.Error).Should().Be((int)TestRule.ErrorsCount.DefaultValue());

toolNotificationCount.Should().Be(0);
configurationNotificationCount.Should().Be(0);
Expand Down Expand Up @@ -1370,7 +1429,7 @@ public void AnalyzeCommandBase_MultithreadedShouldUseCacheIfFilesAreTheSame()
generateSameOutput: false,
expectedResultCode: 0,
expectedResultCount: 7,
expectedCacheSize: 0);
expectedCacheSize: 20);
}

private static readonly IList<string> ComprehensiveKindAndLevelsByFileName = new List<string>
Expand Down Expand Up @@ -1777,7 +1836,7 @@ private void PostUriTestHelper(string postUri, int expectedReturnCode, RuntimeCo

// As configured by injected TestRuleBehaviors, we should
// see an error per scan target (one file in this case).
resultCount.Should().Be(1);
resultCount.Should().Be((int)TestRule.ErrorsCount.DefaultValue());
run.Results[0].Kind.Should().Be(ResultKind.Fail);

toolNotificationCount.Should().Be(0);
Expand Down
31 changes: 23 additions & 8 deletions src/Test.UnitTests.Sarif.Driver/TestRule.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.IO;
using System.Reflection;
using System.Resources;
using System.Threading;

using FluentAssertions;

Expand All @@ -22,6 +23,9 @@ namespace Microsoft.CodeAnalysis.Sarif
[Export(typeof(ReportingDescriptor)), Export(typeof(IOptionsProvider)), Export(typeof(Skimmer<TestAnalysisContext>))]
internal class TestRule : TestRuleBase, IOptionsProvider
{
internal static int s_seed = (int)DateTime.UtcNow.Ticks;
internal static Random s_random = new Random(s_seed);

[ThreadStatic]
internal static TestRuleBehaviors s_testRuleBehaviors;

Expand Down Expand Up @@ -169,13 +173,20 @@ public override void Analyze(TestAnalysisContext context)

case TestRuleBehaviors.LogError:
{
context.Logger.Log(this,
new Result
{
RuleId = this.Id,
Level = FailureLevel.Error,
Message = new Message { Text = "Simple test rule message." }
});
uint errorsCount = context.Policy.GetProperty(ErrorsCount);

for (uint i = 0; i < errorsCount; i++)
{
context.Logger.Log(this,
new Result
{
RuleId = this.Id,
Level = FailureLevel.Error,
Message = new Message { Text = "Simple test rule message." }
});

Thread.Sleep(s_random.Next(0, 10));
}
break;
}

Expand Down Expand Up @@ -245,11 +256,15 @@ public static void RaiseExceptionViaReflection()

public IEnumerable<IOption> GetOptions()
{
return new IOption[] { Behaviors, UnusedOption };
return new IOption[] { Behaviors, UnusedOption, ErrorsCount };
}

private const string AnalyzerName = TestRuleId + "." + nameof(TestRule);

public static PerLanguageOption<uint> ErrorsCount { get; } =
new PerLanguageOption<uint>(
AnalyzerName, nameof(ErrorsCount), defaultValue: () => { return 1; });

public static PerLanguageOption<TestRuleBehaviors> Behaviors { get; } =
new PerLanguageOption<TestRuleBehaviors>(
AnalyzerName, nameof(TestRuleBehaviors), defaultValue: () => { return TestRuleBehaviors.None; });
Expand Down

0 comments on commit 0177c03

Please sign in to comment.