Fix 'InvalidOperationException' with message Collection was modified; enumeration operation may not execute in MultithreadedAnalyzeCommandBase, which is raised when analyzing with the --hashes switch.

src/ReleaseHistory.md

@@ -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)
 

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

@@ -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;
 
@@ -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)
             {
@@ -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))
@@ -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;
@@ -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);

src/Sarif.Driver/Sdk/PluginDriverCommand.cs

@@ -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;

src/Sarif/HashUtilities.cs

@@ -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))
                     {

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

@@ -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);
@@ -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()
         {
@@ -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);
@@ -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);
@@ -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>
@@ -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);

src/Test.UnitTests.Sarif.Driver/TestRule.cs

@@ -7,6 +7,7 @@
 using System.IO;
 using System.Reflection;
 using System.Resources;
+using System.Threading;
 
 using FluentAssertions;
 
@@ -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;
 
@@ -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;
                 }
 
@@ -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; });