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

Starting process with empty ProcessStartInfo.Environment incorrectly inherits all variables #8766

Closed
jonorossi opened this issue May 18, 2018 · 8 comments · Fixed by #11630

Comments

@jonorossi
Copy link
Contributor

@jonorossi jonorossi commented May 18, 2018

Steps to Reproduce

Simple mcs <file.cs>; mono <file.exe>.

For macOS/Linux:

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

        process.StartInfo.Environment.Clear();
        //process.StartInfo.Environment["DUMMY"] = "";

        process.Start();
    }
}

Windows:

class Program
{
    static void Main(string[] args)
    {
        var process = new System.Diagnostics.Process();
        process.StartInfo.FileName = "cmd";
        process.StartInfo.Arguments = "/c set";
        process.StartInfo.UseShellExecute = false;

        process.StartInfo.Environment.Clear();
        //process.StartInfo.Environment["DUMMY"] = "";

        process.Start();
    }
}

Current Behavior

On Mono 5.x ProcessStartInfo.Environment.Clear() correctly clears the collection, but calling Process.Start() incorrectly inherits the parent process's environment variables. Running the above unmodified code on macOS with Mono 5.x incorrectly outputs all environment variables, if you uncomment the line setting a dummy environment variable the program will output a single environment variable.

As expected the ProcessStartInfo.EnvironmentVariables property also exhibits the same behaviour.

Sidenote: .NET Framework on Windows doesn't completely clear the environment variables (at least in the above example), but it does not inherit everything, it is likely that cmd is just adding the 3 environment variables below before running set, we could create an env clone for Windows but there really isn't any need:

COMSPEC=C:\Windows\SysWOW64\cmd.exe
PATHEXT=.COM;.EXE;.BAT;.CMD;.VBS;.JS;.WS;.MSC
PROMPT=$P$G

Expected Behavior

On Mono versions prior to 5.x (and on .NET Framework 4.6.1 on Windows) clearing the collection clears the collection, and starting the process doesn't inherit the parent's environment variables, the example program unmodified above on macOS should output nothing.

On which platforms did you notice this

[x] macOS
[ ] Linux
[ ] Windows

Version Used:

  • Mono 4.6.2.7 doesn't have the problem
  • Mono 5.4.1.6 has the problem

Possible Fix

The problem appears to be caused by procInfo.envVariables being left null in Process.StartWithCreateProcess when there are no environment variables as envVariables is lazily created rather than being created immediately inside if (startInfo.HaveEnvVars). This was broken by the changes in 31f90b9.

@jonorossi

This comment has been minimized.

Copy link
Contributor Author

@jonorossi jonorossi commented May 18, 2018

I haven't submitted a pull request because I don't know how you'd want a unit test for this.

@jaykrell

This comment has been minimized.

Copy link
Collaborator

@jaykrell jaykrell commented May 21, 2018

Right, it is a fine but existing line between null and empty.

@jonorossi

This comment has been minimized.

Copy link
Contributor Author

@jonorossi jonorossi commented May 21, 2018

Right, it is a fine but existing line between null and empty.

@jaykrell could you please clarify, I don't understand what your comment is referring to.

@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented May 21, 2018

@jonorossi would be nice to include test case but we can help you with that

@jonorossi

This comment has been minimized.

Copy link
Contributor Author

@jonorossi jonorossi commented May 22, 2018

@marek-safar I can submit a unit test. I asked in my comment above (#8766 (comment)) about unit testing because I wasn't sure whether a unit test that ran /usr/bin/env with a redirected stdout was really a unit test. I probably should just look at the existing tests because this is probably already being done. Does the unit test have to run on Windows?

@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented May 22, 2018

It can be unit test but a standalone test would work too and it can for sure be platform specific

@jonorossi

This comment has been minimized.

Copy link
Contributor Author

@jonorossi jonorossi commented Nov 9, 2018

I finally got around to submitting a pull request (#11630) to fix this with unit tests.

akoeplinger added a commit that referenced this issue Nov 15, 2018
…erits all variables (#11644)

This pull request fixes starting a process with an empty environment variable collection in `ProcessStartInfo`. As per the issue it fixes a regression that it used to work but currently an empty environment variable collection works like it was never touched.

I also made a minor fix to remove the `StringBuilder` because a simple `string.Concat` does the job without the `StringBuilder` allocation and overhead.

I've included unit tests. Please let me know if you'd like any changes.

Fixes #8766

Backport of #11630.
akoeplinger added a commit that referenced this issue Nov 15, 2018
…erits all variables (#11645)

This pull request fixes starting a process with an empty environment variable collection in `ProcessStartInfo`. As per the issue it fixes a regression that it used to work but currently an empty environment variable collection works like it was never touched.

I also made a minor fix to remove the `StringBuilder` because a simple `string.Concat` does the job without the `StringBuilder` allocation and overhead.

I've included unit tests. Please let me know if you'd like any changes.

Fixes #8766

Backport of #11630.
akoeplinger added a commit that referenced this issue Nov 15, 2018
…variables (#11630)

This pull request fixes starting a process with an empty environment variable collection in `ProcessStartInfo`. As per the issue it fixes a regression that it used to work but currently an empty environment variable collection works like it was never touched.

I also made a minor fix to remove the `StringBuilder` because a simple `string.Concat` does the job without the `StringBuilder` allocation and overhead.

I've included unit tests. Please let me know if you'd like any changes.

Fixes #8766
@jonorossi

This comment has been minimized.

Copy link
Contributor Author

@jonorossi jonorossi commented Nov 16, 2018

Thanks for merging and even backporting it.

@marek-safar marek-safar added this to the 2018-08 (5.18.xx) milestone Nov 23, 2018
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Dec 6, 2018
Bumps to mono/api-snapshot@b99fc87.
Bumps to mono/bockbuild@5af573e.
Bumps to mono/boringssl@41221b4.
Bumps to mono/corefx@23d0b58.
Bumps to mono/corert@af496fc.
Bumps to mono/linker@7af03ce.
Bumps to mono/NUnitLite@00e259a.
Bumps to mono/reference-assemblies@9325826.
Bumps to mono/roslyn-binaries@249709f.
Bumps to mono/xunit-binaries@bb58347.

	$ git diff --shortstat b63e5378..23f2024a      # mono 
	 1630 files changed, 50926 insertions(+), 92212 deletions(-)

Fixes: mono/mono#6352
Fixes: mono/mono#6947
Fixes: mono/mono#6992
Fixes: mono/mono#7615
Fixes: mono/mono#8340
Fixes: mono/mono#8407
Fixes: mono/mono#8575
Fixes: mono/mono#8627
Fixes: mono/mono#8707
Fixes: mono/mono#8766
Fixes: mono/mono#8848
Fixes: mono/mono#8866
Fixes: mono/mono#8935
Fixes: mono/mono#9010
Fixes: mono/mono#9023
Fixes: mono/mono#9031
Fixes: mono/mono#9033
Fixes: mono/mono#9106
Fixes: mono/mono#9109
Fixes: mono/mono#9155
Fixes: mono/mono#9179
Fixes: mono/mono#9232
Fixes: mono/mono#9234
Fixes: mono/mono#9262
Fixes: mono/mono#9277
Fixes: mono/mono#9292
Fixes: mono/mono#9318
Fixes: mono/mono#9318
Fixes: mono/mono#9332
Fixes: mono/mono#9407
Fixes: mono/mono#9421
Fixes: mono/mono#9505
Fixes: mono/mono#9542
Fixes: mono/mono#9581
Fixes: mono/mono#9623
Fixes: mono/mono#9684
Fixes: mono/mono#9750
Fixes: mono/mono#9753
Fixes: mono/mono#9772
Fixes: mono/mono#9839
Fixes: mono/mono#9869
Fixes: mono/mono#9921
Fixes: mono/mono#9943
Fixes: mono/mono#9947
Fixes: mono/mono#9973
Fixes: mono/mono#9996
Fixes: mono/mono#10000
Fixes: mono/mono#10031
Fixes: mono/mono#10035
Fixes: mono/mono#10227
Fixes: mono/mono#10243
Fixes: mono/mono#10303
Fixes: mono/mono#10448
Fixes: mono/mono#10483
Fixes: mono/mono#10488
Fixes: mono/mono#10863
Fixes: mono/mono#11123
Fixes: mono/mono#11138
Fixes? mono/mono#11146
Fixes: mono/mono#11202
Fixes: mono/mono#11378
Fixes: mono/mono#11479
Fixes: mono/mono#11613
Fixes: #1951
Fixes: xamarin/xamarin-macios#4347
Fixes: xamarin/xamarin-macios#4617
Fixes: xamarin/xamarin-macios#4984
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Apr 24, 2019
Bumps to mono/api-snapshot@ae01378
Bumps to mono/reference-assemblies@e5173a5
Bumps to mono/bockbuild@d30329d
Bumps to mono/boringssl@3d87996
Bumps to mono/corefx@72f7d76
Bumps to mono/corert@1b7d4a1
Bumps to mono/helix-binaries@7e893ea
Bumps to mono/illinker-test-assets@f21ff68
Bumps to mono/linker@13d864e
Bumps to mono/llvm@1aaaaa5 [mono]
Bumps to mono/llvm@2c2cffe [xamarin-android]
Bumps to mono/NUnitLite@0029561
Bumps to mono/roslyn-binaries@0bbc9b4
Bumps to mono/xunit-binaries@8f6e62e

	$ git diff --shortstat 886c4901..e66c7667      # mono
        3597 files changed, 350850 insertions(+), 91128 deletions(-)
	$ git diff --shortstat 349752c464c5fc93b32e7d45825f2890c85c8b7d..2c2cffedf01e0fe266b9aaad2c2563e05b750ff4
	 240 files changed, 18562 insertions(+), 6581 deletions(-)

Context: dotnet/coreclr#22046

Fixes: CVE 2018-8292 on macOS
Fixes: http://work.devdiv.io/737323
Fixes: dotnet/corefx#33965
Fixes: dotnet/standard#642
Fixes: mono/mono#6997
Fixes: mono/mono#7326
Fixes: mono/mono#7517
Fixes: mono/mono#7750
Fixes: mono/mono#7859
Fixes: mono/mono#8360
Fixes: mono/mono#8460
Fixes: mono/mono#8766
Fixes: mono/mono#8922
Fixes: mono/mono#9418
Fixes: mono/mono#9507
Fixes: mono/mono#9951
Fixes: mono/mono#10024
Fixes: mono/mono#10030
Fixes: mono/mono#10038
Fixes: mono/mono#10448
Fixes: mono/mono#10735
Fixes: mono/mono#10735
Fixes: mono/mono#10737
Fixes: mono/mono#10743
Fixes: mono/mono#10834
Fixes: mono/mono#10837
Fixes: mono/mono#10838
Fixes: mono/mono#10863
Fixes: mono/mono#10945
Fixes: mono/mono#11020
Fixes: mono/mono#11021
Fixes: mono/mono#11021
Fixes: mono/mono#11049
Fixes: mono/mono#11091
Fixes: mono/mono#11095
Fixes: mono/mono#11123
Fixes: mono/mono#11138
Fixes: mono/mono#11146
Fixes: mono/mono#11202
Fixes: mono/mono#11214
Fixes: mono/mono#11317
Fixes: mono/mono#11326
Fixes: mono/mono#11378
Fixes: mono/mono#11385
Fixes: mono/mono#11478
Fixes: mono/mono#11479
Fixes: mono/mono#11488
Fixes: mono/mono#11489
Fixes: mono/mono#11527
Fixes: mono/mono#11529
Fixes: mono/mono#11596
Fixes: mono/mono#11603
Fixes: mono/mono#11613
Fixes: mono/mono#11623
Fixes: mono/mono#11663
Fixes: mono/mono#11681
Fixes: mono/mono#11684
Fixes: mono/mono#11693
Fixes: mono/mono#11697
Fixes: mono/mono#11779
Fixes: mono/mono#11809
Fixes: mono/mono#11858
Fixes: mono/mono#11895
Fixes: mono/mono#11898
Fixes: mono/mono#11898
Fixes: mono/mono#11965
Fixes: mono/mono#12182
Fixes: mono/mono#12193
Fixes: mono/mono#12218
Fixes: mono/mono#12235
Fixes: mono/mono#12263
Fixes: mono/mono#12307
Fixes: mono/mono#12331
Fixes: mono/mono#12362
Fixes: mono/mono#12374
Fixes: mono/mono#12402
Fixes: mono/mono#12421
Fixes: mono/mono#12461
Fixes: mono/mono#12479
Fixes: mono/mono#12479
Fixes: mono/mono#12552
Fixes: mono/mono#12603
Fixes: mono/mono#12747
Fixes: mono/mono#12831
Fixes: mono/mono#12843
Fixes: mono/mono#12881
Fixes: mono/mono#13030
Fixes: mono/mono#13284
Fixes: mono/mono#13297
Fixes: mono/mono#13455
Fixes: mono/mono#13460
Fixes: mono/mono#13478
Fixes: mono/mono#13479
Fixes: mono/mono#13522
Fixes: mono/mono#13607
Fixes: mono/mono#13610
Fixes: mono/mono#13610
Fixes: mono/mono#13639
Fixes: mono/mono#13672
Fixes: mono/mono#13834
Fixes: mono/mono#13878
Fixes: mono/mono#6352
Fixes: mono/monodevelop#6898
Fixes: xamarin/maccore#1069
Fixes: xamarin/maccore#1407
Fixes: xamarin/maccore#604
Fixes: xamarin/xamarin-macios#4984
Fixes: xamarin/xamarin-macios#5289
Fixes: xamarin/xamarin-macios#5363
Fixes: xamarin/xamarin-macios#5381
Fixes: https://issuetracker.unity3d.com/issues/editor-crashes-with-g-logv-when-entering-play-mode-with-active-flowcanvas-script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.