Environment.SetEnvironmentVariable does not support setting empty values

[ladipro]

Description

Environment.SetEnvironmentVariable internally normalizes the value argument from empty string to null:

runtime/src/libraries/System.Private.CoreLib/src/System/Environment.cs

Lines 233 to 237 in 4523fbf

	if (string.IsNullOrEmpty(value) || value[0] == '\0')
	{
	// Explicitly null out value if it's empty
	value = null;
	}

This is preventing the callers of this API to set variables to an empty value, which is otherwise supported on all platforms.

Consider the following code on Windows:

using System;
using System.Runtime.InteropServices;

namespace ConsoleApp
{
    class Program
    {
        [DllImport("kernel32", CharSet = CharSet.Unicode)]
        internal static extern bool SetEnvironmentVariable(string lpName, string? lpValue);

        static void Main(string[] args)
        {
            SetEnvironmentVariable("_MYVAR", "");
            Console.WriteLine(Environment.GetEnvironmentVariables().Contains("_MYVAR"));

            Environment.SetEnvironmentVariable("_MYVAR", "");
            Console.WriteLine(Environment.GetEnvironmentVariables().Contains("_MYVAR"));
        }
    }
}

It prints True False as the P/Invoke works as expected and the BCL call deletes the variable, same as if Environment.SetEnvironmentVariable("_MYVAR", null) was called.

This program is expected to print True True.

Configuration

.NET Core 5.0.201 on Windows 10

Regression?

Not a regression, this behavior was inherited from .NET Framework.

Other information

I am proposing to make a breaking change to the API and remove the "" -> null normalization. If this is not possible, please consider adding a new API that would allow the program to set environment variables to empty values.

Note that ProcessStartupInfo.EnvironmentVariables supports empty values and has actually the opposite problem of interpreting null as "" (#34446).

This issue was found when moving an out-of-proc invocation of a tool to in-process and trying to keep the same logic of passing environment variables (previously via ProcessStartupInfo) to the current process with Environment.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[iSazonov]

We could start with:

namespace System
{
    public static partial class Environment
    {
        public static void SetEnvironmentVariable(string variable, string? value, bool AllowEmptyValue = false)

        public static void SetEnvironmentVariable(string variable, string? value, EnvironmentVariableTarget target, bool AllowEmptyValue = false)
    }
}

[jkotas]

I think that it would be better to just fix the API to do the right thing by default. It is a pretty minor breaking change, not worth it to introduce a new API for it.

[iSazonov]

@danmoseley closed #15660 with "This would be too breaking to change now, I think." :-)

This API is so popular that it can break a lot. Thinkin g about PowerShell I'm even afraid to imagine how many scripts will break. I checked PowerShell follows the .Net behavior (and cmd.exe too).

[jkotas]

We are only talking about null vs. empty string distinction. I expect that it should be very rare for this distinction to matter.

I would love to see real-world code examples that this change breaks.

[stephentoub]

I don't know if it would end up breaking anything, but here are some real examples of SetEnvironmentVariable explicitly setting the value to empty string:
https://github.com/microsoft/binskim/blob/d9afb65c89a621411efded74c27999281d87867e/src/BinaryParsers/PEBinary/ProgramDatabase/Pdb.cs#L99-L100
https://github.com/godotengine/godot/blob/4b6e9f315739c4400e3c2d27cd72028779bce1e5/modules/mono/editor/GodotTools/GodotTools/Ides/MessagingServer.cs#L358-L359
https://github.com/gigya/microdot-samples/blob/bdb959564872495a67d1b71edbf1a7d26fdad3da/InventoryService/InventoryService.Client/Program.cs#L14
https://github.com/btcpayserver/btcpayserver-docker/blob/4b192beed0455499731a9b544820da1f0656cf5f/docker-compose-generator/src/Program.cs#L89
https://github.com/dotnet/sdk/blob/c3e4414f2a9f319b19ff61a9515dbb89ba2a7f28/src/Assets/TestProjects/DesktopMinusRid/Program.cs#L14-L15
https://github.com/ArduPilot/MissionPlanner/blob/1933bff0ef912b0f14733cd8ffcc921bf8e4e9bc/ExtLibs/GStreamerHud/MainForm.cs#L29
https://github.com/dotnet/jitutils/blob/945715f07ce9c4381b3ab199f2cd52b586a89415/src/superpmi/superpmicollect.cs#L325-L328
https://github.com/pnp/PnP-PowerShell/blob/9ddc21fd15c347216a9a9f1a6c33c5d0291733c6/Commands/Base/DisconnectOnline.cs#L77-L97
https://github.com/microsoft/azure-pipelines-agent/blob/ab5e9e551587178949218db1a398bf0b67e1215c/src/Agent.Worker/ExecutionContext.cs#L556
https://github.com/SonarSource/sonar-scanner-msbuild/blob/0779f4d58eec352021ce36e32fe8df574c78bba8/src/SonarScanner.MSBuild.Shim/SonarScanner.Wrapper.cs#L148
Presumably the distinction would matter if, for example, subsequent code distinguished Environment.GetEnvironmentVariable returning null vs empty string, e.g. by checking it just for null rather than string.IsNullOrEmpty. I've not looked to see if that happens with these specific examples, but there are plenty of places that do perform only a null check to see if a value was set or not.

[iSazonov]

Before the breaking change we could introduce an analyzer to suggest users to use null instead of empty string for removing environment variable. And add the recommendation in docs.

[jkotas]

I went through the @stephentoub list. I found one place that would be broken, the rest is fine or probably fine.

https://github.com/microsoft/binskim/blob/d9afb65c89a621411efded74c27999281d87867e/src/BinaryParsers/PEBinary/ProgramDatabase/Pdb.cs#L99-L100

Ok: https://devdiv.visualstudio.com/DevDiv/_git/VS?path=%2Fsrc%2Fdebugger%2Fsymbollocator%2Fsymbollocator.cpp&_a=contents&version=GBmain (internal link)

https://github.com/godotengine/godot/blob/4b6e9f315739c4400e3c2d27cd72028779bce1e5/modules/mono/editor/GodotTools/GodotTools/Ides/MessagingServer.cs#L358-L359

Ok: https://github.com/godotengine/godot/blob/15bd2bf03f25c7c6217cb625572c6785b9cfbf6b/modules/mono/mono_gd/gd_mono.cpp#L135

https://github.com/gigya/microdot-samples/blob/bdb959564872495a67d1b71edbf1a7d26fdad3da/InventoryService/InventoryService.Client/Program.cs#L14

Broken: https://github.com/gigya/microdot/blob/77ea576344e4ca8377b8fe6b23141f616ef7da88/Gigya.Microdot.Hosting/Environment/EnvironmentVarialbesConfigurationSource.cs#L42

https://github.com/btcpayserver/btcpayserver-docker/blob/4b192beed0455499731a9b544820da1f0656cf5f/docker-compose-generator/src/Program.cs#L89

Ok: https://github.com/btcpayserver/btcpayserver-docker/blob/5ad43e3f60ea6ad6d6f0ae662d60bb7087d67272/docker-compose-generator/docker-fragments/btcpayserver.yml#L7

https://github.com/dotnet/sdk/blob/c3e4414f2a9f319b19ff61a9515dbb89ba2a7f28/src/Assets/TestProjects/DesktopMinusRid/Program.cs#L14-L15

Ok (PATH)

https://github.com/ArduPilot/MissionPlanner/blob/1933bff0ef912b0f14733cd8ffcc921bf8e4e9bc/ExtLibs/GStreamerHud/MainForm.cs#L29

? (probably ok)

https://github.com/dotnet/jitutils/blob/945715f07ce9c4381b3ab199f2cd52b586a89415/src/superpmi/superpmicollect.cs#L325-L328

Ok:

runtime/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp

Line 69 in 1ba22eb

if (dwRetVal != 0)

https://github.com/pnp/PnP-PowerShell/blob/9ddc21fd15c347216a9a9f1a6c33c5d0291733c6/Commands/Base/DisconnectOnline.cs#L77-L97

? (probably ok)

https://github.com/microsoft/azure-pipelines-agent/blob/ab5e9e551587178949218db1a398bf0b67e1215c/src/Agent.Worker/ExecutionContext.cs#L556

Ok https://github.com/microsoft/azure-pipelines-agent/blob/763575e4443b61e2957d60dca9a9e068d29ccd92/src/Agent.Sdk/Knob/EnvironmentKnobSource.cs#L22

https://github.com/SonarSource/sonar-scanner-msbuild/blob/0779f4d58eec352021ce36e32fe8df574c78bba8/src/SonarScanner.MSBuild.Shim/SonarScanner.Wrapper.cs#L148

Ok https://github.com/SonarSource/sonar-scanner-msbuild/blob/0779f4d58eec352021ce36e32fe8df574c78bba8/src/SonarScanner.MSBuild.Shim/SonarScanner.Wrapper.cs#L144

[MisinformedDNA]

but there are plenty of places that do perform only a null check to see if a value was set or not

But then you'd have to ask how many of these "places" assign an empty string, but check for null. The affected pool seems really tiny.

[stephentoub]

But then you'd have to ask how many of these "places" assign an empty string

Some, sure. Others do things like:

static readonly bool s_blahEnabled = Environment.GetEnvironmentVariable("blah") != null;

[mklement0]

Also worth noting that there is currently no P/Invoke workaround on Unix-like platforms, because environment modifications made by native libraries aren't seen by managed callers (and vice versa; see #9529 for background).

It is on Unix that the practice of setting empty-value environment variables is common: in some use cases, environment variables serve as flags based on their existence alone, in which case their value is irrelevant.

POSIX-compatible shells make setting "value-less" (de facto value: the empty string) easy:

# Create 'NO_VALUE_ without a value (with the empty string as the value).
export NO_VALUE=

# Child process-scoped equivalent
NO_VALUE= foo -bar

Due to the current inability to set such variables in .NET, PowerShell too lacks this ability.

With individual variables set deliberately that isn't much of a problem - just use something like $env:NO_VALUE = 'dummy' -
but it presents a serious problem in code that tries to methodically save and restore environment variables (with unknown values), as the following example shows:

# Run from bash, for instance:
NO_VALUE= pwsh -noprofile -c - <<'EOF'
  
  "NO_VALUE exists? " + ('' -eq [Environment]::GetEnvironmentVariable('NO_VALUE'))

  # Save the current value
  $saved = $env:NO_VALUE

  # ... temporarily modify the value

  # Try to restore the previous value.
  # !! With a value-less variable, this REMOVES it.
  $env:NO_VALUE = $aved

  # !! This now reports $false
  "NO_VALUE exists? " + ('' -eq [Environment]::GetEnvironmentVariable('NO_VALUE'))
  
EOF