Memory leak in Environment.SetEnvironmentVariable on Unix

[stephentoub]

Repro:

using System;

class Program
{
    static void Main()
    {
        while (true)
        {
            Environment.SetEnvironmentVariable("testvar", "hello world"); // add it
            Environment.SetEnvironmentVariable("testvar", null);  // remove it
        }
    }
}

The problem appears to stem from here:
https://github.com/dotnet/coreclr/blob/2cf141dce5c3a22efd148e88f8a732835ee68336/src/pal/src/cruntime/misc.cpp#L687-L690

[adityamandaleeka]

Looking at all of the PAL environment code and reading through the docs for the POSIX environment functions, the code we have in the PAL to deal with the environment seems dubious. It does some potentially dangerous things with the contents of environ. I came up with the following plan to solve this leak and also fix how we interact with the environment in general:

• Read the contents of the environment once on startup and load them into some internal representation of the state of the environment.
• Future calls to GetEnvironmentVariable and SetEnvironmentVariable affect only this internal representation of the environment, and we remove all the code we have that touches environ directly.
  • We’ll ensure that when processes are created from managed code, they’ll inherit the environment values in our internal representation.
• The code in misc.cpp to get an environment variable (MiscGetenv) currently just returns the pointer to the string in the environment block and has a comment that says the returned value should not be modified or freed. Instead, we change it so it always makes a copy of the string and leaves it up to the caller to manage the memory. This will enable us to manage our internal representation however we see fit.
  • As part of this change, we will audit all the places where MiscGetenv is used to ensure there isn’t a hot path that would degrade due to this change.

This plan obviously means that we will not support the case where a user wants to modify the environment from managed code and expects to see those changes reflected in a native library they PInvoke to, or vice versa. However, reading and writing environment variables in a shared library is problematic anyway (without some special consideration) because none of the POSIX environment functions are thread-safe. In addition, users who really want to keep the two versions of the environment in sync will have workarounds like PInvoking set/getenv (handling the threading concerns with their own locks) and then using Environment.Get/SetEnvironmentVariable, or the other way around. On the other hand, if we use the POSIX functions internally, user code can never safely PInvoke them since they won’t have a guarantee that one of the many CLR background threads isn’t using them for something.

[janvorli]

@adityamandaleeka It seems there is a problem with child processes. With your scheme, a child process launched by the managed process won't see the parent process environment changes, while it should inherit them.

[stephentoub]

a child process launched by the managed process won't see the parent process environment changes

At least for processes launched via Process.Start, we already pass the current environment variables down:
https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs#L301
https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs#L233
https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs#L88

[adityamandaleeka]

@janvorli I've opened a PR for this change, and I verified that Environment.SetEnvironmentVariable followed by Process.Start does work as expected and fills in the child process's native environment block properly.

[janvorli]

@adityamandaleeka ok, thanks for verifying it!

[adityamandaleeka]

Fixed with dotnet/coreclr#3140. Closing the issue.