diff options
author | Tom Deseyn <tom.deseyn@gmail.com> | 2019-01-15 21:43:08 +0100 |
---|---|---|
committer | Jan Kotas <jkotas@microsoft.com> | 2019-01-15 12:43:08 -0800 |
commit | 3f0903598d0e269e6aaf8e237b47269abe4ab7e8 (patch) | |
tree | aff29f9a428caf4b1591987dff07fd146d15909b | |
parent | 86b4ac59ef9d19e8f635086ea6a20a291c1dd26a (diff) | |
download | coreclr-3f0903598d0e269e6aaf8e237b47269abe4ab7e8.tar.gz coreclr-3f0903598d0e269e6aaf8e237b47269abe4ab7e8.tar.bz2 coreclr-3f0903598d0e269e6aaf8e237b47269abe4ab7e8.zip |
On SIGTERM default to a non-zero exit code (#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
-rw-r--r-- | src/pal/inc/pal.h | 2 | ||||
-rw-r--r-- | src/pal/src/config.h.in | 1 | ||||
-rw-r--r-- | src/pal/src/configure.cmake | 1 | ||||
-rw-r--r-- | src/pal/src/exception/signal.cpp | 191 | ||||
-rw-r--r-- | src/pal/src/synchmgr/synchmanager.cpp | 6 | ||||
-rw-r--r-- | src/vm/exceptionhandling.cpp | 20 |
6 files changed, 130 insertions, 91 deletions
diff --git a/src/pal/inc/pal.h b/src/pal/inc/pal.h index d49cb13589..76e04cdbdb 100644 --- a/src/pal/inc/pal.h +++ b/src/pal/inc/pal.h @@ -5057,7 +5057,7 @@ public: 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 diff --git a/src/pal/src/config.h.in b/src/pal/src/config.h.in index 1ed071e33f..d0e14df13a 100644 --- a/src/pal/src/config.h.in +++ b/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 diff --git a/src/pal/src/configure.cmake b/src/pal/src/configure.cmake index d105d851e3..c8b6f3bd6e 100644 --- a/src/pal/src/configure.cmake +++ b/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) diff --git a/src/pal/src/exception/signal.cpp b/src/pal/src/exception/signal.cpp index fe15a70f83..df5a5f22a4 100644 --- a/src/pal/src/exception/signal.cpp +++ b/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 *********************************************/ @@ -242,6 +237,68 @@ void SEHCleanupSignals() #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 handle SIGILL signal (EXCEPTION_ILLEGAL_INSTRUCTION, others?) @@ -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); +}
\ No newline at end of file diff --git a/src/pal/src/synchmgr/synchmanager.cpp b/src/pal/src/synchmgr/synchmanager.cpp index 349b3de135..1bdfb9426b 100644 --- a/src/pal/src/synchmgr/synchmanager.cpp +++ b/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; diff --git a/src/vm/exceptionhandling.cpp b/src/vm/exceptionhandling.cpp index 869084cff9..8e6da16e8b 100644 --- a/src/vm/exceptionhandling.cpp +++ b/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 } |