Environment.GetEnvironmentVariable doesn't respect changes made with setenv

[stephentoub]

Whereas getenv will return a value previously set with setenv, Environment.GetEnvironmentVariable does not, instead pulling from a cached palEnvironment in the PAL layer. As such, if a process P/Invokes to setenv to change the value of an environment variable, a subsequent call to Environment.GetEnvironmentVariable will not see the change (it will see a change made with Environment.SetEnvironmentVariable).

[ellismg]

@jkotas Do you know what the history behind this decision was?

[jkotas]

From looking at the code, I suspect that the cached environment was attempt to fix thread safety or consistency problems between Environment.GetEnvironmentVariables and Environment.SetEnvironmentVariable.

The enumeration of the environment starts by reading the environ global variable, without any locks. Consider what may happen if somebody calls setenv while the enumeration is in progress.

[jkotas]

Some flavors of libc documentation explicitly mention this problem. From http://pubs.opengroup.org/onlinepubs/009695399/functions/setenv.html :

Conforming multi-threaded applications shall not use the environ variable to access or modify any environment variable while any other thread is concurrently modifying any environment variable. A call to any function dependent on any environment variable shall be considered a use of the environ variable to access that environment variable.

[jkotas]

IMHO, it may be best to deal with all this by throwing PlatformNotSupportedException from Environment.SetEnvironment on Unix.

[ellismg]

I don't believe that's a viable path forward. DNX runtime code already does this (perhaps that could be removed in favor of some other communication mechanism).

How bad would it be to just surround read and write with a mutex? Obviously this could be racy in the case where the CLR and some third party native library are partying on the environment block at the same time, but I worry if we go down this path then folks are just going to start pinvoking to getenv and setenv directly, and it's not clear that's what we want.

[jkotas]

Frameworks implemented by us need to be secure and reliable - no crashes on races, no potential use after free bugs, no memory leaks, ... . I do not think that secure and reliable implementation of process environment updates is possible on standard Unix. Wrapping our calls with a lock does not help because there are guaranteed to be other calls from random libraries not protected by this lock.

If throwing PlatformNotSupportedException from SetEnvironmentVariable is a problem, it should be ok to keep the current behavior where PAL maintains a private updateable copy of the environment, but clean it up to make it really private - remove the code that is trying to publish back into process environment after update because of it has memory leak, and makes fragile assumptions about libc implementation.

[stephentoub]

I believe with dotnet/coreclr#3140 we've reaffirmed this is by design.

[k15tfu]

@jkotas Hi again! I have a similar problem: The profiler modifies the environment variable at startup to control the way the child processes' profilers work. For example, it can be used to make profiling of child processes optional. Unfortunately coreclr does EnvironInitialize() before profiler startup, so I cannot unset environment variables. Can you suggest how to deal with that?

I see a few options:

1. call EnvironInitialize after profiler startup to give him a chance to modify them
2. allow to modify environment variables cache inside coreclr, for example by calling EnvironPutenv / SetEnvironmentVariableW

[jkotas]

You can inject a call to Environment.SetEnvironmentVariable; or make the profiler for child processes to be no-op. It may be best to open separate discussion issue for this topic.