Migrate SignFile Task to TaskEnvironment API#13173
Conversation
|
Hello @@copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo. |
Co-authored-by: JanProvaznik <25267098+JanProvaznik@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Migrates the SignFile task to use the TaskEnvironment API for path resolution to better support MSBuild’s multithreaded execution model.
Changes:
- Mark
SignFileas multithread-capable via[MSBuildMultiThreadableTask]andIMultiThreadableTask, adding aTaskEnvironmentproperty. - Resolve
SigningTarget.ItemSpecto an absolute path viaTaskEnvironment.GetAbsolutePath(...)before signing. - Add handling/logging for
ArgumentExceptionthrown during path resolution.
| [SupportedOSPlatform("windows")] | ||
| public sealed class SignFile : Task | ||
| [MSBuildMultiThreadableTask] | ||
| public sealed class SignFile : Task, IMultiThreadableTask |
There was a problem hiding this comment.
Marking SignFile as [MSBuildMultiThreadableTask] / IMultiThreadableTask looks unsafe because it calls SecurityUtilities.SignFile(...), which internally depends on process-global state (e.g., falls back to Directory.GetCurrentDirectory() when locating signtool and uses SetDllDirectoryW during manifest signing). In a multithreaded build this can lead to nondeterministic tool resolution or interference between concurrently executing tasks. Consider removing the multithreadable attribute/interface until SecurityUtilities can take/use TaskEnvironment (or otherwise avoid process-global current directory / DLL directory changes) for tool discovery and process execution.
… in SecurityUtilities The SignFile task migration to IMultiThreadableTask was unsafe because SecurityUtilities.SignFile() transitively depended on process-global state: 1. GetPathToTool() used Directory.GetCurrentDirectory() as fallback for signtool.exe discovery — meaningless in multithreaded mode where each task has its own ProjectDirectory. 2. SignPEFileInternal() created ProcessStartInfo without setting WorkingDirectory — inheriting the process CWD. 3. SetDllDirectoryW() during manifest signing on .NET Core modified process-wide DLL search path — concurrent tasks would race. Fix: instead of duplicating methods, refactor existing ones to accept a projectDirectory parameter (null = use process CWD for back-compat): - GetPathToTool(resources) delegates to GetPathToTool(resources, fallbackDir) - SignPEFileInternal uses fallbackDir ?? Directory.GetCurrentDirectory() for both tool discovery and ProcessStartInfo.WorkingDirectory - SetDllDirectoryW calls wrapped in lock(s_manifestSigningLock) - SignFile task passes TaskEnvironment.ProjectDirectory.Value - No default parameter values anywhere — all calls are explicit Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Context
SignFile task needed migration to support MSBuild's multithreaded execution model. Tasks must use
TaskEnvironmentfor path resolution instead of relying on process working directory.Changes Made
[MSBuildMultiThreadableTask]attribute and implementIMultiThreadableTaskTaskEnvironmentpropertySigningTarget.ItemSpecviaTaskEnvironment.GetAbsolutePath()before passing toSecurityUtilities.SignFile()ArgumentExceptionfrom path resolution with appropriate error loggingThread-safety fix for
SecurityUtilitiesSecurityUtilities.SignFile()transitively used process-global state, which is unsafe under multithreaded execution:GetPathToTool()usedDirectory.GetCurrentDirectory()as fallback for signtool.exe discoverySignPEFileInternal()creatednew ProcessStartInfo()without settingWorkingDirectory, inheriting process CWDSetDllDirectoryW()modified process-wide DLL search path during manifest signingFix: Thread
TaskEnvironmentthrough the entireSecurityUtilities.SignFile→SignFileInternal→SignPEFile→SignPEFileInternalcall chain via internal overloads. Public API signatures are unchanged (old methods delegate withtaskEnvironment: null).SignPEFileInternalnow usestaskEnvironment.GetProcessStartInfo()as baselineProcessStartInfo(inherits correctWorkingDirectoryand environment variables from the task's project context)GetPathToToolaccepts afallbackDirectoryparameter instead of callingDirectory.GetCurrentDirectory()SetDllDirectoryWcalls are serialized with a lock under#if RUNTIME_TYPE_NETCORETaskEnvironmentHelper.CreateMultithreadedForTest()helper for testsTesting
GetPathToTool_WithFallbackDirectory_NeverUsesCwd— verifies fallback directory is never the process CWDSignFile_WithTaskEnvironment_UsesTaskEnvironmentProjectDirectory— verifiesMultiThreadedTaskEnvironmentDriversets correctWorkingDirectoryonProcessStartInfoSignFile_Successtests unchangedNotes
SignFile is Windows-only (
[SupportedOSPlatform("windows")]), so unit tests are skipped on Linux CI runners.