Analyzer flagging problematic APIs in ThreadSafeTask#12143
Draft
JanProvaznik wants to merge 13 commits intodotnet:mainfrom
Draft
Analyzer flagging problematic APIs in ThreadSafeTask#12143JanProvaznik wants to merge 13 commits intodotnet:mainfrom
JanProvaznik wants to merge 13 commits intodotnet:mainfrom
Conversation
JanProvaznik
force-pushed
the
threadsafe-analyzer
branch
from
4180290 to
843f1b1
Compare
JanProvaznik
added a commit
to JanProvaznik/msbuild
that referenced
this pull request
Sep 30, 2025
This spec proposes a Roslyn analyzer to detect unsafe API usage in IMultiThreadableTask implementations. The analyzer categorizes problematic APIs into 4 tiers (MSB9996-9999) and provides automated code fixes for common issues. Key features: - MSB9999 (Error): Critical APIs with no safe alternative (Environment.Exit, ThreadPool settings) - MSB9998 (Warning): APIs requiring TaskEnvironment alternatives (Environment.CurrentDirectory, GetEnvironmentVariable) - MSB9997 (Warning): File APIs requiring absolute paths (File.*, Directory.*) - MSB9996 (Warning): Potentially problematic APIs (Console.*, Assembly.Load*) Full implementation available in PR dotnet#12143
MSBuildTask0001: Critical unsafe APIs (Console.*, Environment.Exit, etc.) - Error MSBuildTask0002: Environment/path APIs needing TaskEnvironment - Warning MSBuildTask0003: File/Directory APIs with unwrapped path arguments - Warning MSBuildTask0004: Dynamic assembly loading (Assembly.Load, etc.) - Warning MSBuildTask0005: Transitive unsafe API calls through call graph - Warning - 97 passing tests (80 direct, 6 code fix, 10 transitive, 1 nullable) - Code fixers for MSBuildTask0002 and MSBuildTask0003 - Nullable AbsolutePath? implicit conversion detection - Type-level Console ban (catches all Console.* members) - Helper class opt-in via [MSBuildMultiThreadableTaskAnalyzed] - Transitive call graph BFS with max depth 20 - Validated against MSBuild Tasks project (256 violations found)
JanProvaznik
force-pushed
the
threadsafe-analyzer
branch
from
81efecd to
6355c84
Compare
…cal variables - Add Path.GetDirectoryName(safe), Path.Combine(safe, ...), Path.GetFullPath(safe) recognition to IsWrappedSafely in both direct and transitive analyzers - Add local variable tracing: follow ILocalReferenceOperation back to its initializer to check if the assigned value is safe - Fix nullable AbsolutePath? handling in transitive analyzer (was missing) - Add 10 new unit tests covering all patterns (positive and negative cases) - Eliminates 11 false positives on MSBuild Tasks (18 -> 7 direct violations) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move null literal and default value checks after conversion unwrap to correctly handle Roslyn wrapping null in IConversionOperation. Also apply the same fix to TransitiveCallChainAnalyzer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Phase 1.5: When the transitive analyzer's BFS encounters a method with no source code (defined in a referenced assembly), read its IL via System.Reflection.Metadata to discover outgoing call edges and banned API violations. Key features: - PEReader cache for efficient assembly loading - IL opcode parser for call/callvirt/newobj/ldftn/ldvirtftn - Method resolution: metadata tokens → IMethodSymbol via Compilation - Safe-type suppression: AbsolutePath, TaskEnvironment internal calls are not flagged (they handle paths safely) - BCL boundary: System.*, mscorlib not traversed - Property getter/setter edges in call graph - Property accessor → banned property mapping for IL-discovered calls - File-path type checking for IL-discovered calls 8 new cross-assembly tests (114 total, all pass). Verified against MSBuild Tasks: 256 transitive violations, 0 FP from safe types, 14s build time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rewrite IL opcode table with correct ECMA-335 classifications Fixed: 0x6E (conv.ovf.u8.un) was 4→0, 0x8E (ldlen) was 4→0, 0xC2 (refanyval) was 0→4, FE.14 (initobj) was 0→4, FE.1C (sizeof) was 0→4, added box (0x8C) and ldelema (0x8F) - Add proper switch opcode handling in GetOperandSize - Add property getter/setter edges in call graph (ScanOperation) - Add property accessor → banned property mapping for IL violations - Add file-path type checking for IL-discovered calls - Add IsPathParameterName to ILCallGraphExtender for constructor overload selection - 10 cross-assembly tests (116 total, all pass) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously MSBuildTask0002 and MSBuildTask0003 only fired on IMultiThreadableTask implementations. Now all 5 rules fire on every ITask so the analyzer can guide migration of any task, not just those already opted in. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add comprehensive list of BCL types that accept file path strings: - System.Xml.Linq: XDocument, XElement (Save/Load) - System.Xml: XmlDocument, XmlReader, XmlWriter, XmlTextReader/Writer - System.Xml.Xsl: XslCompiledTransform - System.IO.Compression: ZipFile - System.IO.MemoryMappedFiles: MemoryMappedFile - System.Security.Cryptography: X509Certificate, X509Certificate2 - System.Diagnostics: FileVersionInfo, TextWriterTraceListener - System.Resources: ResourceReader, ResourceWriter - System.IO: BinaryReader, BinaryWriter Also add 'uri'/'url' to path parameter name detection. Fire all rules on all ITask implementations (not just IMultiThreadableTask). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
New option: msbuild_task_analyzer.scope (in .editorconfig) - 'all' (default): All rules fire on every ITask implementation - 'multithreadable_only': MSBuildTask0002/0003/0005 fire only on IMultiThreadableTask or [MSBuildMultiThreadableTask] attributed tasks (MSBuildTask0001/0004 always fire on all tasks regardless) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix: When checking IL-discovered calls against file-path types (XDocument.Save, File.OpenRead, etc.), check ALL overloads of the method name for path-like parameters, not just the single resolved overload. ResolveToMethodSymbol returns the first match by name which may be a non-path overload (e.g., Save(Stream) instead of Save(string fileName)). Add 4 multi-assembly tests: - Two-library chain: Task → LibMiddle → LibBase → File.OpenRead - Three-library chain: Task → LibA → LibB → LibC → Environment.Exit - XDocument.Save through library wrapper - Mixed safe/unsafe library methods Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix: When checking IL-discovered calls against file-path types, check ALL overloads of the method name for path-like parameters. ResolveToMethodSymbol returns the first match by name which may be a non-path overload (e.g., Save(Stream) instead of Save(string)). Add 5 new tests: - Two-library chain: Task → LibMiddle → LibBase → File.OpenRead - Three-library chain: Task → LibA → LibB → LibC → Environment.Exit - XDocument.Save through library wrapper (was failing, now passes) - Mixed safe/unsafe library methods - Source helper → external lib → File.Open (SDK LockFileCache pattern) 124 tests all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…guation Two critical bugs preventing deep transitive chain detection: 1. ConcurrentBag enumeration loss: ConcurrentBag's thread-local storage caused items added from different threads during Phase 1 to be silently dropped during enumeration in Phase 2. Fixed by calling .ToArray() to snapshot the bag before iterating, ensuring all edges are visited. 2. IL overload resolution: ResolveToMethodSymbol returned the first method overload by name, ignoring parameter count. When a thin wrapper overload (e.g., GetLockFile(string, ILogger)) delegated to a fuller overload (e.g., GetLockFile(string, ILogger, int)), the IL resolver returned the same wrapper, creating an infinite loop that the already-visited check silently swallowed. Fixed by extracting parameter count from IL signature blobs (ECMA-335) and using it to disambiguate overloads in ResolveToMethodSymbol. These fixes double the SDK detection rate (380 → 770 violations) and correctly catch the CheckForTargetInAssetsFile → LockFileCache → NuGet.ProjectModel → File.Exists chain that was previously missed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #11833
Context
In threadsafe tasks it's dangerous to call some apis that modify or depend on process state since the process will be shared which could cause race conditions or unexpected behavior.
To benefit from the multithreaded epic for customers implementing custom tasks they have to opt in. We want to make it clear for them what is problematic. Flagging it in code seems like a reasonable option.
Using naively the BannedApiAnalyzer from roslyn would generate too much noise as some customers might not want to migrate their task, and also there are logging utilities in the package which we don't want to generate warnings for. So we need a custom implementation that analyzes only relevant code.
finalization depends on #12111
Changes Made
new Roslyn analyzer inspired by https://github.com/dotnet/roslyn/blob/69728a35a4414e1d1b12dfcecccf178a34e99561/src/RoslynAnalyzers/Microsoft.CodeAnalysis.BannedApiAnalyzers/BannedApiAnalyzers.Help.md
Testing
for now manually tested that a project referencing the utilities package and implementing IThreadSafeTask flags the APIs
TODO: figure out a realistic e2e test (that has to depend on the packages)
Notes
we could consider also a codefixer that replaces the APIs instead of just flagging. On the other hand the migration requires some thought so it might be better to create some friction instead of "press fix all and then be surprised something broke".
open question: the config file could be integrated into the source code which would enable localization. For now I sticked to the bannedapianalyzer inspiration but that might not be wise.
We can consider also creating an analyzer message for all importing the regular
Taskabstract class along the lines "have you considered migrating your Task implementation to IThreadSafeTask"?