Skip to content

Commit bfe7a08

Browse files
Copilotstephentoubjkotas
authored
Avoid intermediate managed string and array allocations for envp and argv in Unix process start (#126201)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com> Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com>
1 parent 28be713 commit bfe7a08

2 files changed

Lines changed: 88 additions & 57 deletions

File tree

‎src/libraries/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs‎

Lines changed: 82 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System;
5+
using System.Collections.Generic;
56
using System.ComponentModel;
67
using System.Diagnostics;
78
using System.Runtime.InteropServices;
@@ -13,7 +14,7 @@ internal static partial class Interop
1314
internal static partial class Sys
1415
{
1516
internal static unsafe int ForkAndExecProcess(
16-
string filename, string[] argv, string[] envp, string? cwd,
17+
string filename, string[] argv, IDictionary<string, string?> env, string? cwd,
1718
bool setUser, uint userId, uint groupId, uint[]? groups,
1819
out int lpChildPid, SafeFileHandle? stdinFd, SafeFileHandle? stdoutFd, SafeFileHandle? stderrFd, bool shouldThrow = true)
1920
{
@@ -43,8 +44,8 @@ internal static unsafe int ForkAndExecProcess(
4344
stderrRawFd = stderrFd.DangerousGetHandle().ToInt32();
4445
}
4546

46-
AllocNullTerminatedArray(argv, ref argvPtr);
47-
AllocNullTerminatedArray(envp, ref envpPtr);
47+
AllocArgvArray(argv, ref argvPtr);
48+
AllocEnvpArray(env, ref envpPtr);
4849
fixed (uint* pGroups = groups)
4950
{
5051
result = ForkAndExecProcess(
@@ -56,8 +57,8 @@ internal static unsafe int ForkAndExecProcess(
5657
}
5758
finally
5859
{
59-
FreeArray(envpPtr, envp.Length);
60-
FreeArray(argvPtr, argv.Length);
60+
NativeMemory.Free(envpPtr);
61+
NativeMemory.Free(argvPtr);
6162

6263
if (stdinRefAdded)
6364
stdinFd!.DangerousRelease();
@@ -74,46 +75,96 @@ private static unsafe partial int ForkAndExecProcess(
7475
int setUser, uint userId, uint groupId, uint* groups, int groupsLength,
7576
out int lpChildPid, int stdinFd, int stdoutFd, int stderrFd);
7677

77-
private static unsafe void AllocNullTerminatedArray(string[] arr, ref byte** arrPtr)
78+
/// <summary>
79+
/// Allocates a single native memory block containing both a null-terminated pointer array
80+
/// and the UTF-8 encoded string data for the given array of strings.
81+
/// </summary>
82+
private static unsafe void AllocArgvArray(string[] arr, ref byte** arrPtr)
7883
{
79-
nuint arrLength = (nuint)arr.Length + 1; // +1 is for null termination
80-
81-
// Allocate the unmanaged array to hold each string pointer.
82-
// It needs to have an extra element to null terminate the array.
83-
// Zero the memory so that if any of the individual string allocations fails,
84-
// we can loop through the array to free any that succeeded.
85-
// The last element will remain null.
86-
arrPtr = (byte**)NativeMemory.AllocZeroed(arrLength, (nuint)sizeof(byte*));
87-
88-
// Now copy each string to unmanaged memory referenced from the array.
89-
// We need the data to be an unmanaged, null-terminated array of UTF8-encoded bytes.
90-
for (int i = 0; i < arr.Length; i++)
84+
int count = arr.Length;
85+
86+
// First pass: compute total byte length of all strings.
87+
int dataByteLength = 0;
88+
foreach (string str in arr)
9189
{
92-
string str = arr[i];
90+
dataByteLength = checked(dataByteLength + Encoding.UTF8.GetByteCount(str) + 1); // +1 for null terminator
91+
}
9392

94-
int byteLength = Encoding.UTF8.GetByteCount(str);
95-
arrPtr[i] = (byte*)NativeMemory.Alloc((nuint)byteLength + 1); //+1 for null termination
93+
// Allocate a single block: pointer array (count + 1 for null terminator) followed by string data.
94+
nuint pointersByteLength = checked((nuint)(count + 1) * (nuint)sizeof(byte*));
95+
byte* block = (byte*)NativeMemory.Alloc(checked(pointersByteLength + (nuint)dataByteLength));
96+
arrPtr = (byte**)block;
9697

97-
int bytesWritten = Encoding.UTF8.GetBytes(str, new Span<byte>(arrPtr[i], byteLength));
98-
Debug.Assert(bytesWritten == byteLength);
98+
// Create spans over both portions of the block for bounds-checked access.
99+
byte* dataPtr = block + pointersByteLength;
100+
Span<nint> pointers = new Span<nint>(block, count + 1);
101+
Span<byte> data = new Span<byte>(dataPtr, dataByteLength);
102+
103+
int dataOffset = 0;
104+
for (int i = 0; i < count; i++)
105+
{
106+
pointers[i] = (nint)(dataPtr + dataOffset);
99107

100-
arrPtr[i][bytesWritten] = (byte)'\0'; // null terminate
108+
int bytesWritten = Encoding.UTF8.GetBytes(arr[i], data.Slice(dataOffset));
109+
data[dataOffset + bytesWritten] = (byte)'\0';
110+
dataOffset += bytesWritten + 1;
101111
}
112+
113+
pointers[count] = 0; // null terminator
114+
Debug.Assert(dataOffset == dataByteLength);
102115
}
103116

104-
private static unsafe void FreeArray(byte** arr, int length)
117+
/// <summary>
118+
/// Allocates a single native memory block containing both a null-terminated pointer array
119+
/// and the UTF-8 encoded "key=value\0" data for all non-null entries in the environment dictionary.
120+
/// </summary>
121+
private static unsafe void AllocEnvpArray(IDictionary<string, string?> env, ref byte** arrPtr)
105122
{
106-
if (arr != null)
123+
// First pass: count entries with non-null values and compute total buffer size.
124+
int count = 0;
125+
int dataByteLength = 0;
126+
foreach (KeyValuePair<string, string?> pair in env)
107127
{
108-
// Free each element of the array
109-
for (int i = 0; i < length; i++)
128+
if (pair.Value is not null)
110129
{
111-
NativeMemory.Free(arr[i]);
130+
// Each entry: UTF8(key) + '=' + UTF8(value) + '\0'
131+
dataByteLength = checked(dataByteLength + Encoding.UTF8.GetByteCount(pair.Key) + 1 + Encoding.UTF8.GetByteCount(pair.Value) + 1);
132+
count++;
112133
}
134+
}
135+
136+
// Allocate a single block: pointer array (count + 1 for null terminator) followed by string data.
137+
nuint pointersByteLength = checked((nuint)(count + 1) * (nuint)sizeof(byte*));
138+
byte* block = (byte*)NativeMemory.Alloc(checked(pointersByteLength + (nuint)dataByteLength));
139+
arrPtr = (byte**)block;
113140

114-
// And then the array itself
115-
NativeMemory.Free(arr);
141+
// Create spans over both portions of the block for bounds-checked access.
142+
byte* dataPtr = block + pointersByteLength;
143+
Span<nint> pointers = new Span<nint>(block, count + 1);
144+
Span<byte> data = new Span<byte>(dataPtr, dataByteLength);
145+
146+
// Second pass: encode each key=value pair directly into the buffer.
147+
int entryIndex = 0;
148+
int dataOffset = 0;
149+
foreach (KeyValuePair<string, string?> pair in env)
150+
{
151+
if (pair.Value is not null)
152+
{
153+
pointers[entryIndex] = (nint)(dataPtr + dataOffset);
154+
155+
int keyBytes = Encoding.UTF8.GetBytes(pair.Key, data.Slice(dataOffset));
156+
data[dataOffset + keyBytes] = (byte)'=';
157+
int valueBytes = Encoding.UTF8.GetBytes(pair.Value, data.Slice(dataOffset + keyBytes + 1));
158+
data[dataOffset + keyBytes + 1 + valueBytes] = (byte)'\0';
159+
160+
dataOffset += keyBytes + 1 + valueBytes + 1;
161+
entryIndex++;
162+
}
116163
}
164+
165+
pointers[entryIndex] = 0; // null terminator
166+
Debug.Assert(entryIndex == count);
167+
Debug.Assert(dataOffset == dataByteLength);
117168
}
118169
}
119170
}

‎src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs‎

Lines changed: 6 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,7 @@ private bool StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle,
370370
string? filename;
371371
string[] argv;
372372

373-
string[] envp = CreateEnvp(startInfo);
373+
IDictionary<string, string?> env = startInfo.Environment;
374374
string? cwd = !string.IsNullOrWhiteSpace(startInfo.WorkingDirectory) ? startInfo.WorkingDirectory : null;
375375

376376
bool setCredentials = !string.IsNullOrEmpty(startInfo.UserName);
@@ -412,7 +412,7 @@ private bool StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle,
412412
argv = ParseArgv(startInfo);
413413

414414
isExecuting = ForkAndExecProcess(
415-
startInfo, filename, argv, envp, cwd,
415+
startInfo, filename, argv, env, cwd,
416416
setCredentials, userId, groupId, groups,
417417
stdinHandle, stdoutHandle, stderrHandle, usesTerminal,
418418
throwOnNoExec: false); // return false instead of throwing on ENOEXEC
@@ -425,7 +425,7 @@ private bool StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle,
425425
argv = ParseArgv(startInfo, filename, ignoreArguments: true);
426426

427427
ForkAndExecProcess(
428-
startInfo, filename, argv, envp, cwd,
428+
startInfo, filename, argv, env, cwd,
429429
setCredentials, userId, groupId, groups,
430430
stdinHandle, stdoutHandle, stderrHandle, usesTerminal);
431431
}
@@ -440,7 +440,7 @@ private bool StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle,
440440
}
441441

442442
ForkAndExecProcess(
443-
startInfo, filename, argv, envp, cwd,
443+
startInfo, filename, argv, env, cwd,
444444
setCredentials, userId, groupId, groups,
445445
stdinHandle, stdoutHandle, stderrHandle, usesTerminal);
446446
}
@@ -450,7 +450,7 @@ private bool StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle,
450450

451451
private bool ForkAndExecProcess(
452452
ProcessStartInfo startInfo, string? resolvedFilename, string[] argv,
453-
string[] envp, string? cwd, bool setCredentials, uint userId,
453+
IDictionary<string, string?> env, string? cwd, bool setCredentials, uint userId,
454454
uint groupId, uint[]? groups,
455455
SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle, SafeFileHandle? stderrHandle,
456456
bool usesTerminal, bool throwOnNoExec = true)
@@ -479,7 +479,7 @@ private bool ForkAndExecProcess(
479479
// is used to fork/execve as executing managed code in a forked process is not safe (only
480480
// the calling thread will transfer, thread IDs aren't stable across the fork, etc.)
481481
int errno = Interop.Sys.ForkAndExecProcess(
482-
resolvedFilename, argv, envp, cwd,
482+
resolvedFilename, argv, env, cwd,
483483
setCredentials, userId, groupId, groups,
484484
out childPid, stdinHandle, stdoutHandle, stderrHandle);
485485

@@ -563,26 +563,6 @@ private static string[] ParseArgv(ProcessStartInfo psi, string? resolvedExe = nu
563563
return argvList.ToArray();
564564
}
565565

566-
/// <summary>Converts the environment variables information from a ProcessStartInfo into an envp array.</summary>
567-
/// <param name="psi">The ProcessStartInfo.</param>
568-
/// <returns>The envp array.</returns>
569-
private static string[] CreateEnvp(ProcessStartInfo psi)
570-
{
571-
var envp = new string[psi.Environment.Count];
572-
int index = 0;
573-
foreach (KeyValuePair<string, string?> pair in psi.Environment)
574-
{
575-
// Ignore null values for consistency with Environment.SetEnvironmentVariable
576-
if (pair.Value != null)
577-
{
578-
envp[index++] = pair.Key + "=" + pair.Value;
579-
}
580-
}
581-
// Resize the array in case we skipped some entries
582-
Array.Resize(ref envp, index);
583-
return envp;
584-
}
585-
586566
private static string? ResolveExecutableForShellExecute(string filename, string? workingDirectory)
587567
{
588568
// Determine if filename points to an executable file.

0 commit comments

Comments
 (0)