Fix `ArgumentException` when recurse is enabled and two file target specifiers generates the same file paths #2438

eddynaka · 2022-02-01T21:44:18Z

Description

If you pass two targets like c:\path1\1.dll, c:\path1\path2\1.dll and recurse is enabled, the MultithreadedAnalyzeCommandBase would generate the following searches:
c:\path1\ -> search using recurse looking for 1.dll
c:\path1\path2\ -> search using recurse looking for 1.dll

This would generate path duplications, causing an ArgumentException when hashing:

error ERR999.UnhandledEngineException : System.ArgumentException: The key already existed in the dictionary.
at System.Collections.Concurrent.ConcurrentDictionary`2.System.Collections.Generic.IDictionary<TKey,TValue>.Add(TKey key, TValue value)
at Microsoft.CodeAnalysis.Sarif.Driver.MultithreadedAnalyzeCommandBase`2.HashAsync() in /_/src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs:line 494
ArgumentException: The key already existed in the dictionary.
at System.Collections.Concurrent.ConcurrentDictionary`2.System.Collections.Generic.IDictionary<TKey,TValue>.Add(TKey key, TValue value)
at Microsoft.CodeAnalysis.Sarif.Driver.MultithreadedAnalyzeCommandBase`2.HashAsync()

Tests

Created a test where I'm passing two target specifiers that will list the same file. When that happens, no exception should occur since we will start checking if we already added the item to the IDictionary.

…plicated file analysis

eddynaka · 2022-02-01T21:51:55Z

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

@@ -1509,7 +1561,7 @@ private static IFileSystem CreateDefaultFileSystemForResultsCaching(IList<string
                mockFileSystem.Setup(x => x.FileReadAllText(It.Is<string>(f => f == fullyQualifiedName))).Returns(logFileContents);

                mockFileSystem.Setup(x => x.FileOpenRead(It.Is<string>(f => f == fullyQualifiedName)))
-                        .Returns(new MemoryStream(Encoding.UTF8.GetBytes(generateSameInput ? logFileContents : fileNameWithoutExtension)));
+                    .Returns(new NonDisposingDelegatingStream(new MemoryStream(Encoding.UTF8.GetBytes(generateSameInput ? logFileContents : fileNameWithoutExtension))));


NonDisposingDelegatingStream

This will enable us to read over and over during tests without generating an exception where the stream is already closed.

michaelcfanning · 2022-02-01T21:54:59Z

src/ReleaseHistory.md

@@ -6,6 +6,7 @@
 * BUGFIX: Adjust Json Serialization field order for ReportingDescriptor and skip emit empty AutomationDetails node. [#2420](https://github.com/microsoft/sarif-sdk/pull/2420)
 * 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)


recurse

--recurse #Closed

michaelcfanning · 2022-02-01T21:56:30Z

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

+
+            Action action = () =>
+            {
+                foreach (var testCase in testCases)


foreach (var testCase in testCases)

foreach (bool multithreaded in new bool[] { true, false} #Closed

done!
thanks for the suggestion!

michaelcfanning

…pecifiers generates the same file paths (#2438) * Fixing ArgumentException when passing two filePaths that generates duplicated file analysis * Fixing dotnet-format issues and updating releasehistory * Removing comments * Addressing PR feedback * Addressing PR feedback

) * Add new visitor to get deterministic SARIF log by sorting results * Fix dotnet format issue * updating format * remove unnecessary using Format & minor fixes * Add Run Comparer to support sorting logs with multiple runs. * Add command argument unit tests fix dotnet format * use ContainsKey to avoid allocating variable * Fixing `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` artifacts generation (#2433) * Fixing multithreaded artifacts generation * Fixing tests and flags * Loosing precision. * Applying fix for AnalyzeCommandBase * Enabling tests * Updating test case and release history * Creating const to prevent magical numbers everywhere * Rebaselining tests * Creating Artifacts flag to keep previous behavior * Addressing PR feedback. * Rollback changes * Update SARIF2012.ProvideRuleProperties_Invalid.sarif * updating back * Ordering deprecated names * `SarifLogger` now emits an artifacts table entry if `artifactLocation` is not null for tool configuration and tool execution notifications. (#2437) * Fixing artifacts generation when logging notifications * Updating release history. * Updating ReleaseHistory * Fix `ArgumentException` when recurse is enabled and two file target specifiers generates the same file paths (#2438) * Fixing ArgumentException when passing two filePaths that generates duplicated file analysis * Fixing dotnet-format issues and updating releasehistory * Removing comments * Addressing PR feedback * Addressing PR feedback * Addressing PR review issues Add suppression support (#2435) * Add suppression support * Add incompatibility check and make suppressions non-null Co-authored-by: Eddy Nakamura <eddynaka@gmail.com> Update releasehistory fix couple test cases * Fix issues in PR review * Add xml comments * Fix test issues * fix dotnet format * Addressing review feedbacks * Fix tests * Update extension methods names * Change xml doc comments to normal comments Co-authored-by: Eddy Nakamura <eddynaka@gmail.com> Co-authored-by: Michael C. Fanning <michael.fanning@microsoft.com>

Fixing ArgumentException when passing two filePaths that generates du…

95c979d

…plicated file analysis

eddynaka changed the title ~~Fixing ArgumentException when passing two filePaths that generate duplicated file analysis and recurse is enabled.~~ Fix ArgumentException when recurse is enabled and two file target specifiers generates the same file paths Feb 1, 2022

eddynaka added 2 commits February 1, 2022 13:47

Fixing dotnet-format issues and updating releasehistory

663b3ab

Removing comments

b302644

eddynaka commented Feb 1, 2022

View reviewed changes

eddynaka marked this pull request as ready for review February 1, 2022 21:53

eddynaka requested a review from michaelcfanning as a code owner February 1, 2022 21:53

michaelcfanning reviewed Feb 1, 2022

View reviewed changes

michaelcfanning approved these changes Feb 1, 2022

View reviewed changes

Addressing PR feedback

f0c13bc

eddynaka enabled auto-merge (squash) February 1, 2022 21:59

eddynaka disabled auto-merge February 1, 2022 22:02

Addressing PR feedback

da0002b

eddynaka enabled auto-merge (squash) February 1, 2022 22:04

eddynaka merged commit 7851441 into main Feb 1, 2022

eddynaka deleted the users/ednakamu/fixing-recurse-issue-targets-specifier branch February 1, 2022 22:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `ArgumentException` when recurse is enabled and two file target specifiers generates the same file paths #2438

Fix `ArgumentException` when recurse is enabled and two file target specifiers generates the same file paths #2438

eddynaka commented Feb 1, 2022 •

edited

eddynaka Feb 1, 2022

michaelcfanning Feb 1, 2022 •

edited by eddynaka

michaelcfanning Feb 1, 2022 •

edited by eddynaka

eddynaka Feb 1, 2022

michaelcfanning left a comment

Fix ArgumentException when recurse is enabled and two file target specifiers generates the same file paths #2438

Fix ArgumentException when recurse is enabled and two file target specifiers generates the same file paths #2438

Conversation

eddynaka commented Feb 1, 2022 • edited

Description

Tests

eddynaka Feb 1, 2022

Choose a reason for hiding this comment

michaelcfanning Feb 1, 2022 • edited by eddynaka

Choose a reason for hiding this comment

michaelcfanning Feb 1, 2022 • edited by eddynaka

Choose a reason for hiding this comment

eddynaka Feb 1, 2022

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

Fix `ArgumentException` when recurse is enabled and two file target specifiers generates the same file paths #2438

Fix `ArgumentException` when recurse is enabled and two file target specifiers generates the same file paths #2438

eddynaka commented Feb 1, 2022 •

edited

michaelcfanning Feb 1, 2022 •

edited by eddynaka

michaelcfanning Feb 1, 2022 •

edited by eddynaka