Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'ProcessStartInfo.EnvironmentVariables ["FOO"] = null' launches with empty environment variable #34446

Closed
rolfbjarne opened this issue Apr 2, 2020 · 4 comments · Fixed by #103551
Labels
area-System.Diagnostics.Process in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@rolfbjarne
Copy link
Member

Repro:

class Program
{
    static void Main(string[] args)
    {
        var process = new System.Diagnostics.Process();
        process.StartInfo.FileName = "env";
        process.StartInfo.UseShellExecute = false;
        process.StartInfo.EnvironmentVariables ["FOO"] = null;
        process.Start();
        process.WaitForExit ();
    }
}

Execute:

$ dotnet run | grep FOO
FOO=

Try with mono:

csc *.cs /nologo && mono *.exe | grep FOO
[no output]

Allowing setting an environment variable to null to remove it would match the behavior of the Environment class, where you're supposed to Environment.SetEnvironmentVariable ("FOO", null); to delete that variable.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Diagnostics.Process untriaged New issue has not been triaged by the area owner labels Apr 2, 2020
@akoeplinger
Copy link
Member

/cc @stephentoub

rolfbjarne added a commit to xamarin/xamarin-macios that referenced this issue Apr 2, 2020
There is a difference between mono and .netcore with regards to environment
variables when launching processes.

For the following example:

    process.StartInfo.EnvironmentVariables ["FOO"] = null;

.netcore will launch the process with an empty FOO variable, while mono will
launch the process with no FOO variable set.

So unstead remove the variable from the collection of environment variables
(this works fine even if the collection doesn't contain the variable).

Ref: dotnet/runtime#34446
@tmds
Copy link
Member

tmds commented Apr 2, 2020

This is the result of:

On Windows it is implemented as:

for (int i = 0; i < sd.Count; ++i)
{
stringBuff.Append(keys[i]);
stringBuff.Append('=');
stringBuff.Append(sd[keys[i]]);
stringBuff.Append('\0');
}

So on Windows, it has a similar behavior.

If you use EnvironmentVariables.Remove("FOO") you can get rid of the environment variable for the child.

Allowing setting an environment variable to null to remove it would match the behavior of the Environment class, where you're supposed to Environment.SetEnvironmentVariable ("FOO", null); to delete that variable.

The consistency would be nice. It would also be a breaking change.
Note that Environment doesn't have a separate method for Remove.

@tamlin-mike
Copy link

It would also be a breaking change.

Would it really? If you in Windows execute set FOO= in a shell, FOO gets unset. I'm not aware of any instance where it would ever be requested, or even possible, to neither have nor spawn a child process with an environment variable with no value at all. In fact, even the documentation for SetEnvironmentVariable states about the value "If this parameter is NULL, the variable is deleted from the current process's environment."

To my understanding, that establishes it's in fact impossible, using the Win32 API, to have an environment variable without a value, and I'm willing to go out on a limb and guess the POSIX spec says the same thing.

If extending your quoted code to include the leading comment, we see even there that a value is expected:

// create a list of null terminated "key=val" strings

So it seems that however we view it, from logic, from customer complaint, from Win32 and even from the documented intention of the source code in question itself, and likely also from POSIX and any concievable shell on any platform, past, present or future, this is a bug.

It seems odd to oppose fixing such a bug, using unsubstantiated FUD as the only argument.

If the claim could be substantiated my position about fixing this bug would change, but I honestly don't think you can. I don't think anyone, anywehere, could be so... sick, as to depends on such a logic-defying behaviour, unless they were using it for malware.

@tmds
Copy link
Member

tmds commented Apr 2, 2020

@tamlin-mike both Windows and Linux can have environment variables which are set to an empty string.

It's important to identify breaking changes even when the suggested behavior is meaningful.

rolfbjarne added a commit to xamarin/xamarin-macios that referenced this issue Apr 2, 2020
There is a difference between mono and .netcore with regards to environment
variables when launching processes.

For the following example:

    process.StartInfo.EnvironmentVariables ["FOO"] = null;

.netcore will launch the process with an empty FOO variable, while mono will
launch the process with no FOO variable set.

So unstead remove the variable from the collection of environment variables
(this works fine even if the collection doesn't contain the variable).

Ref: dotnet/runtime#34446
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Apr 8, 2020
There is a difference between mono and .netcore with regards to environment
variables when launching processes.

For the following example:

    process.StartInfo.EnvironmentVariables ["FOO"] = null;

.netcore will launch the process with an empty FOO variable, while mono will
launch the process with no FOO variable set.

So unstead remove the variable from the collection of environment variables
(this works fine even if the collection doesn't contain the variable).

Ref: dotnet/runtime#34446
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Apr 8, 2020
There is a difference between mono and .netcore with regards to environment
variables when launching processes.

For the following example:

    process.StartInfo.EnvironmentVariables ["FOO"] = null;

.netcore will launch the process with an empty FOO variable, while mono will
launch the process with no FOO variable set.

So unstead remove the variable from the collection of environment variables
(this works fine even if the collection doesn't contain the variable).

Ref: dotnet/runtime#34446
rolfbjarne added a commit to xamarin/xamarin-macios that referenced this issue Apr 8, 2020
)

There is a difference between mono and .netcore with regards to environment
variables when launching processes.

For the following example:

    process.StartInfo.EnvironmentVariables ["FOO"] = null;

.netcore will launch the process with an empty FOO variable, while mono will
launch the process with no FOO variable set.

So unstead remove the variable from the collection of environment variables
(this works fine even if the collection doesn't contain the variable).

Ref: dotnet/runtime#34446
@adamsitnik adamsitnik added this to the Future milestone Jun 25, 2020
@adamsitnik adamsitnik removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2020
mandel-macaque pushed a commit to xamarin/xamarin-macios that referenced this issue Oct 12, 2020
)

There is a difference between mono and .netcore with regards to environment
variables when launching processes.

For the following example:

    process.StartInfo.EnvironmentVariables ["FOO"] = null;

.netcore will launch the process with an empty FOO variable, while mono will
launch the process with no FOO variable set.

So unstead remove the variable from the collection of environment variables
(this works fine even if the collection doesn't contain the variable).

Ref: dotnet/runtime#34446
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jun 17, 2024
@jkotas jkotas closed this as completed in 2656730 Jun 18, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Diagnostics.Process in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants