Support environment variables with empty value (#103551)

* Support environment variables with no value Fixes #50554 * Fix #34446: Ignore null values in ProcessStartInfo.Environment * Add tests
dotnet · Jun 18, 2024 · 2656730 · 2656730
1 parent 01b0a30
commit 2656730
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 25 deletions.
diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
@@ -612,10 +612,16 @@ private static string[] CreateEnvp(ProcessStartInfo psi)
         {
             var envp = new string[psi.Environment.Count];
             int index = 0;
-            foreach (var pair in psi.Environment)
+            foreach (KeyValuePair<string, string?> pair in psi.Environment)
             {
-                envp[index++] = pair.Key + "=" + pair.Value;
+                // Ignore null values for consistency with Environment.SetEnvironmentVariable
+                if (pair.Value != null)
+                {
+                    envp[index++] = pair.Key + "=" + pair.Value;
+                }
             }
+            // Resize the array in case we skipped some entries
+            Array.Resize(ref envp, index);
             return envp;
         }
 

diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
@@ -873,7 +873,13 @@ private static string GetEnvironmentVariablesBlock(DictionaryWrapper sd)
             var result = new StringBuilder(8 * keys.Length);
             foreach (string key in keys)
             {
-                result.Append(key).Append('=').Append(sd[key]).Append('\0');
+                string? value = sd[key];
+
+                // Ignore null values for consistency with Environment.SetEnvironmentVariable
+                if (value != null)
+                {
+                    result.Append(key).Append('=').Append(value).Append('\0');
+                }
             }
 
             return result.ToString();

diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs
@@ -235,6 +235,8 @@ public void TestEnvironmentOfChildProcess()
         {
             const string ExtraEnvVar = "TestEnvironmentOfChildProcess_SpecialStuff";
             Environment.SetEnvironmentVariable(ExtraEnvVar, "\x1234" + Environment.NewLine + "\x5678"); // ensure some Unicode characters and newlines are in the output
+            const string EmptyEnvVar = "TestEnvironmentOfChildProcess_Empty";
+            Environment.SetEnvironmentVariable(EmptyEnvVar, "");
             try
             {
                 // Schedule a process to see what env vars it gets.  Have it write out those variables
@@ -274,6 +276,31 @@ public void TestEnvironmentOfChildProcess()
             finally
             {
                 Environment.SetEnvironmentVariable(ExtraEnvVar, null);
+                Environment.SetEnvironmentVariable(EmptyEnvVar, null);
+            }
+        }
+
+        [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        public void EnvironmentNullValue()
+        {
+            const string NullEnvVar = "TestEnvironmentOfChildProcess_Null";
+            Environment.SetEnvironmentVariable(NullEnvVar, "");
+            try
+            {
+                Process p = CreateProcess(() =>
+                {
+                    // Verify that setting the value to null in StartInfo is going to remove the process environment.
+                    Assert.Null(Environment.GetEnvironmentVariable(NullEnvVar));
+                    return RemoteExecutor.SuccessExitCode;
+                });
+                p.StartInfo.Environment[NullEnvVar] = null;
+                Assert.Null(p.StartInfo.Environment[NullEnvVar]);
+                p.Start();
+                Assert.True(p.WaitForExit(WaitInMS));
+            }
+            finally
+            {
+                Environment.SetEnvironmentVariable(NullEnvVar, null);
             }
         }
 

diff --git a/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Unix.cs
@@ -45,7 +45,7 @@ private static unsafe void SetEnvironmentVariableCore(string variable, string? v
             value = value == null ? null : TrimStringOnFirstZero(value);
             lock (s_environment!)
             {
-                if (string.IsNullOrEmpty(value))
+                if (value == null)
                 {
                     s_environment.Remove(variable);
                 }

diff --git a/src/libraries/System.Private.CoreLib/src/System/Environment.cs b/src/libraries/System.Private.CoreLib/src/System/Environment.cs
@@ -67,7 +67,7 @@ public static IDictionary GetEnvironmentVariables(EnvironmentVariableTarget targ
 
         public static void SetEnvironmentVariable(string variable, string? value)
         {
-            ValidateVariableAndValue(variable, ref value);
+            ValidateVariable(variable);
             SetEnvironmentVariableCore(variable, value);
         }
 
@@ -79,7 +79,7 @@ public static void SetEnvironmentVariable(string variable, string? value, Enviro
                 return;
             }
 
-            ValidateVariableAndValue(variable, ref value);
+            ValidateVariable(variable);
 
             bool fromMachine = ValidateAndConvertRegistryTarget(target);
             SetEnvironmentVariableFromRegistry(variable, value, fromMachine: fromMachine);
@@ -234,7 +234,7 @@ private static bool ValidateAndConvertRegistryTarget(EnvironmentVariableTarget t
             throw new ArgumentOutOfRangeException(nameof(target), target, SR.Format(SR.Arg_EnumIllegalVal, target));
         }
 
-        private static void ValidateVariableAndValue(string variable, ref string? value)
+        private static void ValidateVariable(string variable)
         {
             ArgumentException.ThrowIfNullOrEmpty(variable);
 
@@ -243,12 +243,6 @@ private static void ValidateVariableAndValue(string variable, ref string? value)
 
             if (variable.Contains('='))
                 throw new ArgumentException(SR.Argument_IllegalEnvVarName, nameof(variable));
-
-            if (string.IsNullOrEmpty(value) || value[0] == '\0')
-            {
-                // Explicitly null out value if it's empty
-                value = null;
-            }
         }
     }
 }
diff --git a/...untime/tests/System.Runtime.Extensions.Tests/System/Environment.SetEnvironmentVariable.cs b/...untime/tests/System.Runtime.Extensions.Tests/System/Environment.SetEnvironmentVariable.cs
@@ -251,9 +251,9 @@ public void ModifyEnvironmentVariable_AndEnumerate(EnvironmentVariableTarget tar
 
         [Theory]
         [MemberData(nameof(EnvironmentTests.EnvironmentVariableTargets), MemberType = typeof(EnvironmentTests))]
-        public void DeleteEnvironmentVariable(EnvironmentVariableTarget target)
+        public void EmptyEnvironmentVariable(EnvironmentVariableTarget target)
         {
-            string varName = $"Test_DeleteEnvironmentVariable ({target})";
+            string varName = $"Test_EmptyEnvironmentVariable({target})";
             const string value = "false";
 
             ExecuteAgainstTarget(target,
@@ -262,15 +262,29 @@ public void DeleteEnvironmentVariable(EnvironmentVariableTarget target)
                 // First set the value to something and then ensure that it can be deleted.
                 Environment.SetEnvironmentVariable(varName, value);
                 Environment.SetEnvironmentVariable(varName, string.Empty);
-                Assert.Null(Environment.GetEnvironmentVariable(varName));
+                Assert.Equal(string.Empty, Environment.GetEnvironmentVariable(varName));
 
                 Environment.SetEnvironmentVariable(varName, value);
-                Environment.SetEnvironmentVariable(varName, null);
-                Assert.Null(Environment.GetEnvironmentVariable(varName));
+                Environment.SetEnvironmentVariable(varName, NullString);
+                Assert.Equal(string.Empty, Environment.GetEnvironmentVariable(varName));
+            });
+        }
+
+        [Theory]
+        [MemberData(nameof(EnvironmentTests.EnvironmentVariableTargets), MemberType = typeof(EnvironmentTests))]
+        public void DeleteEnvironmentVariable(EnvironmentVariableTarget target)
+        {
+            string varName = $"Test_DeleteEnvironmentVariable ({target})";
+            const string value = "false";
 
+            ExecuteAgainstTarget(target,
+            () =>
+            {
+                // First set the value to something and then ensure that it can be deleted.
                 Environment.SetEnvironmentVariable(varName, value);
-                Environment.SetEnvironmentVariable(varName, NullString);
+                Environment.SetEnvironmentVariable(varName, null);
                 Assert.Null(Environment.GetEnvironmentVariable(varName));
+                Assert.False(Environment.GetEnvironmentVariables().Contains(varName));
             });
         }
 
@@ -305,11 +319,11 @@ public void DeleteEnvironmentVariableInitialNullInValue()
             try
             {
                 Environment.SetEnvironmentVariable(varName, value);
-                Assert.Null(Environment.GetEnvironmentVariable(varName));
+                Assert.Equal("", Environment.GetEnvironmentVariable(varName));
             }
             finally
             {
-                Environment.SetEnvironmentVariable(varName, string.Empty);
+                Environment.SetEnvironmentVariable(varName, null);
             }
         }
 
@@ -329,8 +343,8 @@ public void NonInitialNullCharacterInVariableName()
             }
             finally
             {
-                Environment.SetEnvironmentVariable(varName, string.Empty);
-                Environment.SetEnvironmentVariable(varNamePrefix, string.Empty);
+                Environment.SetEnvironmentVariable(varName, null);
+                Environment.SetEnvironmentVariable(varNamePrefix, null);
             }
         }
 
@@ -349,7 +363,7 @@ public void NonInitialNullCharacterInValue()
             }
             finally
             {
-                Environment.SetEnvironmentVariable(varName, string.Empty);
+                Environment.SetEnvironmentVariable(varName, null);
             }
         }
 
@@ -363,7 +377,7 @@ public void DeleteNonExistentEnvironmentVariable()
                 Environment.SetEnvironmentVariable(varName, null);
             }
 
-            Environment.SetEnvironmentVariable("TestDeletingNonExistingEnvironmentVariable", string.Empty);
+            Environment.SetEnvironmentVariable("TestDeletingNonExistingEnvironmentVariable", null);
         }
     }
 }