Visitar URL original
Fix thread safety issues by Evangelink · Pull Request #15512 · microsoft/vstest · GitHub
Skip to content

Fix thread safety issues - #15512

Merged
Jakub Jareš (nohwnd) merged 7 commits into
mainfrom
fix/thread-safety-and-xxe-vulnerabilities
Mar 19, 2026
Merged

Fix thread safety issues#15512
Jakub Jareš (nohwnd) merged 7 commits into
mainfrom
fix/thread-safety-and-xxe-vulnerabilities

Conversation

@Evangelink

@Evangelink Amaury Levé (Evangelink) commented Mar 18, 2026 •
edited by nohwnd
Loading

Copy link
Copy Markdown
Member

Summary

Several data structures shared across concurrent test execution threads were not thread-safe, risking corrupted results and lost updates.

Changes

  • ParallelRunDataAggregator / DiscoveryDataAggregator: Replace TryGetValue + set pattern with ConcurrentDictionary.AddOrUpdate in metrics aggregation to eliminate lost-update races
  • DataCollectionAttachmentManager: Replace List<Task> with ConcurrentBag<Task> for _attachmentTasks; replace ContainsKey + TryAdd TOCTOU pattern with GetOrAdd for both AttachmentSets and _attachmentTasks
  • BlameCollector: Replace List<Guid> / Dictionary<Guid, BlameTestObject> with ConcurrentQueue / ConcurrentDictionary; use Interlocked.Increment for _testEndCount; snapshot collections before passing to WriteTestSequence
  • TrxLogger / HtmlLogger: Use Interlocked.Increment for all test counters (TotalTestCount, PassedTestCount, FailedTestCount, SkippedTests) to prevent lost updates under concurrent result arrival

@Evangelink
Fix thread safety in ParallelRunDataAggregator and DiscoveryDataAggre…
ffb7f44 
…gator

- GetAggregatedRunStats() now reads _testRunStatsList under lock
  to prevent InvalidOperationException from concurrent List<T>
  modification during enumeration
- AggregateRunDataMetrics()/AggregateMetrics() now uses
  ConcurrentDictionary.AddOrUpdate instead of TryGetValue+set to
  prevent lost-update race conditions
- Same fix applied to DiscoveryDataAggregator.AggregateMetrics()
- Added concurrency tests that exercise parallel Aggregate + read
@Evangelink
Fix thread safety in DataCollectionAttachmentManager
45201d8 
- Replace List<Task> with ConcurrentBag<Task> for _attachmentTasks
  to prevent corruption when concurrent file transfers call Add()
- Replace ContainsKey+TryAdd TOCTOU pattern with GetOrAdd for both
  AttachmentSets and _attachmentTasks dictionaries
- Existing ParallelAccessShouldNotBreak test covers this scenario
@Evangelink
Fix thread safety in BlameCollector
57c1435 
- Add lock around _testSequence and _testObjectDictionary mutations
  in EventsTestCaseStart and EventsTestCaseEnd to prevent corruption
  under parallel test execution
- Use Interlocked.Increment for _testStartCount and _testEndCount
- Take snapshot under lock in SessionEndedHandler before passing to
  WriteTestSequence
- Added concurrency test with 10 threads x 50 test events each
@Evangelink
Fix thread safety in TrxLogger and HtmlLogger test counters
aec0370 
- TrxLogger: use Interlocked.Increment for TotalTestCount,
  PassedTestCount, and FailedTestCount to prevent lost updates when
  test results arrive concurrently from parallel test runs
- HtmlLogger: same fix for TotalTests, PassedTests, FailedTests,
  SkippedTests counters
- Added thread safety test exercising 10 threads x 100 results each
  verifying exact counter values
@Evangelink
Fix XXE vulnerability in SettingsMigrator
4db2c88 
- Set DtdProcessing = DtdProcessing.Prohibit on XmlTextReader in both
  MigrateRunSettings and ReadTestSettingsNodes to prevent XML External
  Entity attacks via crafted .runsettings/.testsettings files
- Set XmlResolver = null on XmlDocument instances
- Added tests verifying DTD content in both runsettings and
  testsettings files is rejected with XmlException
Copilot AI review requested due to automatic review settings March 18, 2026 14:12
Copilot AI reviewed Mar 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@Evangelink
Amaury Levé (Evangelink) marked this pull request as ready for review March 19, 2026 08:17
nohwnd
Jakub Jareš (nohwnd) reviewed Mar 19, 2026
View reviewed changes
Comment thread src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs Resolved
Comment thread src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/DiscoveryDataAggregator.cs Resolved
Comment thread src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelRunDataAggregator.cs Resolved
Comment thread src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs Resolved
Comment thread src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs Resolved
Comment thread src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs Resolved
Comment thread src/SettingsMigrator/Migrator.cs Resolved
Comment thread ...oft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelRunDataAggregatorTests.cs Resolved
Comment thread test/SettingsMigrator.UnitTests/MigratorTests.cs Resolved
@nohwnd Jakub Jareš (nohwnd) changed the title Fix thread safety issues and XXE vulnerability Fix thread safety issues Mar 19, 2026
@nohwnd

Jakub Jareš (nohwnd) commented Mar 19, 2026

Copy link
Copy Markdown
Member

Copilot some parts were reverted, update description.

Copilot AI commented Mar 19, 2026

Copy link
Copy Markdown
Contributor

Jakub Jareš (@nohwnd) I've opened a new pull request, #15514, to work on those changes. Once the pull request is ready, I'll request review from you.

Initial plan (#15514)
bf0bc3e 
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
@nohwnd
Jakub Jareš (nohwnd) merged commit d895571 into main Mar 19, 2026
6 checks passed
@nohwnd
Jakub Jareš (nohwnd) deleted the fix/thread-safety-and-xxe-vulnerabilities branch March 19, 2026 12:43
This was referenced May 26, 2026
Merged
Merged
Open
Closed
Merged
Closed
Closed
Merged
Open
Closed
Closed
This was referenced Nov 11, 2026
Closed
Closed
Merged
Closed
Merged
Closed
Open
Closed
Open
Open
Closed
Open
Open
Merged
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Closed
Open
Open
Open
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@nohwnd Jakub Jareš (nohwnd) nohwnd approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Evangelink @nohwnd