Prevent tasks from modifying immutable environment variables in MT mode by AR-May · Pull Request #13273 · dotnet/msbuild

AR-May · 2026-02-20T14:18:56Z

Context

Some MSBuild tasks implicitly rely on environment variables remaining immutable across multiple projects builds and cache this state in-process. We would like to disallow to modify certain set of environment variables in the tasks. We can easily check that in the multithreaded mode through task environment. This will help use easier enlighten msbuild tasks that depend on immutable state and cache this state in process. These checks would work only in multithreaded mode.

Changes Made

In multithreaded mode, validate environment variable assignments and disallow modifications to variables marked as immutable
Centralize and refactor environment variable constants to avoid code duplication
added escape hatch to opt-out from the behavior.

Testing

unit tests + manual tests (in process)

AR-May · 2026-02-23T11:04:11Z

src/Framework/FileUtilities.cs

+        /// The string comparison to use for environment variable name comparisons, based on OS environment variable handling.
+        /// Windows: case-insensitive, Unix/Linux: case-sensitive.
+        /// </summary>
+        internal static readonly StringComparison EnvironmentVariableComparison = NativeMethods.IsWindows ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;


Note that environment variable comparisons can be case-sensitive or insensitive independently from the file system’s case sensitivity.

Copilot

Pull request overview

This PR introduces environment variable immutability validation for multithreaded builds to prevent tasks from modifying variables that other concurrent project builds depend on. The implementation centralizes environment variable constants and adds runtime validation that throws when tasks attempt to modify MSBUILD-prefixed or framework-discovery-related variables.

Changes:

Added EnvironmentVariableClassifier to determine which environment variables are immutable (MSBUILD-prefixed variables and specific framework discovery variables)
Refactored environment variable name constants into centralized EnvironmentVariablesNames class in Framework\Constants.cs
Added validation in MultiThreadedTaskEnvironmentDriver to enforce immutability constraints when tasks modify environment variables

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/Framework/EnvironmentVariableClassifier.cs	New classifier to determine if environment variables are immutable based on prefixes and specific names
src/Framework/Constants.cs	Added EnvironmentVariablesNames class with centralized constants, moved DotnetHostPath from Constants class
src/Framework/FileUtilities.cs	Added platform-aware EnvironmentVariableComparer/Comparison utilities for consistent env var name handling
src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs	Added validation logic to prevent modification of immutable environment variables
src/Tasks/Al.cs, SGen.cs, RoslynCodeTaskFactory/RoslynCodeTaskFactoryCompilers.cs	Refactored to use EnvironmentVariablesNames constants instead of hardcoded strings
src/Shared/FrameworkLocationHelper.cs	Refactored to use EnvironmentVariablesNames constants
src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs, Evaluation/Evaluator.cs, BuildManager/EnvironmentVariableValidator.cs	Updated to use EnvironmentVariablesNames.DotnetHostPath
src/Build/Resources/Strings.resx + xlf files	Added error message for immutable variable modification attempts
src/Framework.UnitTests/EnvironmentVariableClassifier_Tests.cs	Unit tests for classifier with case-sensitivity behavior verification
src/Build.UnitTests/BackEnd/TaskEnvironment_Tests.cs	Tests for immutable variable validation in multithreaded mode
src/MSBuild.UnitTests/MSBuildMultithreaded_Tests.cs, Build.UnitTests/BackEnd/BuildManager_Tests.cs, Evaluation/Evaluator_Tests.cs, Tasks.UnitTests/RoslynCodeTaskFactory_Tests.cs, UnitTests.Shared/RunnerUtilities.cs	Test updates to use new constants and avoid MSBUILD-prefixed test variables

Copilot · 2026-02-23T11:12:14Z

src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs

+            // Check for variables being removed (exist in current but not in new environment)
+            foreach (string currentVar in _environmentVariables.Keys)
+            {
+                if (!newEnvironment.ContainsKey(currentVar))
+                {
+                    EnsureVariableCanBeModified(currentVar);
+                }
+            }


The SetEnvironment method has a critical bug in handling case-sensitivity when checking for removed variables. The code calls newEnvironment.ContainsKey(currentVar), but this check uses the comparer of the newEnvironment dictionary, which may differ from the _environmentVariables dictionary's comparer.

For example:

_environmentVariables uses CommunicationsUtilities.EnvironmentVariableComparer (OrdinalIgnoreCase)

If newEnvironment is passed with a case-sensitive comparer and contains "msbuildtest"

But _environmentVariables contains "MSBUILDTEST"

Then newEnvironment.ContainsKey("MSBUILDTEST") returns false (case mismatch)

This incorrectly triggers the "variable removed" validation even though the variable is present

The fix is to use _environmentVariables.Comparer.Equals(currentVar, key) when checking if a key exists in newEnvironment, or to ensure both dictionaries use compatible comparers.

Copilot · 2026-02-23T11:12:15Z

src/Framework/EnvironmentVariableClassifier.cs

+                EnvironmentVariablesNames.ProgramW6432
+            ], FrameworkFileUtilities.EnvironmentVariableComparer);
+
+            // On case-sensitive systems, both "MSBUILD" and "MSBuild" prefixes are used


The comment "On case-sensitive systems, both 'MSBUILD' and 'MSBuild' prefixes are used" is misleading. The HashSet deduplication happens on all systems, but has different results:

On Windows (case-insensitive comparer): HashSet keeps only one entry since "MSBUILD" and "MSBuild" are considered equal

On Unix/Linux (case-sensitive comparer): HashSet keeps both entries since they are distinct

The comment implies this behavior is specific to case-sensitive systems, when in fact the deduplication logic works on all systems. Consider revising to: "The HashSet deduplication ensures both 'MSBUILD' and 'MSBuild' prefixes are available on case-sensitive systems, while on case-insensitive systems they are treated as equivalent."

Suggested change

// On case-sensitive systems, both "MSBUILD" and "MSBuild" prefixes are used

// The HashSet deduplication ensures both "MSBUILD" and "MSBuild" prefixes are available on case-sensitive systems,

// while on case-insensitive systems they are treated as equivalent.

src/Framework.UnitTests/EnvironmentVariableClassifier_Tests.cs

src/Framework/Constants.cs

JanProvaznik · 2026-02-24T11:28:43Z

src/Framework/EnvironmentVariableClassifier.cs

+        /// </summary>
+        private EnvironmentVariableClassifier()
+        {
+            _immutableVariables = FrozenSet.ToFrozenSet([


why just some of the ones that are in the class?

I guess it was answered in #13273 (comment) comment

This is the place that solely define the set of immutable variables.

how to decide which ones are immutable and which ones mutable?

e.g. are and should the DOTNET_* be mutable?

I for time being added ones for which i am sure that they should be immutable. This set might and should extend. For adding DOTNET we would need to make sure there is no task that legitimately sets it in SDK repo or any other dotnet related repos.

can you write that up as an issue?

This is a case where we should consider being as restrictive as we can be in v1, since we can't change it later.

JanProvaznik · 2026-02-24T11:30:02Z

src/Framework/FileUtilities.cs

+        /// The string comparison to use for environment variable name comparisons, based on OS environment variable handling.
+        /// Windows: case-insensitive, Unix/Linux: case-sensitive.
+        /// </summary>
+        internal static readonly StringComparison EnvironmentVariableComparison = NativeMethods.IsWindows ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;


this is just moving from shared fileutilities to framework?

JanProvaznik · 2026-02-24T11:30:15Z

src/Framework/Traits.cs

+        /// in multithreaded build mode. This escape hatch should only be used for compatibility with tasks that need to
+        /// modify these variables and understand the risks of doing so in a concurrent build environment.
+        /// </summary>
+        public readonly bool DisableImmutableEnvironmentVariableCheck = Environment.GetEnvironmentVariable("MSBUILDDISABLEIMMUTABLEENVIRONMENTVARIABLECHECK") == "1";


AR-May · 2026-03-03T16:08:23Z

@rainersigwald and I synced, the question was raised whether we actually need these checks.

In multi‑process MSBuild, if a task mutates environment variables that the build engine or static cache relies on, the change can have different effects:

It may have no immediate effect if the value was already captured (e.g., in Traits) or if static caches were initialized earlier using the original value.
It may change build behavior if the engine consults the variable after the mutation or some static caches were initialized later. Which is undesirable, as it can lead to build inconsistencies.
Any subsequently created process would inherit the modified environment, so the change could affect build behavior there regardless.

In multi-threaded MSBuild, if a task changes environment variables via the TaskEnvironment API, it will not affect the build engine’s behavior - the engine doesn’t use TaskEnvironment nor the static caches, as the process environment variables always would be used to populate those.
If the task causes a new process to be spawned, that child process would use the modified environment. So, engine protection from the env variable that.

So, there is a behavioral difference between the two modes, and it is inevitable. We cannot, and should not, make the build engine rely on the environment variables set by tasks, since different threads could set different values. The old behavior is inherently timing‑dependent and potentially inconsistent, depending on when projects or tasks run.

The only real benefit I see from these checks is debuggability. If a solution relies on this inconsistent behavior (e.g., setting an environment variable and expecting the engine or a static cache to observe it), then with /mt the variable would be silently ignored rather than triggering an error, making such issues harder to detect.

AR-May · 2026-03-04T16:46:26Z

Ok, discussed on the sync, we would like to warn the customers that by modifying the environment variable they are relying on the inconsistent behavior (that their env variable is set before the static caches are populated). Main points:

It should be a warning in the task, not error/exception. Only in mt mode.
In the warning include a link to msbuild markdown document that describes differences in behavior and why multiprocess mode was unreliable if some task sets those variables.
Document that in case of changes on the machine (like installing a new framework) the processes need to restart to recognize the new locations.
Also, there should be examples how to properly set a prohibited environment variable for using in a child process (using the ProcessStartInfo and TaskEnvironment API)
DOTNET_HOST_PATH should be prohibited variable.
We should be able to populate the set of prohibited names later, as we are unlikely to find all of them at once right away.

### Context Enlighten` GetFrameworkPath` and `GetFrameworkSdkPath` tasks. ### Changes Made - `GetFrameworkPath` and `GetFrameworkSdkPath` depend on the `ToolLocationHelper` and `FrameworkLocationHelper` that uses environment variables to compute the locations of tools and frameworks. They then store it in process-wide cache. GetFrameworkPath should be safe as used environment variables are not supposed to be modified during the build by engine or tasks. To be on the safe side, merge after #13273 that ensures that those variables does not change. - added locks to `GetFrameworkSdkPath` to make it thread-safe. ### Testing Unit tests ### Notes merge after #13273 We also assume that environment variables that supposed to contain path to objects, like "COMPLUS_INSTALLROOT" is absolute. This was an implicit assumption prior this change as well. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

JanProvaznik · 2026-03-19T11:07:01Z

@AR-May do I get the discussion correctly that this should be reworked? Please draft or close the pr in that case

AR-May added 8 commits February 20, 2026 13:44

v1

b337b70

deduplicate env var names

e399fdb

Improve the classifier

3fff389

add tests

7b95417

Use localizable strings for the user-facing text

f55ebda

fix failing tests

3ac15d3

fix Task Environment tests

7c2493b

Account for various casing of msbuild prefix.

05aca89

AR-May mentioned this pull request Feb 23, 2026

Enlighten GetFrameworkPath and GetFrameworkSdkPath. #13282

Merged

AR-May added 2 commits February 23, 2026 11:43

fix bug and add tests

0023da9

Update comments

d1c6828

AR-May commented Feb 23, 2026

View reviewed changes

AR-May marked this pull request as ready for review February 23, 2026 11:04

Copilot AI review requested due to automatic review settings February 23, 2026 11:04

Copilot started reviewing on behalf of AR-May February 23, 2026 11:05 View session

Copilot AI reviewed Feb 23, 2026

View reviewed changes

Add escape hatch.

e231f0f

AR-May self-assigned this Feb 23, 2026

AR-May force-pushed the protect-env-vars branch from 51fd1f1 to 9a496bd Compare February 24, 2026 11:29

JanProvaznik requested changes Feb 24, 2026

View reviewed changes

Add docs

0d46746

AR-May force-pushed the protect-env-vars branch from 9a496bd to 0d46746 Compare February 24, 2026 11:32

JanProvaznik approved these changes Feb 26, 2026

View reviewed changes

AR-May marked this pull request as draft March 19, 2026 13:57

	// On case-sensitive systems, both "MSBUILD" and "MSBuild" prefixes are used
	// The HashSet deduplication ensures both "MSBUILD" and "MSBuild" prefixes are available on case-sensitive systems,
	// while on case-insensitive systems they are treated as equivalent.

Conversation

AR-May commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AR-May commented Mar 3, 2026

Uh oh!

AR-May commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JanProvaznik commented Mar 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AR-May commented Feb 20, 2026 •

edited

Loading

AR-May commented Mar 4, 2026 •

edited

Loading