Skip to content

Commit 638b886

Browse files
adamsitnikadamsitnik
committed
address my own feedback:
- reduce code duplication - acquire the locks! - fix Windows tests: enable inheritance - fix Unix implementation: handle "usesTerminal"
1 parent 59234fd commit 638b886

7 files changed

Lines changed: 183 additions & 225 deletions

File tree

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,6 @@ internal ProcessStartArguments() { }
304304
public Microsoft.Win32.SafeHandles.SafeFileHandle StandardError { get { throw null; } }
305305
public Microsoft.Win32.SafeHandles.SafeFileHandle StandardInput { get { throw null; } }
306306
public Microsoft.Win32.SafeHandles.SafeFileHandle StandardOutput { get { throw null; } }
307-
public string? WorkingDirectory { get { throw null; } }
308307
}
309308
public enum ProcessPriorityClass
310309
{

‎src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,79 @@ internal static SafeProcessHandle StartCore(ProcessStartInfo startInfo, SafeFile
218218
out waitStateHolder);
219219
}
220220

221+
internal static unsafe SafeProcessHandle StartWithCallback(ProcessStartInfo startInfo, SafeFileHandle childInput, SafeFileHandle childOutput, SafeFileHandle childError,
222+
Func<ProcessStartArguments, SafeProcessHandle> callback, out ProcessWaitState.Holder? waitStateHolder)
223+
{
224+
waitStateHolder = null;
225+
ProcessUtils.EnsureInitialized();
226+
227+
string? resolvedFileName = ProcessUtils.ResolvePath(startInfo.FileName);
228+
string[] argv = ProcessUtils.ParseArgv(startInfo);
229+
bool usesTerminal = UsesTerminal(childInput, childOutput, childError);
230+
231+
byte** argvPtr = null;
232+
byte** envpPtr = null;
233+
234+
try
235+
{
236+
Interop.Sys.AllocArgvArray(argv, ref argvPtr);
237+
Interop.Sys.AllocEnvpArray(startInfo.Environment, ref envpPtr);
238+
239+
ProcessStartArguments args = new()
240+
{
241+
FileName = resolvedFileName,
242+
Arguments = argvPtr,
243+
EnvironmentVariables = envpPtr,
244+
StandardInput = childInput,
245+
StandardOutput = childOutput,
246+
StandardError = childError,
247+
ProcessStartInfo = startInfo,
248+
};
249+
250+
// Lock to avoid races with OnSigChild
251+
// By using a ReaderWriterLock we allow multiple processes to start concurrently.
252+
ProcessUtils.s_processStartLock.EnterReadLock();
253+
254+
try
255+
{
256+
if (usesTerminal)
257+
{
258+
ProcessUtils.ConfigureTerminalForChildProcesses(1);
259+
}
260+
261+
SafeProcessHandle processHandle = callback(args);
262+
if (processHandle is null || processHandle.IsInvalid)
263+
{
264+
throw new ArgumentException(SR.Argument_InvalidHandle, nameof(callback));
265+
}
266+
267+
waitStateHolder = new ProcessWaitState.Holder(processHandle.ProcessId, isNewChild: true, usesTerminal);
268+
return processHandle;
269+
}
270+
catch
271+
{
272+
if (usesTerminal)
273+
{
274+
// We failed to launch a child that could use the terminal.
275+
ProcessUtils.s_processStartLock.EnterWriteLock();
276+
ProcessUtils.ConfigureTerminalForChildProcesses(-1);
277+
ProcessUtils.s_processStartLock.ExitWriteLock();
278+
}
279+
280+
throw;
281+
}
282+
finally
283+
{
284+
ProcessUtils.s_processStartLock.ExitReadLock();
285+
}
286+
}
287+
finally
288+
{
289+
NativeMemory.Free(envpPtr);
290+
NativeMemory.Free(argvPtr);
291+
}
292+
}
293+
221294
private static SafeProcessHandle StartWithShellExecute(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle,
222295
SafeFileHandle? stderrHandle, out ProcessWaitState.Holder? waitStateHolder)
223296
{

‎src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,77 @@ internal static unsafe SafeProcessHandle StartCore(ProcessStartInfo startInfo, S
356356
return procSH;
357357
}
358358

359+
internal static unsafe SafeProcessHandle StartWithCallback(ProcessStartInfo startInfo, SafeFileHandle childInput, SafeFileHandle childOutput, SafeFileHandle childError,
360+
Func<ProcessStartArguments, SafeProcessHandle> callback)
361+
{
362+
ValueStringBuilder commandLine = new(stackalloc char[256]);
363+
ProcessUtils.BuildCommandLine(startInfo, ref commandLine);
364+
commandLine.NullTerminate();
365+
366+
string? environmentBlock = null;
367+
if (startInfo._environmentVariables != null)
368+
{
369+
environmentBlock = ProcessUtils.GetEnvironmentVariablesBlock(startInfo._environmentVariables!);
370+
}
371+
372+
ProcessStartArguments args = new()
373+
{
374+
FileName = null, // On Windows, the file name is embedded in the command line
375+
StandardInput = childInput,
376+
StandardOutput = childOutput,
377+
StandardError = childError,
378+
ProcessStartInfo = startInfo,
379+
};
380+
381+
// Acquire the process lock to avoid accidental handle inheritance issues.
382+
ProcessUtils.s_processStartLock.EnterWriteLock();
383+
384+
try
385+
{
386+
EnableInheritance(childInput);
387+
EnableInheritance(childOutput);
388+
EnableInheritance(childError);
389+
390+
fixed (char* commandLinePtr = &commandLine.GetPinnableReference())
391+
fixed (char* environmentBlockPtr = environmentBlock)
392+
{
393+
args.Arguments = commandLinePtr;
394+
args.EnvironmentVariables = environmentBlockPtr;
395+
return callback(args);
396+
}
397+
}
398+
finally
399+
{
400+
ProcessUtils.s_processStartLock.ExitWriteLock();
401+
}
402+
403+
static void EnableInheritance(SafeFileHandle safeHandle)
404+
{
405+
bool refAdded = false;
406+
407+
try
408+
{
409+
safeHandle.DangerousAddRef(ref refAdded);
410+
411+
// Enable inheritance on this handle so the child process can use it.
412+
if (!Interop.Kernel32.SetHandleInformation(
413+
safeHandle.DangerousGetHandle(),
414+
Interop.Kernel32.HandleFlags.HANDLE_FLAG_INHERIT,
415+
Interop.Kernel32.HandleFlags.HANDLE_FLAG_INHERIT))
416+
{
417+
throw new Win32Exception(Marshal.GetLastWin32Error());
418+
}
419+
}
420+
finally
421+
{
422+
if (refAdded)
423+
{
424+
safeHandle.DangerousRelease();
425+
}
426+
}
427+
}
428+
}
429+
359430
private static unsafe SafeProcessHandle StartWithShellExecute(ProcessStartInfo startInfo)
360431
{
361432
if (!string.IsNullOrEmpty(startInfo.UserName) || startInfo.Password != null)

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

Lines changed: 11 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -358,13 +358,20 @@ private SafeProcessHandle GetProcessHandle()
358358
return new SafeProcessHandle(_waitStateHolder!.IncrementRefCount());
359359
}
360360

361-
private bool StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle, SafeFileHandle? stderrHandle, SafeHandle[]? inheritedHandles)
361+
private bool StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle, SafeFileHandle? stderrHandle, SafeHandle[]? inheritedHandles, Func<ProcessStartArguments, SafeProcessHandle>? callback)
362362
{
363-
SafeProcessHandle startedProcess = SafeProcessHandle.StartCore(startInfo, stdinHandle, stdoutHandle, stderrHandle, inheritedHandles, out ProcessWaitState.Holder? waitStateHolder);
363+
ProcessWaitState.Holder? waitStateHolder = null;
364+
365+
SafeProcessHandle startedProcess = callback is null
366+
? SafeProcessHandle.StartCore(startInfo, stdinHandle, stdoutHandle, stderrHandle, inheritedHandles, out waitStateHolder)
367+
: SafeProcessHandle.StartWithCallback(startInfo, stdinHandle!, stdoutHandle!, stderrHandle!, callback, out waitStateHolder);
368+
364369
Debug.Assert(!startedProcess.IsInvalid);
365370

366-
// SafeProcessHandle has its own copy of the wait state holder, so we need to increment the ref count for our copy.
367-
_waitStateHolder = waitStateHolder!.IncrementRefCount();
371+
_waitStateHolder = callback is null
372+
? waitStateHolder!.IncrementRefCount() // SafeProcessHandle has its own copy of the wait state holder, so we need to increment the ref count for our copy.
373+
: waitStateHolder; // we created a dedicated holder
374+
368375
SetProcessHandle(startedProcess);
369376
SetProcessId(startedProcess.ProcessId);
370377
return true;
@@ -409,43 +416,6 @@ private static AnonymousPipeClientStream OpenStream(SafeFileHandle handle, FileA
409416
return new AnonymousPipeClientStream(direction, safePipeHandle);
410417
}
411418

412-
private static unsafe partial SafeProcessHandle InvokeStartCallback(ProcessStartInfo startInfo, SafeFileHandle childInput, SafeFileHandle childOutput, SafeFileHandle childError, Func<ProcessStartArguments, SafeProcessHandle> callback)
413-
{
414-
string? resolvedFileName = ProcessUtils.ResolvePath(startInfo.FileName);
415-
string[] argv = ProcessUtils.ParseArgv(startInfo);
416-
417-
IDictionary<string, string?> env = startInfo.Environment;
418-
string? workingDirectory = !string.IsNullOrWhiteSpace(startInfo.WorkingDirectory) ? startInfo.WorkingDirectory : null;
419-
420-
byte** argvPtr = null;
421-
byte** envpPtr = null;
422-
423-
try
424-
{
425-
Interop.Sys.AllocArgvArray(argv, ref argvPtr);
426-
Interop.Sys.AllocEnvpArray(env, ref envpPtr);
427-
428-
ProcessStartArguments args = new()
429-
{
430-
FileName = resolvedFileName,
431-
Arguments = argvPtr,
432-
WorkingDirectory = workingDirectory,
433-
EnvironmentVariables = envpPtr,
434-
StandardInput = childInput,
435-
StandardOutput = childOutput,
436-
StandardError = childError,
437-
ProcessStartInfo = startInfo,
438-
};
439-
440-
return callback(args);
441-
}
442-
finally
443-
{
444-
NativeMemory.Free(envpPtr);
445-
NativeMemory.Free(argvPtr);
446-
}
447-
}
448-
449419
private static Encoding GetStandardInputEncoding() => Encoding.Default;
450420

451421
private static Encoding GetStandardOutputEncoding() => Encoding.Default;

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

Lines changed: 10 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -508,12 +508,19 @@ private SafeProcessHandle GetProcessHandle(int access, bool throwIfExited = true
508508

509509
private static ConsoleEncoding GetStandardOutputEncoding() => GetEncoding((int)Interop.Kernel32.GetConsoleOutputCP());
510510

511-
private bool StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle, SafeFileHandle? stderrHandle, SafeHandle[]? inheritedHandles)
511+
private bool StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle, SafeFileHandle? stdoutHandle, SafeFileHandle? stderrHandle, SafeHandle[]? inheritedHandles, Func<ProcessStartArguments, SafeProcessHandle>? callback)
512512
{
513-
SafeProcessHandle startedProcess = SafeProcessHandle.StartCore(startInfo, stdinHandle, stdoutHandle, stderrHandle, inheritedHandles);
513+
SafeProcessHandle startedProcess = callback is null
514+
? SafeProcessHandle.StartCore(startInfo, stdinHandle, stdoutHandle, stderrHandle, inheritedHandles)
515+
: SafeProcessHandle.StartWithCallback(startInfo, stdinHandle!, stdoutHandle!, stderrHandle!, callback);
514516

515-
if (startedProcess.IsInvalid)
517+
if (startedProcess is null || startedProcess.IsInvalid)
516518
{
519+
if (callback is not null)
520+
{
521+
throw new ArgumentException(SR.Argument_InvalidHandle, nameof(callback));
522+
}
523+
517524
Debug.Assert(startInfo.UseShellExecute);
518525
return false;
519526
}
@@ -526,43 +533,6 @@ private bool StartCore(ProcessStartInfo startInfo, SafeFileHandle? stdinHandle,
526533
return true;
527534
}
528535

529-
private static unsafe partial SafeProcessHandle InvokeStartCallback(ProcessStartInfo startInfo, SafeFileHandle childInput, SafeFileHandle childOutput, SafeFileHandle childError, Func<ProcessStartArguments, SafeProcessHandle> callback)
530-
{
531-
ValueStringBuilder commandLine = new(stackalloc char[256]);
532-
ProcessUtils.BuildCommandLine(startInfo, ref commandLine);
533-
commandLine.NullTerminate();
534-
535-
string? environmentBlock = null;
536-
if (startInfo._environmentVariables != null)
537-
{
538-
environmentBlock = ProcessUtils.GetEnvironmentVariablesBlock(startInfo._environmentVariables!);
539-
}
540-
541-
string? workingDirectory = startInfo.WorkingDirectory;
542-
if (workingDirectory is not null && workingDirectory.Length == 0)
543-
{
544-
workingDirectory = null;
545-
}
546-
547-
ProcessStartArguments args = new()
548-
{
549-
FileName = null, // On Windows, the file name is embedded in the command line
550-
WorkingDirectory = workingDirectory,
551-
StandardInput = childInput,
552-
StandardOutput = childOutput,
553-
StandardError = childError,
554-
ProcessStartInfo = startInfo,
555-
};
556-
557-
fixed (char* commandLinePtr = &commandLine.GetPinnableReference())
558-
fixed (char* environmentBlockPtr = environmentBlock)
559-
{
560-
args.Arguments = commandLinePtr;
561-
args.EnvironmentVariables = environmentBlockPtr;
562-
return callback(args);
563-
}
564-
}
565-
566536
private string GetMainWindowTitle()
567537
{
568538
IntPtr handle = MainWindowHandle;

0 commit comments

Comments
 (0)