Fix Process.ExitCode on mac for killed processes (#29407) (#29445)

* Fix Process.ExitCode on mac for killed processes

#26291 changed process reaping
from using waitpid to waitid. This caused a regression on mac, since
for processes that are killed, (on mac) waitid does not return the
signal number that caused the process to terminated.

We change back to waitpid for reaping children and determining the
exit code. waitid is used to find terminated children.

Fixes https://github.com/dotnet/corefx/issues/29370

* TestExitCodeKilledChild: remove runtime check

* TestExitCodeKilledChild: remove greater than assert

Author: joperezr

src/Common/src/Interop/Unix/System.Native/Interop.WaitId.cs

@@ -10,17 +10,14 @@ internal static partial class Interop
     internal static partial class Sys
     {        
         /// <summary>
-        /// Waits for terminated child processes.
+        /// Returns the pid of a terminated child without reaping it.
         /// </summary>
-        /// <param name="pid">The PID of a child process. -1 for any child.</param>
-        /// <param name="status">The output exit status of the process</param>
-        /// <param name="keepWaitable">Tells the OS to leave the child waitable or reap it.</param>
         /// <returns>
         /// 1) returns the process id of a terminated child process
-        /// 2) if no children are waiting, 0 is returned
+        /// 2) if no children are terminated, 0 is returned
         /// 3) on error, -1 is returned
         /// </returns>
-        [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_WaitIdExitedNoHang", SetLastError = true)]
-        internal static extern int WaitIdExitedNoHang(int pid, out int exitCode, bool keepWaitable);
+        [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_WaitIdAnyExitedNoHangNoWait", SetLastError = true)]
+        internal static extern int WaitIdAnyExitedNoHangNoWait();
     }
 }

src/Common/src/Interop/Unix/System.Native/Interop.WaitPid.cs

@@ -0,0 +1,24 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Runtime.InteropServices;
+
+internal static partial class Interop
+{
+    internal static partial class Sys
+    {
+        /// <summary>
+        /// Reaps a terminated child.
+        /// </summary>
+        /// <returns>
+        /// 1) when a child is reaped, its process id is returned
+        /// 2) if pid is not a child or there are no unwaited-for children, -1 is returned (errno=ECHILD)
+        /// 3) if the child has not yet terminated, 0 is returned
+        /// 4) on error, -1 is returned.
+        /// </returns>
+        [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_WaitPidExitedNoHang", SetLastError = true)]
+        internal static extern int WaitPidExitedNoHang(int pid, out int exitCode);
+    }
+}

src/Native/Unix/System.Native/pal_process.cpp

@@ -450,34 +450,46 @@ extern "C" void SystemNative_SysLog(SysLogPriority priority, const char* message
     syslog(static_cast<int>(priority), message, arg1);
 }
 
-extern "C" int32_t SystemNative_WaitIdExitedNoHang(int32_t pid, int32_t* exitCode, int32_t keepWaitable)
+extern "C" int32_t SystemNative_WaitIdAnyExitedNoHangNoWait()
 {
-    assert(exitCode != nullptr);
-
     siginfo_t siginfo;
     int32_t result;
-    idtype_t idtype = pid == -1 ? P_ALL : P_PID;
-    int options = WEXITED | WNOHANG;
-    if (keepWaitable != 0)
-    {
-        options |= WNOWAIT;
-    }
-    while (CheckInterrupted(result = waitid(idtype, static_cast<id_t>(pid), &siginfo, options)));
-    if (idtype == P_ALL && result == -1 && errno == ECHILD)
+    while (CheckInterrupted(result = waitid(P_ALL, 0, &siginfo, WEXITED | WNOHANG | WNOWAIT)));
+    if (result == -1 && errno == ECHILD)
     {
+        // The calling process has no existing unwaited-for child processes.
         result = 0;
     }
     else if (result == 0 && siginfo.si_signo == SIGCHLD)
     {
-        if (siginfo.si_code == CLD_EXITED)
+        result = siginfo.si_pid;
+    }
+    return result;
+}
+
+extern "C" int32_t SystemNative_WaitPidExitedNoHang(int32_t pid, int32_t* exitCode)
+{
+    assert(exitCode != nullptr);
+
+    int32_t result;
+    int status;
+    while (CheckInterrupted(result = waitpid(pid, &status, WNOHANG)));
+    if (result > 0)
+    {
+        if (WIFEXITED(status))
+        {
+            // the child terminated normally.
+            *exitCode = WEXITSTATUS(status);
+        }
+        else if (WIFSIGNALED(status))
         {
-            *exitCode = siginfo.si_status;
+            // child process was terminated by a signal.
+            *exitCode = 128 + WTERMSIG(status);
         }
         else
         {
-            *exitCode = 128 + siginfo.si_status;
+            assert(false);
         }
-        result = siginfo.si_pid;
     }
     return result;
 }

src/Native/Unix/System.Native/pal_process.h

@@ -206,13 +206,23 @@ extern "C" int32_t SystemNative_GetSid(int32_t pid);
 extern "C" void SystemNative_SysLog(SysLogPriority priority, const char* message, const char* arg1);
 
 /**
- * Waits for terminated child processes.
+ * Returns the pid of a terminated child without reaping it.
  *
  * 1) returns the process id of a terminated child process
- * 2) if no children are waiting, 0 is returned
+ * 2) if no children are terminated, 0 is returned
  * 3) on error, -1 is returned
  */
-extern "C" int32_t SystemNative_WaitIdExitedNoHang(int32_t pid, int32_t* exitCode, int32_t keepWaitable);
+extern "C" int32_t SystemNative_WaitIdAnyExitedNoHangNoWait();
+
+/**
+ * Reaps a terminated child.
+ *
+ * 1) when a child is reaped, its process id is returned
+ * 2) if pid is not a child or there are no unwaited-for children, -1 is returned (errno=ECHILD)
+ * 3) if the child has not yet terminated, 0 is returned
+ * 4) on error, -1 is returned.
+ */
+extern "C" int32_t SystemNative_WaitPidExitedNoHang(int32_t pid, int32_t* exitCode);
 
 /**
  * Gets the configurable limit or variable for system path or file descriptor options.

src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj

@@ -351,6 +351,9 @@
     <Compile Include="$(CommonPath)\Interop\Unix\System.Native\Interop.WaitId.cs">
       <Link>Common\Interop\Unix\Interop.WaitId.cs</Link>
     </Compile>
+    <Compile Include="$(CommonPath)\Interop\Unix\System.Native\Interop.WaitPid.cs">
+      <Link>Common\Interop\Unix\Interop.WaitPid.cs</Link>
+    </Compile>
   </ItemGroup>
   <ItemGroup Condition=" '$(TargetsLinux)' == 'true'">
     <Compile Include="System\Diagnostics\Process.Linux.cs" />

src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs

@@ -527,7 +527,7 @@ private bool TryReapChild()
 
                 // Try to get the state of the child process
                 int exitCode;
-                int waitResult = Interop.Sys.WaitIdExitedNoHang(_processId, out exitCode, keepWaitable: false);
+                int waitResult = Interop.Sys.WaitPidExitedNoHang(_processId, out exitCode);
 
                 if (waitResult == _processId)
                 {
@@ -563,8 +563,7 @@ internal static void CheckChildren(bool reapAll)
                 do
                 {
                     // Find a process that terminated without reaping it yet.
-                    int exitCode;
-                    pid = Interop.Sys.WaitIdExitedNoHang(-1, out exitCode, keepWaitable: true);
+                    pid = Interop.Sys.WaitIdAnyExitedNoHangNoWait();
                     if (pid > 0)
                     {
                         if (s_childProcessWaitStates.TryGetValue(pid, out ProcessWaitState pws))
@@ -638,7 +637,7 @@ internal static void CheckChildren(bool reapAll)
                     do
                     {
                         int exitCode;
-                        pid = Interop.Sys.WaitIdExitedNoHang(-1, out exitCode, keepWaitable: false);
+                        pid = Interop.Sys.WaitPidExitedNoHang(-1, out exitCode);
                     } while (pid > 0);
                 }
             }

src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

@@ -387,6 +387,21 @@ public void TestStartWithNonExistingUserThrows()
             Assert.Throws<Win32Exception>(() => p.Start());
         }
 
+        [Fact]
+        public void TestExitCodeKilledChild()
+        {
+            using (Process p = CreateProcessLong())
+            {
+                p.Start();
+                p.Kill();
+                p.WaitForExit();
+
+                // SIGKILL may change per platform
+                const int SIGKILL = 9; // Linux, macOS, FreeBSD, ...
+                Assert.Equal(128 + SIGKILL, p.ExitCode);
+            }
+        }
+
         /// <summary>
         /// Tests when running as a normal user and starting a new process as the same user
         /// works as expected.