Process.GetProcessById bugfix (#64723)

* Bugfix: Process.GetProcessById claims killed process is still alive in .NET Core #63937

* Added test for GetProcessById with killed process

Co-authored-by: epeshk <>

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 0504652..8635a81 100644 (file)
--- 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 @@ namespace System.Diagnostics
                 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 ad79126..8172dba 100644 (file)
--- 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 @@ namespace System.Diagnostics
         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 @@ namespace System.Diagnostics
             }
             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;
+        }
     }
 
     /// <devdoc>

diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs
index b9d7083..441036b 100644 (file)
--- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs
+++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs
@@ -1118,6 +1118,18 @@ namespace System.Diagnostics.Tests
             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<ArgumentException>(() => Process.GetProcessById(processId));
+            GC.KeepAlive(handle);
+        }
+
         [Fact]
         [SkipOnPlatform(TestPlatforms.iOS | TestPlatforms.tvOS, "libproc is not supported on iOS/tvOS")]
         public void TestGetProcesses()