From 26567303fc4cb7068de49f78e36a17b53c548275 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 18 Jun 2024 13:38:31 -0700 Subject: [PATCH] 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 --- .../src/System/Diagnostics/Process.Unix.cs | 10 ++++- .../src/System/Diagnostics/Process.Windows.cs | 8 +++- .../tests/ProcessStartInfoTests.cs | 27 +++++++++++++ .../src/System/Environment.Variables.Unix.cs | 2 +- .../src/System/Environment.cs | 12 ++---- .../Environment.SetEnvironmentVariable.cs | 38 +++++++++++++------ 6 files changed, 72 insertions(+), 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 index 16d5ea52e01c9..2dde9eb778cdc 100644 --- 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 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 index c8096a1394c70..4407fc81e821e 100644 --- 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 index c2458a9691101..3992857ce574b 100644 --- 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 index 539264e8e30b3..4824cbc998831 100644 --- 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 index ff58e43d54eb3..aaefefd0d22d4 100644 --- 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/src/libraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/Environment.SetEnvironmentVariable.cs b/src/libraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/Environment.SetEnvironmentVariable.cs index 3ed66a152738d..9f2f86dc7031f 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/Environment.SetEnvironmentVariable.cs +++ b/src/libraries/System.Runtime/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); } } }