From 484f90405a7d44446a057bcf16885d813c895267 Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Wed, 2 May 2018 10:05:16 -0700 Subject: [PATCH] Fix Process.ExitCode on mac for killed processes (#29407) (#29445) * Fix Process.ExitCode on mac for killed processes https://github.com/dotnet/corefx/pull/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 --- .../Unix/System.Native/Interop.WaitId.cs | 11 ++--- .../Unix/System.Native/Interop.WaitPid.cs | 24 +++++++++++ src/Native/Unix/System.Native/pal_process.cpp | 42 ++++++++++++------- src/Native/Unix/System.Native/pal_process.h | 16 +++++-- .../src/System.Diagnostics.Process.csproj | 3 ++ .../Diagnostics/ProcessWaitState.Unix.cs | 7 ++-- .../tests/ProcessTests.Unix.cs | 15 +++++++ 7 files changed, 89 insertions(+), 29 deletions(-) create mode 100644 src/Common/src/Interop/Unix/System.Native/Interop.WaitPid.cs diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.WaitId.cs b/src/Common/src/Interop/Unix/System.Native/Interop.WaitId.cs index a8a0df58a867..3bf9e82e1145 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.WaitId.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.WaitId.cs @@ -10,17 +10,14 @@ internal static partial class Interop internal static partial class Sys { /// - /// Waits for terminated child processes. + /// Returns the pid of a terminated child without reaping it. /// - /// The PID of a child process. -1 for any child. - /// The output exit status of the process - /// Tells the OS to leave the child waitable or reap 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 /// - [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(); } } diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.WaitPid.cs b/src/Common/src/Interop/Unix/System.Native/Interop.WaitPid.cs new file mode 100644 index 000000000000..8292971d48e8 --- /dev/null +++ b/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 + { + /// + /// 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. + /// + [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_WaitPidExitedNoHang", SetLastError = true)] + internal static extern int WaitPidExitedNoHang(int pid, out int exitCode); + } +} diff --git a/src/Native/Unix/System.Native/pal_process.cpp b/src/Native/Unix/System.Native/pal_process.cpp index 0d627f4588b6..1790e76deaa1 100644 --- a/src/Native/Unix/System.Native/pal_process.cpp +++ b/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(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(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; } diff --git a/src/Native/Unix/System.Native/pal_process.h b/src/Native/Unix/System.Native/pal_process.h index 55b30accb572..71155f0d1eb5 100644 --- a/src/Native/Unix/System.Native/pal_process.h +++ b/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. diff --git a/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj b/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj index c8de23462b6d..87a772cbd427 100644 --- a/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj +++ b/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj @@ -351,6 +351,9 @@ Common\Interop\Unix\Interop.WaitId.cs + + Common\Interop\Unix\Interop.WaitPid.cs + diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs index f9ab4d64752d..bb5e79ca6901 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs +++ b/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); } } diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index c8a652f2687a..226aa0e273de 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -387,6 +387,21 @@ public void TestStartWithNonExistingUserThrows() Assert.Throws(() => 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); + } + } + /// /// Tests when running as a normal user and starting a new process as the same user /// works as expected.