diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Win32.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Win32.cs index 95703701791797..bd551a53f8d311 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Win32.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Win32.cs @@ -328,13 +328,65 @@ private bool WaitForInputIdleCore(int milliseconds) /// private bool IsParentOf(Process possibleChild) { - try + // Use non-throwing helpers to avoid first-chance exceptions during enumeration. + // This is critical for performance when a debugger is attached. + if (!TryGetStartTime(out DateTime myStartTime) || + !possibleChild.TryGetStartTime(out DateTime childStartTime) || + !possibleChild.TryGetParentProcessId(out int childParentId)) { - return StartTime < possibleChild.StartTime && Id == possibleChild.ParentProcessId; + return false; } - catch (Exception e) when (IsProcessInvalidException(e)) + + return myStartTime < childStartTime && Id == childParentId; + } + + /// + /// Try to get the process start time without throwing exceptions. + /// + private bool TryGetStartTime(out DateTime startTime) + { + startTime = default; + using (SafeProcessHandle handle = GetProcessHandle(Interop.Advapi32.ProcessOptions.PROCESS_QUERY_LIMITED_INFORMATION, false)) { - return false; + if (handle.IsInvalid) + { + return false; + } + + ProcessThreadTimes processTimes = new ProcessThreadTimes(); + if (!Interop.Kernel32.GetProcessTimes(handle, + out processTimes._create, out processTimes._exit, + out processTimes._kernel, out processTimes._user)) + { + return false; + } + + startTime = processTimes.StartTime; + return true; + } + } + + /// + /// Try to get the parent process ID without throwing exceptions. + /// + private unsafe bool TryGetParentProcessId(out int parentProcessId) + { + parentProcessId = 0; + using (SafeProcessHandle handle = GetProcessHandle(Interop.Advapi32.ProcessOptions.PROCESS_QUERY_LIMITED_INFORMATION, false)) + { + if (handle.IsInvalid) + { + return false; + } + + Interop.NtDll.PROCESS_BASIC_INFORMATION info; + if (Interop.NtDll.NtQueryInformationProcess(handle, Interop.NtDll.ProcessBasicInformation, &info, (uint)sizeof(Interop.NtDll.PROCESS_BASIC_INFORMATION), out _) != 0) + { + return false; + } + + parentProcessId = (int)info.InheritedFromUniqueProcessId; + return true; } } @@ -359,14 +411,20 @@ private unsafe int ParentProcessId private bool Equals(Process process) { - try + // Check IDs first since they're cheap to compare and will fail most of the time. + if (Id != process.Id) { - return Id == process.Id && StartTime == process.StartTime; + return false; } - catch (Exception e) when (IsProcessInvalidException(e)) + + // Use non-throwing helper to avoid first-chance exceptions during enumeration. + if (!TryGetStartTime(out DateTime myStartTime) || + !process.TryGetStartTime(out DateTime otherStartTime)) { return false; } + + return myStartTime == otherStartTime; } private List? KillTree() diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Windows.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Windows.cs index c27da2b283ed42..102ba0be16367d 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Windows.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Windows.cs @@ -236,7 +236,7 @@ static unsafe ProcessManager() } } - public static SafeProcessHandle OpenProcess(int processId, int access, bool throwIfExited) + public static SafeProcessHandle OpenProcess(int processId, int access, bool throwOnError) { SafeProcessHandle processHandle = Interop.Kernel32.OpenProcess(access, false, processId); int result = Marshal.GetLastWin32Error(); @@ -247,24 +247,21 @@ public static SafeProcessHandle OpenProcess(int processId, int access, bool thro processHandle.Dispose(); + if (!throwOnError) + { + return SafeProcessHandle.InvalidHandle; + } + if (processId == 0) { throw new Win32Exception(Interop.Errors.ERROR_ACCESS_DENIED); } - // If the handle is invalid because the process has exited, only throw an exception if throwIfExited is true. - // Assume the process is still running if the error was ERROR_ACCESS_DENIED for better performance - if (result != Interop.Errors.ERROR_ACCESS_DENIED && !IsProcessRunning(processId)) + if (!IsProcessRunning(processId)) { - if (throwIfExited) - { - throw new InvalidOperationException(SR.Format(SR.ProcessHasExited, processId.ToString())); - } - else - { - return SafeProcessHandle.InvalidHandle; - } + throw new InvalidOperationException(SR.Format(SR.ProcessHasExited, processId.ToString())); } + throw new Win32Exception(result); } diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Windows.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Windows.cs index 7850ca8afcbd7d..11992f29453dbe 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Windows.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Windows.cs @@ -5,6 +5,8 @@ using System.ComponentModel; using System.IO; using System.Runtime.InteropServices; +using System.Threading; +using Microsoft.DotNet.RemoteExecutor; using Microsoft.DotNet.XUnitExtensions; using Xunit; @@ -55,5 +57,98 @@ private static unsafe void ReEnableCtrlCHandlerIfNeeded(PosixSignal signal) } } } + + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + public void Kill_EntireProcessTree_MinimalExceptions() + { + // This test validates that Kill(true) doesn't throw excessive exceptions internally + // during process enumeration, which causes severe performance degradation with debugger attached. + // See https://github.com/dotnet/runtime/issues/121279 + + int exceptionCount = 0; + EventHandler handler = + (sender, e) => Interlocked.Increment(ref exceptionCount); + + AppDomain.CurrentDomain.FirstChanceException += handler; + + try + { + // Create a process tree based on the repro from the issue: + // Main process spawns child1, child1 spawns child2 + RemoteInvokeHandle handle = RemoteExecutor.Invoke(CreateProcessTreeAndWait); + Process parentProcess = handle.Process; + + try + { + // Wait a bit to ensure the process tree is established + Thread.Sleep(1500); + + // Reset the counter before killing + exceptionCount = 0; + + // Kill the entire process tree + parentProcess.Kill(entireProcessTree: true); + + // Wait for process to exit + Assert.True(parentProcess.WaitForExit(Helpers.PassingTestTimeoutMilliseconds)); + + // The fix should ensure that we don't throw exceptions for each system process + // that we can't access during enumeration. Allow up to 5 exceptions for edge cases + // (e.g., processes that exit during enumeration, rare error conditions) + Assert.True(exceptionCount <= 5, + $"Expected no more than 5 exceptions during Kill(true), but got {exceptionCount}"); + } + finally + { + try + { + if (!parentProcess.HasExited) + { + parentProcess.Kill(); + } + handle.Dispose(); + } + catch + { + // Best effort cleanup + } + } + } + finally + { + AppDomain.CurrentDomain.FirstChanceException -= handler; + } + } + + private static int CreateProcessTreeAndWait() + { + // This mimics the repro code from the issue + // Create child1 which will create child2 + ProcessStartInfo psi = new ProcessStartInfo + { + FileName = RemoteExecutor.HostRunner, + Arguments = $"exec \"{RemoteExecutor.Path}\" {typeof(ProcessTests).Assembly.Location} {nameof(SleepForever)}", + UseShellExecute = false, + RedirectStandardOutput = true, + RedirectStandardError = true, + }; + + using (Process child1 = Process.Start(psi)) + { + // Wait a bit to ensure child1 is running + Thread.Sleep(500); + + // Keep this process alive + Thread.Sleep(Timeout.Infinite); + } + + return RemoteExecutor.SuccessExitCode; + } + + private static int SleepForever() + { + Thread.Sleep(Timeout.Infinite); + return RemoteExecutor.SuccessExitCode; + } } }