On SIGTERM default to a non-zero exit code (dotnet/coreclr#21300)

* On SIGTERM default to a non-zero exit code

* Fix Windows builds

* Improve SIG_DFL/SIG_IGN handling

* Remove PAL_GetTerminationExitCode

* Use sa_handler/sa_sigaction based on SA_SIGINFO; remove HAVE_SIGINFO_T.

* configure.cmake: remove siginfo_t check

* Move restore_signal_and_resend so OSX can use it; add function documentation

* Fix OSX build: include pal/process.h for gPID

* Check SIG_IGN and SIG_DFL against sa_handler

* Don't use sa_handler when SA_SIGINFO is set

* Fix equality check

* Swap order of checking SA_SIGINFO and SIG_IGN/SIG_DFL


Commit migrated from dotnet/coreclr@3f09035

Author: tmds

src/coreclr/src/pal/inc/pal.h

@@ -5057,7 +5057,7 @@ struct PAL_SEHException
 
 typedef BOOL (*PHARDWARE_EXCEPTION_HANDLER)(PAL_SEHException* ex);
 typedef BOOL (*PHARDWARE_EXCEPTION_SAFETY_CHECK_FUNCTION)(PCONTEXT contextRecord, PEXCEPTION_RECORD exceptionRecord);
-typedef VOID (*PTERMINATION_REQUEST_HANDLER)();
+typedef VOID (*PTERMINATION_REQUEST_HANDLER)(int terminationExitCode);
 typedef DWORD (*PGET_GCMARKER_EXCEPTION_CODE)(LPVOID ip);
 
 PALIMPORT

src/coreclr/src/pal/src/config.h.in

@@ -78,7 +78,6 @@
 #cmakedefine01 HAVE_GREGSET_T
 #cmakedefine01 HAVE___GREGSET_T
 #cmakedefine01 HAVE_FPSTATE_GLIBC_RESERVED1
-#cmakedefine01 HAVE_SIGINFO_T
 #cmakedefine01 HAVE_UCONTEXT_T
 #cmakedefine01 HAVE_PTHREAD_RWLOCK_T
 #cmakedefine01 HAVE_PRWATCH_T

src/coreclr/src/pal/src/configure.cmake

@@ -128,7 +128,6 @@ set(CMAKE_EXTRA_INCLUDE_FILES asm/ptrace.h)
 check_type_size("struct pt_regs" PT_REGS)
 set(CMAKE_EXTRA_INCLUDE_FILES)
 set(CMAKE_EXTRA_INCLUDE_FILES signal.h)
-check_type_size(siginfo_t SIGINFO_T)
 set(CMAKE_EXTRA_INCLUDE_FILES)
 set(CMAKE_EXTRA_INCLUDE_FILES ucontext.h)
 check_type_size(ucontext_t UCONTEXT_T)

src/coreclr/src/pal/src/exception/signal.cpp

@@ -23,6 +23,7 @@ SET_DEFAULT_DEBUG_CHANNEL(EXCEPT); // some headers have code with asserts, so do
 
 #include "pal/corunix.hpp"
 #include "pal/handleapi.hpp"
+#include "pal/process.h"
 #include "pal/thread.hpp"
 #include "pal/threadinfo.hpp"
 #include "pal/threadsusp.hpp"
@@ -36,7 +37,6 @@ SET_DEFAULT_DEBUG_CHANNEL(EXCEPT); // some headers have code with asserts, so do
 
 #if !HAVE_MACH_EXCEPTIONS
 #include "pal/init.h"
-#include "pal/process.h"
 #include "pal/debug.h"
 #include "pal/virtual.h"
 #include "pal/utils.h"
@@ -62,12 +62,6 @@ using namespace CorUnix;
 
 /* local type definitions *****************************************************/
 
-#if !HAVE_SIGINFO_T
-/* This allows us to compile on platforms that don't have siginfo_t.
- * Exceptions will work poorly on those platforms. */
-#warning Exceptions will work poorly on this platform
-typedef void *siginfo_t;
-#endif  /* !HAVE_SIGINFO_T */
 typedef void (*SIGFUNC)(int, siginfo_t *, void *);
 
 /* internal function declarations *********************************************/
@@ -91,6 +85,7 @@ static void inject_activation_handler(int code, siginfo_t *siginfo, void *contex
 
 static void handle_signal(int signal_id, SIGFUNC sigfunc, struct sigaction *previousAction, int additionalFlags = 0, bool skipIgnored = false);
 static void restore_signal(int signal_id, struct sigaction *previousAction);
+static void restore_signal_and_resend(int code, struct sigaction* action);
 
 /* internal data declarations *********************************************/
 
@@ -240,6 +235,68 @@ void SEHCleanupSignals()
 /* internal function definitions **********************************************/
 
 #if !HAVE_MACH_EXCEPTIONS
+/*++
+Function :
+    invoke_previous_action
+
+    synchronously invokes the previous action or aborts when that is not possible
+
+Parameters :
+    action  : previous sigaction struct
+    code    : signal code
+    siginfo : signal siginfo
+    context : signal context
+    signalRestarts: BOOL state : TRUE if the process will be signalled again
+
+    (no return value)
+--*/
+static void invoke_previous_action(struct sigaction* action, int code, siginfo_t *siginfo, void *context, bool signalRestarts = true)
+{
+    _ASSERTE(action != NULL);
+
+    if (action->sa_flags & SA_SIGINFO)
+    {
+        // Directly call the previous handler.
+        _ASSERTE(action->sa_sigaction != NULL);
+        action->sa_sigaction(code, siginfo, context);
+    }
+    else
+    {
+        if (action->sa_handler == SIG_IGN)
+        {
+            if (signalRestarts)
+            {
+                // This signal mustn't be ignored because it will be restarted.
+                PROCAbort();
+            }
+            return;
+        }
+        else if (action->sa_handler == SIG_DFL)
+        {
+            if (signalRestarts)
+            {
+                // Restore the original and restart h/w exception.
+                restore_signal(code, action);
+            }
+            else
+            {
+                // We can't invoke the original handler because returning from the
+                // handler doesn't restart the exception.
+                PROCAbort();
+            }
+        }
+        else
+        {
+            // Directly call the previous handler.
+            _ASSERTE(action->sa_handler != NULL);
+            action->sa_handler(code);
+        }
+    }
+
+    PROCNotifyProcessShutdown();
+    PROCCreateCrashDumpIfEnabled();
+}
+
 /*++
 Function :
     sigill_handler
@@ -261,18 +318,7 @@ static void sigill_handler(int code, siginfo_t *siginfo, void *context)
         }
     }
 
-    if (g_previous_sigill.sa_sigaction != NULL)
-    {
-        g_previous_sigill.sa_sigaction(code, siginfo, context);
-    }
-    else
-    {
-        // Restore the original or default handler and restart h/w exception
-        restore_signal(code, &g_previous_sigill);
-    }
-
-    PROCNotifyProcessShutdown();
-    PROCCreateCrashDumpIfEnabled();
+    invoke_previous_action(&g_previous_sigill, code, siginfo, context);
 }
 
 /*++
@@ -296,18 +342,7 @@ static void sigfpe_handler(int code, siginfo_t *siginfo, void *context)
         }
     }
 
-    if (g_previous_sigfpe.sa_sigaction != NULL)
-    {
-        g_previous_sigfpe.sa_sigaction(code, siginfo, context);
-    }
-    else
-    {
-        // Restore the original or default handler and restart h/w exception
-        restore_signal(code, &g_previous_sigfpe);
-    }
-
-    PROCNotifyProcessShutdown();
-    PROCCreateCrashDumpIfEnabled();
+    invoke_previous_action(&g_previous_sigfpe, code, siginfo, context);
 }
 
 /*++
@@ -418,18 +453,7 @@ static void sigsegv_handler(int code, siginfo_t *siginfo, void *context)
         }
     }
 
-    if (g_previous_sigsegv.sa_sigaction != NULL)
-    {
-        g_previous_sigsegv.sa_sigaction(code, siginfo, context);
-    }
-    else
-    {
-        // Restore the original or default handler and restart h/w exception
-        restore_signal(code, &g_previous_sigsegv);
-    }
-
-    PROCNotifyProcessShutdown();
-    PROCCreateCrashDumpIfEnabled();
+    invoke_previous_action(&g_previous_sigsegv, code, siginfo, context);
 }
 
 /*++
@@ -453,19 +477,8 @@ static void sigtrap_handler(int code, siginfo_t *siginfo, void *context)
         }
     }
 
-    if (g_previous_sigtrap.sa_sigaction != NULL)
-    {
-        g_previous_sigtrap.sa_sigaction(code, siginfo, context);
-    }
-    else
-    {
-        // We abort instead of restore the original or default handler and returning
-        // because returning from a SIGTRAP handler continues execution past the trap.
-        PROCAbort();
-    }
-
-    PROCNotifyProcessShutdown();
-    PROCCreateCrashDumpIfEnabled();
+    // The signal doesn't restart, returning from a SIGTRAP handler continues execution past the trap.
+    invoke_previous_action(&g_previous_sigtrap, code, siginfo, context, /* signalRestarts */ false);
 }
 
 /*++
@@ -492,18 +505,7 @@ static void sigbus_handler(int code, siginfo_t *siginfo, void *context)
         }
     }
 
-    if (g_previous_sigbus.sa_sigaction != NULL)
-    {
-        g_previous_sigbus.sa_sigaction(code, siginfo, context);
-    }
-    else
-    {
-        // Restore the original or default handler and restart h/w exception
-        restore_signal(code, &g_previous_sigbus);
-    }
-
-    PROCNotifyProcessShutdown();
-    PROCCreateCrashDumpIfEnabled();
+    invoke_previous_action(&g_previous_sigbus, code, siginfo, context);
 }
 
 /*++
@@ -521,9 +523,7 @@ static void sigint_handler(int code, siginfo_t *siginfo, void *context)
 {
     PROCNotifyProcessShutdown();
 
-    // Restore the original or default handler and resend signal
-    restore_signal(code, &g_previous_sigint);
-    kill(gPID, code);
+    restore_signal_and_resend(code, &g_previous_sigint);
 }
 
 /*++
@@ -541,9 +541,7 @@ static void sigquit_handler(int code, siginfo_t *siginfo, void *context)
 {
     PROCNotifyProcessShutdown();
 
-    // Restore the original or default handler and resend signal
-    restore_signal(code, &g_previous_sigquit);
-    kill(gPID, code);
+    restore_signal_and_resend(code, &g_previous_sigquit);
 }
 #endif // !HAVE_MACH_EXCEPTIONS
 
@@ -569,10 +567,7 @@ static void sigterm_handler(int code, siginfo_t *siginfo, void *context)
     }
     else
     {
-        if (g_previous_sigterm.sa_sigaction != NULL)
-        {
-            g_previous_sigterm.sa_sigaction(code, siginfo, context);
-        }
+        restore_signal_and_resend(SIGTERM, &g_previous_sigterm);
     }
 }
 
@@ -612,9 +607,23 @@ static void inject_activation_handler(int code, siginfo_t *siginfo, void *contex
             CONTEXTToNativeContext(&winContext, ucontext);
         }
     }
-    else if (g_previous_activation.sa_sigaction != NULL)
+    else
     {
-        g_previous_activation.sa_sigaction(code, siginfo, context);
+        // Call the original handler when it is not ignored or default (terminate).
+        if (g_previous_activation.sa_flags & SA_SIGINFO)
+        {
+            _ASSERTE(g_previous_activation.sa_sigaction != NULL);
+            g_previous_activation.sa_sigaction(code, siginfo, context);
+        }
+        else
+        {
+            if (g_previous_activation.sa_handler != SIG_IGN &&
+                g_previous_activation.sa_handler != SIG_DFL)
+            {
+                _ASSERTE(g_previous_activation.sa_handler != NULL);
+                g_previous_activation.sa_handler(code);
+            }
+        }
     }
 }
 #endif
@@ -832,13 +841,9 @@ void handle_signal(int signal_id, SIGFUNC sigfunc, struct sigaction *previousAct
     struct sigaction newAction;
 
     newAction.sa_flags = SA_RESTART | additionalFlags;
-#if HAVE_SIGINFO_T
     newAction.sa_handler = NULL;
     newAction.sa_sigaction = sigfunc;
     newAction.sa_flags |= SA_SIGINFO;
-#else   /* HAVE_SIGINFO_T */
-    newAction.sa_handler = SIG_DFL;
-#endif  /* HAVE_SIGINFO_T */
     sigemptyset(&newAction.sa_mask);
 
 #ifdef INJECT_ACTIVATION_SIGNAL
@@ -891,3 +896,21 @@ void restore_signal(int signal_id, struct sigaction *previousAction)
             errno, strerror(errno));
     }
 }
+
+/*++
+Function :
+    restore_signal_and_resend
+
+    restore handler for specified signal and signal the process
+
+Parameters :
+    int signal_id : signal to handle
+    previousAction : previous sigaction struct to restore
+
+    (no return value)
+--*/
+void restore_signal_and_resend(int signal_id, struct sigaction* previousAction)
+{
+    restore_signal(signal_id, previousAction);
+    kill(gPID, signal_id);
+}

src/coreclr/src/pal/src/synchmgr/synchmanager.cpp

@@ -1657,7 +1657,11 @@ namespace CorUnix
         // Call the termination request handler if one is registered.
         if (g_terminationRequestHandler != NULL)
         {
-            g_terminationRequestHandler();
+            // The process will terminate normally by calling exit.
+            // We use an exit code of '128 + signo'. This is a convention used in popular
+            // shells to calculate an exit code when the process was terminated by a signal.
+            // This is also used by the Process.ExitCode implementation.
+            g_terminationRequestHandler(128 + SIGTERM);
         }
 
         return 0;

src/coreclr/src/vm/exceptionhandling.cpp

@@ -16,6 +16,7 @@
 #include "perfcounters.h"
 #include "eventtrace.h"
 #include "virtualcallstub.h"
+#include "utilcode.h"
 
 #if defined(_TARGET_X86_)
 #define USE_CURRENT_CONTEXT_IN_FILTER
@@ -203,10 +204,23 @@ static inline void UpdatePerformanceMetrics(CrawlFrame *pcfThisFrame, BOOL bIsRe
     ETW::ExceptionLog::ExceptionThrown(pcfThisFrame, bIsRethrownException, bIsNewException);
 }
 
-void ShutdownEEAndExitProcess()
+#ifdef FEATURE_PAL
+static LONG volatile g_termination_triggered = 0;
+
+void HandleTerminationRequest(int terminationExitCode)
 {
-    ForceEEShutdown(SCA_ExitProcessWhenShutdownComplete);
+    // We set a non-zero exit code to indicate the process didn't terminate cleanly.
+    // This value can be changed by the user by setting Environment.ExitCode in the
+    // ProcessExit event. We only start termination on the first SIGTERM signal
+    // to ensure we don't overwrite an exit code already set in ProcessExit.
+    if (InterlockedCompareExchange(&g_termination_triggered, 1, 0) == 0)
+    {
+        SetLatchedExitCode(terminationExitCode);
+
+        ForceEEShutdown(SCA_ExitProcessWhenShutdownComplete);
+    }
 }
+#endif
 
 void InitializeExceptionHandling()
 {
@@ -229,7 +243,7 @@ void InitializeExceptionHandling()
     PAL_SetGetGcMarkerExceptionCode(GetGcMarkerExceptionCode);
 
     // Register handler for termination requests (e.g. SIGTERM)
-    PAL_SetTerminationRequestHandler(ShutdownEEAndExitProcess);
+    PAL_SetTerminationRequestHandler(HandleTerminationRequest);
 #endif // FEATURE_PAL
 }