Prevent tasks from modifying immutable environment variables in MT mode

documentation/specs/multithreading/msbuild-task-enlightenment.md

@@ -0,0 +1,26 @@
+# MSBuild Repository Task Enlightenment Guidelines
+
+This document provides specific guidance for enlightening tasks within the MSBuild repository.
+
+## Acceptable Changes in Task Behavior During Enlightenment
+
+**The fundamental principle is to preserve task behavior and outputs for all input options.**
+
+### Permissible Changes
+Modifications to error messaging or failure location are acceptable when a task fails with a different but reasonable error at an alternative execution point, provided that **no significant side effects** (such as disk modifications or output changes) occur between the original and new failure points.
+
+### Changes Requiring Change Waves
+Modifications that alter the success or failure outcome of a task for given inputs require careful evaluation. Such changes are permissible only when implemented behind a **change wave** and when the affected scenarios represent **obscure or edge use cases**.
+
+## Immutable Environment Variables in MSBuild
+
+Certain MSBuild tasks (such as `GetFrameworkPath`) use internal infrastructure that depends on environment variables for resolution. These results are cached in-process for reuse by both tasks and internal MSBuild components. MSBuild assumes that these variables remain constant throughout the build process. 
+
+While multiprocess mode cannot prevent tasks from modifying these variables, multithreaded mode enables MSBuild to enforce protection of environment variables that must remain immutable. Attempts to modify these protected variables will result in an `InvalidOperationException`.
+
+With this protection in place, we will allow MSBuild tasks to continue using internal infrastructure directly without requiring the TaskEnvironment API.
+
+MSBuild protects environment variables in these categories:
+
+1. **Variables with MSBuild-specific prefixes** (e.g. ones used in Traits)
+2. **Framework and SDK location variables** (e.g., `COMPLUS_INSTALL_ROOT`, `COMPLUS_VERSION`, `ReferenceAssemblyRoot`, `ProgramW6432`, etc)

src/Build.UnitTests/BackEnd/BuildManager_Tests.cs

@@ -3491,7 +3491,7 @@ public void WarningsAreTreatedAsErrorsButTargetsStillSucceed()
         [Fact]
         public void DotnetHostPathDirectoryWarning()
         {
-            _env.SetEnvironmentVariable(Constants.DotnetHostPathEnvVarName, Path.GetTempPath());
+            _env.SetEnvironmentVariable(EnvironmentVariablesNames.DotnetHostPath, Path.GetTempPath());
 
             BuildRequestData data = GetBuildRequestData(CleanupFileContents(@"<Project xmlns='msbuildnamespace' ToolsVersion='msbuilddefaulttoolsversion'><Target Name='test'/></Project>"));
             _buildManager.Build(_parameters, data);
@@ -3506,7 +3506,7 @@ public void DotnetHostPathDirectoryWarning()
         public void DotnetHostPathFileNoWarning()
         {
             TransientTestFile tempFile = _env.CreateFile("dotnet.exe", "");
-            _env.SetEnvironmentVariable(Constants.DotnetHostPathEnvVarName, tempFile.Path);
+            _env.SetEnvironmentVariable(EnvironmentVariablesNames.DotnetHostPath, tempFile.Path);
 
             BuildRequestData data = GetBuildRequestData(CleanupFileContents(@"<Project xmlns='msbuildnamespace' ToolsVersion='msbuilddefaulttoolsversion'><Target Name='test'/></Project>"));
             _buildManager.Build(_parameters, data);
@@ -3520,7 +3520,7 @@ public void DotnetHostPathFileNoWarning()
         [Fact]
         public void DotnetHostPathNotSetNoWarning()
         {
-            _env.SetEnvironmentVariable(Constants.DotnetHostPathEnvVarName, null);
+            _env.SetEnvironmentVariable(EnvironmentVariablesNames.DotnetHostPath, null);
 
             BuildRequestData data = GetBuildRequestData(CleanupFileContents(@"<Project xmlns='msbuildnamespace' ToolsVersion='msbuilddefaulttoolsversion'><Target Name='test'/></Project>"));
             _buildManager.Build(_parameters, data);

src/Build.UnitTests/BackEnd/TaskEnvironment_Tests.cs

@@ -17,6 +17,8 @@ public class TaskEnvironment_Tests
     {
         private const string StubEnvironmentName = "Stub";
         private const string MultithreadedEnvironmentName = "Multithreaded";
+        private const string ImmutableTestVar = "MSBUILDTESTVAR";
+        private const string TestValue = "value";
 
         public static TheoryData<string> EnvironmentTypes =>
             new TheoryData<string>
@@ -32,7 +34,13 @@ private static TaskEnvironment CreateTaskEnvironment(string environmentType)
             return environmentType switch
             {
                 StubEnvironmentName => TaskEnvironmentHelper.CreateForTest(),
-                MultithreadedEnvironmentName => new TaskEnvironment(new MultiThreadedTaskEnvironmentDriver(GetResolvedTempPath())),
+                MultithreadedEnvironmentName => new TaskEnvironment(new MultiThreadedTaskEnvironmentDriver(
+                    GetResolvedTempPath(),
+                    new Dictionary<string, string>
+                    {
+                        ["env_var_1"] = "env_var_1_value",
+                        ["env_var_2"] = "env_var_2_value"
+                    })),
                 _ => throw new ArgumentException($"Unknown environment type: {environmentType}")
             };
         }
@@ -86,7 +94,7 @@ private static string GetResolvedTempPath()
         public void TaskEnvironment_SetAndGetEnvironmentVariable_ShouldWork(string environmentType)
         {
             var taskEnvironment = CreateTaskEnvironment(environmentType);
-            string testVarName = $"MSBUILD_TEST_VAR_{environmentType}_{Guid.NewGuid():N}";
+            string testVarName = $"TEST_ENV_VAR_{environmentType}_{Guid.NewGuid():N}";
             string testVarValue = $"test_value_{environmentType}";
 
             try
@@ -112,7 +120,7 @@ public void TaskEnvironment_SetAndGetEnvironmentVariable_ShouldWork(string envir
         public void TaskEnvironment_SetEnvironmentVariableToNull_ShouldRemoveVariable(string environmentType)
         {
             var taskEnvironment = CreateTaskEnvironment(environmentType);
-            string testVarName = $"MSBUILD_REMOVE_TEST_{environmentType}_{Guid.NewGuid():N}";
+            string testVarName = $"TEST_ENV_VAR_REMOVE_{environmentType}_{Guid.NewGuid():N}";
             string testVarValue = "value_to_remove";
 
             try
@@ -138,7 +146,7 @@ public void TaskEnvironment_SetEnvironmentVariableToNull_ShouldRemoveVariable(st
         public void TaskEnvironment_SetEnvironment_ShouldReplaceAllVariables(string environmentType)
         {
             var taskEnvironment = CreateTaskEnvironment(environmentType);
-            string prefix = $"MSBUILD_SET_ENV_TEST_{environmentType}_{Guid.NewGuid():N}";
+            string prefix = $"TEST_ENV_VAR_SET_{environmentType}_{Guid.NewGuid():N}";
             string var1Name = $"{prefix}_VAR1";
             string var2Name = $"{prefix}_VAR2";
             string var3Name = $"{prefix}_VAR3";
@@ -265,7 +273,7 @@ public void TaskEnvironment_GetProcessStartInfo_ShouldConfigureCorrectly(string
         {
             var taskEnvironment = CreateTaskEnvironment(environmentType);
             string testDirectory = GetResolvedTempPath();
-            string testVarName = $"MSBUILD_PROCESS_TEST_{environmentType}_{Guid.NewGuid():N}";
+            string testVarName = $"TEST_ENV_VAR_PROCESS_{environmentType}_{Guid.NewGuid():N}";
             string testVarValue = "process_test_value";
             string originalDirectory = Directory.GetCurrentDirectory();
 
@@ -303,7 +311,7 @@ public void TaskEnvironment_GetProcessStartInfo_ShouldConfigureCorrectly(string
         [Fact]
         public void TaskEnvironment_StubEnvironment_ShouldAffectSystemEnvironment()
         {
-            string testVarName = $"MSBUILD_STUB_ISOLATION_TEST_{Guid.NewGuid():N}";
+            string testVarName = $"TEST_ENV_VAR_STUB_ISOLATION_{Guid.NewGuid():N}";
             string testVarValue = "stub_test_value";
 
             var stubEnvironment = TaskEnvironmentHelper.CreateForTest();
@@ -333,7 +341,7 @@ public void TaskEnvironment_StubEnvironment_ShouldAffectSystemEnvironment()
         [Fact]
         public void TaskEnvironment_MultithreadedEnvironment_ShouldBeIsolatedFromSystem()
         {
-            string testVarName = $"MSBUILD_MULTITHREADED_ISOLATION_TEST_{Guid.NewGuid():N}";
+            string testVarName = $"TEST_ENV_VAR_MULTITHREADED_ISOLATION_{Guid.NewGuid():N}";
             string testVarValue = "multithreaded_test_value";
 
             using var driver = new MultiThreadedTaskEnvironmentDriver(
@@ -384,5 +392,107 @@ public void TaskEnvironment_GetAbsolutePath_WithInvalidPathChars_ShouldNotThrow(
                 DisposeTaskEnvironment(taskEnvironment);
             }
         }
+
+        [Fact]
+        public void MultiThreadedTaskEnvironmentDriver_SetEnvironmentVariable_ThrowsOnCaseChangeOfImmutableValue()
+        {
+            // Changing just the case of an immutable variable's value should throw because
+            // value comparison must be case-sensitive (StringComparison.Ordinal).
+            // If we incorrectly used case-insensitive comparison, this would not throw.
+            using var driver = new MultiThreadedTaskEnvironmentDriver(
+                GetResolvedTempPath(),
+                new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)
+                {
+                    [ImmutableTestVar] = TestValue
+                });
+            var taskEnvironment = new TaskEnvironment(driver);
+
+            // Changing "value" to "VALUE" is a modification (case-sensitive) and should throw
+            Should.Throw<InvalidOperationException>(() =>
+                taskEnvironment.SetEnvironmentVariable(ImmutableTestVar, TestValue.ToUpperInvariant()));
+        }
+
+        [Fact]
+        public void MultiThreadedTaskEnvironmentDriver_SetEnvironmentVariable_ThrowsOnImmutableVariable()
+        {
+            // MSBuild-prefixed variables are immutable and should throw when adding
+            using var driver = new MultiThreadedTaskEnvironmentDriver(
+                GetResolvedTempPath(),
+                new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase));
+            var taskEnvironment = new TaskEnvironment(driver);
+
+            Should.Throw<InvalidOperationException>(() =>
+                taskEnvironment.SetEnvironmentVariable(ImmutableTestVar, TestValue));
+        }
+
+        [Fact]
+        public void MultiThreadedTaskEnvironmentDriver_SetEnvironmentVariable_AllowsSettingSameValue()
+        {
+            // Setting an immutable variable to its current value should not throw
+            using var driver = new MultiThreadedTaskEnvironmentDriver(
+                GetResolvedTempPath(),
+                new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)
+                {
+                    [ImmutableTestVar] = TestValue
+                });
+            var taskEnvironment = new TaskEnvironment(driver);
+
+            Should.NotThrow(() =>
+                taskEnvironment.SetEnvironmentVariable(ImmutableTestVar, TestValue));
+        }
+
+        [Fact]
+        public void MultiThreadedTaskEnvironmentDriver_SetEnvironment_ThrowsOnImmutableVariableRemoval()
+        {
+            // Removing an immutable variable via SetEnvironment should throw
+            using var driver = new MultiThreadedTaskEnvironmentDriver(
+                GetResolvedTempPath(),
+                new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)
+                {
+                    [ImmutableTestVar] = TestValue
+                });
+            var taskEnvironment = new TaskEnvironment(driver);
+
+            // New environment without the immutable variable - this is a removal
+            var newEnvironment = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+
+            Should.Throw<InvalidOperationException>(() =>
+                taskEnvironment.SetEnvironment(newEnvironment));
+        }
+
+        [Fact]
+        public void MultiThreadedTaskEnvironmentDriver_SetEnvironmentVariable_ThrowsOnImmutableVariableRemoval()
+        {
+            // Removing an immutable variable by setting to null should throw
+            using var driver = new MultiThreadedTaskEnvironmentDriver(
+                GetResolvedTempPath(),
+                new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)
+                {
+                    [ImmutableTestVar] = TestValue
+                });
+            var taskEnvironment = new TaskEnvironment(driver);
+
+            Should.Throw<InvalidOperationException>(() =>
+                taskEnvironment.SetEnvironmentVariable(ImmutableTestVar, null));
+        }
+
+        [Fact]
+        public void MultiThreadedTaskEnvironmentDriver_EscapeHatch_DisablesImmutableVariableCheck()
+        {
+            // When the escape hatch is set, modifying immutable variables should not throw
+            using TestEnvironment env = TestEnvironment.Create();
+            env.SetEnvironmentVariable("MSBUILDDISABLEIMMUTABLEENVIRONMENTVARIABLECHECK", "1");
+
+            using var driver = new MultiThreadedTaskEnvironmentDriver(
+                GetResolvedTempPath(),
+                new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase));
+            var taskEnvironment = new TaskEnvironment(driver);
+
+            // With escape hatch enabled, setting an MSBUILD-prefixed variable should not throw
+            Should.NotThrow(() =>
+                taskEnvironment.SetEnvironmentVariable(ImmutableTestVar, TestValue));
+
+            taskEnvironment.GetEnvironmentVariable(ImmutableTestVar).ShouldBe(TestValue);
+        }
     }
 }

src/Build.UnitTests/Evaluation/Evaluator_Tests.cs

@@ -2626,7 +2626,7 @@ public void MSBuildExtensionsPath64Default()
             if (!String.IsNullOrEmpty(programFiles32))
             {
                 // only set in 32-bit windows on 64-bit machines
-                expected = Environment.GetEnvironmentVariable("ProgramW6432");
+                expected = Environment.GetEnvironmentVariable(EnvironmentVariablesNames.ProgramW6432);
 
                 if (string.IsNullOrEmpty(expected))
                 {

src/Build/BackEnd/BuildManager/EnvironmentVariableValidator.cs

@@ -6,7 +6,6 @@
 using Microsoft.Build.Framework;
 using Microsoft.Build.Shared;
 using Microsoft.Build.Shared.FileSystem;
-using Constants = Microsoft.Build.Framework.Constants;
 
 namespace Microsoft.Build.Execution
 {
@@ -32,7 +31,7 @@ internal static void ValidateEnvironmentVariables(ILoggingService loggingService
         /// </summary>
         private static void ValidateDotnetHostPath(ILoggingService loggingService)
         {
-            string? dotnetHostPath = Environment.GetEnvironmentVariable(Constants.DotnetHostPathEnvVarName);
+            string? dotnetHostPath = Environment.GetEnvironmentVariable(EnvironmentVariablesNames.DotnetHostPath);
             if (string.IsNullOrEmpty(dotnetHostPath))
             {
                 return;

src/Build/BackEnd/TaskExecutionHost/MultiThreadedTaskEnvironmentDriver.cs

@@ -7,6 +7,7 @@
 using System.Diagnostics;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Internal;
+using Microsoft.Build.Shared;
 
 namespace Microsoft.Build.BackEnd
 {
@@ -37,6 +38,21 @@ public MultiThreadedTaskEnvironmentDriver(
             ProjectDirectory = new AbsolutePath(currentDirectoryFullPath, ignoreRootedCheck: true);
         }
 
+        /// <summary>
+        /// Validates that the specified environment variable can be modified.
+        /// Throws if the variable is one that MSBuild assumes should remain constant.
+        /// </summary>
+        /// <param name="name">The name of the environment variable to check.</param>
+        /// <exception cref="InvalidOperationException">Thrown when attempting to modify an immutable environment variable.</exception>
+        private static void EnsureVariableCanBeModified(string name)
+        {
+            if (EnvironmentVariableClassifier.Instance.IsImmutable(name))
+            {
+                throw new InvalidOperationException(
+                    ResourceUtilities.FormatResourceStringStripCodeAndKeyword("TaskCannotModifyImmutableEnvironmentVariable", name));
+            }
+        }
+
         /// <summary>
         /// Initializes a new instance of the <see cref="MultiThreadedTaskEnvironmentDriver"/> class
         /// with the specified working directory and environment variables from the current process.
@@ -88,6 +104,16 @@ public IReadOnlyDictionary<string, string> GetEnvironmentVariables()
         /// <inheritdoc/>
         public void SetEnvironmentVariable(string name, string? value)
         {
+            if (!Traits.Instance.EscapeHatches.DisableImmutableEnvironmentVariableCheck)
+            {
+                // Only validate if we're actually changing the value
+                _environmentVariables.TryGetValue(name, out string? currentValue);
+                if (!string.Equals(currentValue, value, StringComparison.Ordinal))
+                {
+                    EnsureVariableCanBeModified(name);
+                }
+            }
+
             if (value == null)
             {
                 _environmentVariables.Remove(name);
@@ -101,6 +127,30 @@ public void SetEnvironmentVariable(string name, string? value)
         /// <inheritdoc/>
         public void SetEnvironment(IDictionary<string, string> newEnvironment)
         {
+            if (!Traits.Instance.EscapeHatches.DisableImmutableEnvironmentVariableCheck)
+            {
+                // 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);
+                    }
+                }
+
+                // Check for variables being added or modified
+                foreach (KeyValuePair<string, string> entry in newEnvironment)
+                {
+                    _environmentVariables.TryGetValue(entry.Key, out string? currentValue);
+
+                    // Only validate if we're actually changing the value
+                    if (!string.Equals(currentValue, entry.Value, StringComparison.Ordinal))
+                    {
+                        EnsureVariableCanBeModified(entry.Key);
+                    }
+                }
+            }
+
             // Simply replace the entire environment dictionary
             _environmentVariables.Clear();
             foreach (KeyValuePair<string, string> entry in newEnvironment)

src/Build/Evaluation/Evaluator.cs

@@ -1904,7 +1904,7 @@ static string EvaluateProperty(string value, IElementLocation location,
                     string dotnetExe = Path.Combine(FileUtilities.GetFolderAbove(sdkResult.Path, 5), Constants.DotnetProcessName);
                     if (FileSystems.Default.FileExists(dotnetExe))
                     {
-                        _data.AddSdkResolvedEnvironmentVariable(Constants.DotnetHostPathEnvVarName, dotnetExe);
+                        _data.AddSdkResolvedEnvironmentVariable(EnvironmentVariablesNames.DotnetHostPath, dotnetExe);
                     }
                 }
 

src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs

@@ -654,7 +654,7 @@ private static TaskHostParameters AddNetHostParamsIfNeeded(
                 return currentParams;
             }
 
-            string dotnetHostPath = getProperty(Constants.DotnetHostPathEnvVarName)?.EvaluatedValue;
+            string dotnetHostPath = getProperty(EnvironmentVariablesNames.DotnetHostPath)?.EvaluatedValue;
             string netCoreSdkRoot = getProperty(Constants.NetCoreSdkRoot)?.EvaluatedValue?.TrimEnd('/', '\\');
 
             // The NetCoreSdkRoot property got added with .NET 11, so for earlier SDKs we fall back to the RID graph path

src/Build/Resources/Strings.resx

@@ -145,6 +145,9 @@
   <data name="WaitingForEndOfBuild" xml:space="preserve">
     <value>The operation cannot be completed because EndBuild has already been called but existing submissions have not yet completed.</value>
   </data>
+  <data name="TaskCannotModifyImmutableEnvironmentVariable" xml:space="preserve">
+    <value>Cannot modify environment variable '{0}': variable is immutable and must remain constant during the build.</value>
+  </data>
   <data name="EnvironmentDerivedPropertyRead">
     <value>Property '{0}' with value '{1}' expanded from the environment.</value>
   </data>