diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs index 05046528fa6f59..8635a81f9f1452 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs @@ -220,46 +220,14 @@ private void UpdateHasExited() if (handle.IsInvalid) { _exited = true; + return; } - else - { - int localExitCode; - // Although this is the wrong way to check whether the process has exited, - // it was historically the way we checked for it, and a lot of code then took a dependency on - // the fact that this would always be set before the pipes were closed, so they would read - // the exit code out after calling ReadToEnd() or standard output or standard error. In order - // to allow 259 to function as a valid exit code and to break as few people as possible that - // took the ReadToEnd dependency, we check for an exit code before doing the more correct - // check to see if we have been signaled. - if (Interop.Kernel32.GetExitCodeProcess(handle, out localExitCode) && localExitCode != Interop.Kernel32.HandleOptions.STILL_ACTIVE) - { - _exitCode = localExitCode; - _exited = true; - } - else - { - // The best check for exit is that the kernel process object handle is invalid, - // or that it is valid and signaled. Checking if the exit code != STILL_ACTIVE - // does not guarantee the process is closed, - // since some process could return an actual STILL_ACTIVE exit code (259). - if (!_signaled) // if we just came from WaitForExit, don't repeat - { - using (var wh = new Interop.Kernel32.ProcessWaitHandle(handle)) - { - _signaled = wh.WaitOne(0); - } - } - if (_signaled) - { - if (!Interop.Kernel32.GetExitCodeProcess(handle, out localExitCode)) - throw new Win32Exception(); + if (!ProcessManager.HasExited(handle, ref _signaled, out int localExitCode)) + return; - _exitCode = localExitCode; - _exited = true; - } - } - } + _exited = true; + _exitCode = localExitCode; } } 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 ad791263cbf078..8172dba2808037 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 @@ -28,15 +28,16 @@ public static bool IsProcessRunning(int processId) public static bool IsProcessRunning(int processId, string machineName) { // Performance optimization for the local machine: - // First try to OpenProcess by id, if valid handle is returned, the process is definitely running + // First try to OpenProcess by id, if valid handle is returned verify that process is running // Otherwise enumerate all processes and compare ids if (!IsRemoteMachine(machineName)) { - using (SafeProcessHandle processHandle = Interop.Kernel32.OpenProcess(ProcessOptions.PROCESS_QUERY_INFORMATION, false, processId)) + using (SafeProcessHandle processHandle = Interop.Kernel32.OpenProcess(ProcessOptions.PROCESS_QUERY_LIMITED_INFORMATION | ProcessOptions.SYNCHRONIZE, false, processId)) { if (!processHandle.IsInvalid) { - return true; + bool signaled = false; + return !HasExited(processHandle, ref signaled, out _); } } } @@ -247,6 +248,43 @@ public static SafeThreadHandle OpenThread(int threadId, int access) } return threadHandle; } + + // Handle should be valid and have PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE access + public static bool HasExited(SafeProcessHandle handle, ref bool signaled, out int exitCode) + { + // Although this is the wrong way to check whether the process has exited, + // it was historically the way we checked for it, and a lot of code then took a dependency on + // the fact that this would always be set before the pipes were closed, so they would read + // the exit code out after calling ReadToEnd() or standard output or standard error. In order + // to allow 259 to function as a valid exit code and to break as few people as possible that + // took the ReadToEnd dependency, we check for an exit code before doing the more correct + // check to see if we have been signaled. + if (Interop.Kernel32.GetExitCodeProcess(handle, out exitCode) && exitCode != Interop.Kernel32.HandleOptions.STILL_ACTIVE) + { + return true; + } + + // The best check for exit is that the kernel process object handle is invalid, + // or that it is valid and signaled. Checking if the exit code != STILL_ACTIVE + // does not guarantee the process is closed, + // since some process could return an actual STILL_ACTIVE exit code (259). + if (!signaled) // if we just came from Process.WaitForExit, don't repeat + { + using (var wh = new Interop.Kernel32.ProcessWaitHandle(handle)) + { + signaled = wh.WaitOne(0); + } + } + if (signaled) + { + if (!Interop.Kernel32.GetExitCodeProcess(handle, out exitCode)) + throw new Win32Exception(); + + return true; + } + + return false; + } } /// diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index b9d70837e3d168..441036b8380fe2 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -1118,6 +1118,18 @@ public void TestGetProcessById() Assert.Equal(_process.ProcessName, p.ProcessName); } + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + public void GetProcessById_KilledProcess_ThrowsArgumentException() + { + Process process = CreateDefaultProcess(); + var handle = process.SafeHandle; + int processId = process.Id; + process.Kill(); + process.WaitForExit(WaitInMS); + Assert.Throws(() => Process.GetProcessById(processId)); + GC.KeepAlive(handle); + } + [Fact] [SkipOnPlatform(TestPlatforms.iOS | TestPlatforms.tvOS, "libproc is not supported on iOS/tvOS")] public void TestGetProcesses()