Skip to content

Avoid encoding unchanged process environments on Unix#130790

Draft
adamsitnik with Copilot wants to merge 18 commits into
mainfrom
copilot/task-9141961-210716005-d2705bb8-de74-4115-b09d-ccd781cd624d
Draft

Avoid encoding unchanged process environments on Unix#130790
adamsitnik with Copilot wants to merge 18 commits into
mainfrom
copilot/task-9141961-210716005-d2705bb8-de74-4115-b09d-ccd781cd624d

Conversation

Copilot AI commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Unix process startup now passes a null environment pointer when ProcessStartInfo.Environment remains untouched, avoiding dictionary creation and UTF-8 encoding.

  • Native process creation resolves null to environ.
  • Explicit ProcessStartInfo environment changes retain existing encoding behavior.
  • Process-wide environment mutations continue using a managed snapshot to preserve semantics.
  • Added coverage for lazy environment inheritance.

Note

This description was generated with GitHub Copilot.

Copilot AI and others added 2 commits July 15, 2026 14:36
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI self-assigned this Jul 15, 2026
Copilot AI review requested due to automatic review settings July 15, 2026 15:01
Copilot AI removed the request for review from Copilot July 15, 2026 15:01
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Environment.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Environment.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Environment.cs
Comment thread src/native/libs/System.Native/pal_process.c Outdated
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot July 15, 2026 16:07
Copilot AI requested a review from adamsitnik July 15, 2026 16:08

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment thread src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs Outdated
Comment thread src/coreclr/pal/src/misc/environ.cpp Outdated
@adamsitnik

Copy link
Copy Markdown
Member

@EgorBot -osx_arm64 -linux_x64

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Diagnostics;

BenchmarkSwitcher.FromAssembly(typeof(Benchmarks).Assembly).Run(args);

[MemoryDiagnoser]
public class Benchmarks
{
    private static ProcessStartInfo s_startProcessStartInfo = new ProcessStartInfo()
    {
        FileName = "whoami", // exists on both Windows and Unix, and has very short output
        RedirectStandardOutput = true // avoid visible output
    };

    private Process? _startedProcess;

    [Benchmark]
    public void Start()
    {
        _startedProcess = Process.Start(s_startProcessStartInfo)!;
    }

    [IterationCleanup(Target = nameof(Start))]
    public void CleanupStart()
    {
        if (_startedProcess != null)
        {
            _startedProcess.WaitForExit();
            _startedProcess.Dispose();
            _startedProcess = null;
        }
    }

    [Benchmark]
    public void StartAndWaitForExit()
    {
        using (Process p = Process.Start(s_startProcessStartInfo)!)
        {
            p.WaitForExit();
        }
    }
}

Comment thread src/native/libs/System.Native/pal_environment.c Outdated
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot July 15, 2026 16:55

Copilot AI commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

@copilot address my feedback

Addressed the remaining test feedback in 73ed42b7; targeted System.Diagnostics.Process tests pass (637 total, 0 failures).

Copilot AI requested a review from adamsitnik July 15, 2026 17:01

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment thread src/coreclr/pal/src/misc/environ.cpp
Comment thread src/libraries/Common/src/Interop/Unix/System.Native/Interop.SetEnv.cs Outdated
Comment thread src/native/libs/System.Native/entrypoints.c
Comment thread src/native/libs/System.Native/pal_environment.c
Comment thread src/native/libs/System.Native/pal_environment.h
Comment thread src/native/libs/System.Native/pal_environment.m
Comment thread src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Unix.cs Outdated
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI removed the request for review from Copilot July 17, 2026 11:44
@dotnet-policy-service dotnet-policy-service Bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jul 17, 2026
Copilot AI requested review from Copilot and removed request for Copilot July 17, 2026 11:48

Copilot AI commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

@copilot address my feedback

Done in 0e176c98 and a819e691.

I addressed the latest feedback on the UnsafeAccessor path and validated the affected tests locally.

Note

This reply was generated by GitHub Copilot.

Copilot AI requested a review from adamsitnik July 17, 2026 11:49
Copilot AI review requested due to automatic review settings July 17, 2026 11:54
@adamsitnik
adamsitnik force-pushed the copilot/task-9141961-210716005-d2705bb8-de74-4115-b09d-ccd781cd624d branch from a819e69 to b5045c1 Compare July 17, 2026 11:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

{
public static partial class Environment
{
internal static bool HasEnvironmentVariablesBeenModified => throw new PlatformNotSupportedException();
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

{
public static partial class Environment
{
private static volatile bool s_hasEnvironmentVariablesBeenModified;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to other reviewers: this logic is part of Environment.Unix.cs rather than Environment.Variables.Unix.cs because, the latter is used only for Mono (or at least not for CLR):

<Compile Include="$(MSBuildThisFileDirectory)System\Environment.Variables.Unix.cs" Condition="'$(FeatureCoreCLR)' != 'true'" />

As it seems that for CLR we compile Environment.Variables.Windows.cs on UNIX!!!

<ItemGroup Condition="'$(TargetsWindows)' == 'true' or '$(FeatureCoreCLR)'=='true'">
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GetEnvironmentVariable.cs">
<Link>Common\Interop\Windows\Kernel32\Interop.GetEnvironmentVariable.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GetEnvironmentStrings.cs">
<Link>Common\Interop\Windows\Kernel32\Interop.GetEnvironmentStrings.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.FreeEnvironmentStrings.cs">
<Link>Common\Interop\Windows\Kernel32\Interop.FreeEnvironmentStrings.cs</Link>
</Compile>
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.SetEnvironmentVariable.cs">
<Link>Common\Interop\Windows\Kernel32\Interop.SetEnvironmentVariable.cs</Link>
</Compile>
<Compile Include="$(MSBuildThisFileDirectory)System\Environment.Variables.Windows.cs" />
</ItemGroup>

@adamsitnik

Copy link
Copy Markdown
Member

@EgorBot -osx_arm64 -linux_x64

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Diagnostics;

BenchmarkSwitcher.FromAssembly(typeof(Benchmarks).Assembly).Run(args);

[MemoryDiagnoser]
public class Benchmarks
{
    private static ProcessStartInfo s_startProcessStartInfo = new ProcessStartInfo()
    {
        FileName = "whoami", // exists on both Windows and Unix, and has very short output
        RedirectStandardOutput = true // avoid visible output
    };

    private Process? _startedProcess;

    [Benchmark]
    public void Start()
    {
        _startedProcess = Process.Start(s_startProcessStartInfo)!;
    }

    [IterationCleanup(Target = nameof(Start))]
    public void CleanupStart()
    {
        if (_startedProcess != null)
        {
            _startedProcess.WaitForExit();
            _startedProcess.Dispose();
            _startedProcess = null;
        }
    }

    [Benchmark]
    public void StartAndWaitForExit()
    {
        using (Process p = Process.Start(s_startProcessStartInfo)!)
        {
            p.WaitForExit();
        }
    }
}

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

adamsitnik and others added 2 commits July 17, 2026 16:58
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@adamsitnik

Copy link
Copy Markdown
Member

@EgorBot -osx_arm64 -linux_x64

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Diagnostics;

BenchmarkSwitcher.FromAssembly(typeof(Benchmarks).Assembly).Run(args);

[MemoryDiagnoser]
public class Benchmarks
{
    private static ProcessStartInfo s_startProcessStartInfo = new ProcessStartInfo()
    {
        FileName = "whoami", // exists on both Windows and Unix, and has very short output
        RedirectStandardOutput = true // avoid visible output
    };

    private Process? _startedProcess;

    [Benchmark]
    public void Start()
    {
        _startedProcess = Process.Start(s_startProcessStartInfo)!;
    }

    [IterationCleanup(Target = nameof(Start))]
    public void CleanupStart()
    {
        if (_startedProcess != null)
        {
            _startedProcess.WaitForExit();
            _startedProcess.Dispose();
            _startedProcess = null;
        }
    }

    [Benchmark]
    public void StartAndWaitForExit()
    {
        using (Process p = Process.Start(s_startProcessStartInfo)!)
        {
            p.WaitForExit();
        }
    }
}

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comment on lines +283 to +288
[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[InlineData("not_null", true)]
[InlineData("not_null", false)]
[InlineData("null", false)]
[InlineData("null", true)]
public void SeesEnvironmentVariableSetBeforeStart(string value, bool modifyParent)
@adamsitnik

Copy link
Copy Markdown
Member

@EgorBot -osx_arm64 -linux_x64

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Engines;
using BenchmarkDotNet.Running;
using Microsoft.Win32.SafeHandles;
using System;
using System.Diagnostics;
using System.IO;
using System.Text;
using System.Threading.Tasks;

BenchmarkSwitcher.FromAssembly(typeof(DefaultEnvVars).Assembly).Run(args);

[MemoryDiagnoser]
[SimpleJob(launchCount:5, warmupCount:1, iterationCount:100, invocationCount: 1), JitTiering(JitTieringMode.Skip)]
public class DefaultEnvVars
{
    private static readonly SafeFileHandle s_nullHandle = File.OpenNullHandle();
    private static ProcessStartInfo s_startProcessStartInfo = new ProcessStartInfo()
    {
        FileName = "whoami", // exists on both Windows and Unix, and has very short output
        StandardInputHandle = s_nullHandle,
        StandardOutputHandle = s_nullHandle,
        StandardErrorHandle = s_nullHandle,
    };

    private Process? _startedProcess;

    [Benchmark]
    public void Start() => _startedProcess = Process.Start(s_startProcessStartInfo)!;

    [IterationCleanup(Target = nameof(Start))]
    public void CleanupStart()
    {
        if (_startedProcess != null)
        {
            _startedProcess.WaitForExit();
            _startedProcess.Dispose();
            _startedProcess = null;
        }
    }

    [Benchmark]
    public int Run() => Process.Run(s_startProcessStartInfo).ExitCode;

    [GlobalCleanup]
    public void Cleanup() => s_nullHandle.Dispose();
}

@adamsitnik

Copy link
Copy Markdown
Member

@jkotas I've converted it to a draft again, as the benchmarks show regressions in some cases and I simply need to run them locally and profile if needed. Please ignore this PR for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Diagnostics.Process linkable-framework Issues associated with delivering a linker friendly framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants