From 0761780988b93a4d1cc98dc5d4e2d84db29e33fd Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Mon, 3 May 2021 17:31:40 -0700 Subject: [PATCH] Generate coredumps on signals (#52024) Generate coredumps on signals On MacOS, coredumps are generated if there is an unhandled managed exception but signals don't generate them. Enable a stripped down version of the Linux signal handlers for MacOS. Add a SIGABRT handler so calls to abort() in other native code causes dump to be generated. Add COMPlus_EnableDumpOnSigTerm that enables dump generation on SIGTERM. Moved the "generate dump on SIGTERM" logic out of the PAL into runtime's HandleTerminationRequest callback and passed a new SCA enum down to SafeExitProcess that calls TerminateProcess instead of ExitProcess which generates a core dump. --- src/coreclr/inc/clrconfigvalues.h | 1 + .../pal/src/exception/machexception.cpp | 7 -- src/coreclr/pal/src/exception/machmessage.h | 5 +- src/coreclr/pal/src/exception/signal.cpp | 85 ++++++++++++++----- src/coreclr/pal/src/include/pal/signal.hpp | 10 +++ src/coreclr/pal/src/thread/process.cpp | 6 +- src/coreclr/vm/ceemain.h | 5 +- src/coreclr/vm/eepolicy.cpp | 29 +++---- src/coreclr/vm/exceptionhandling.cpp | 3 +- 9 files changed, 105 insertions(+), 46 deletions(-) diff --git a/src/coreclr/inc/clrconfigvalues.h b/src/coreclr/inc/clrconfigvalues.h index 2a0de94724c9a..93aecaa8b45cb 100644 --- a/src/coreclr/inc/clrconfigvalues.h +++ b/src/coreclr/inc/clrconfigvalues.h @@ -663,6 +663,7 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_DbgEnableMiniDump, W("DbgEnableMiniDump"), 0, RETAIL_CONFIG_STRING_INFO(INTERNAL_DbgMiniDumpName, W("DbgMiniDumpName"), "Crash dump name") RETAIL_CONFIG_DWORD_INFO(INTERNAL_DbgMiniDumpType, W("DbgMiniDumpType"), 0, "Crash dump type: 1 normal, 2 withheap, 3 triage, 4 full") RETAIL_CONFIG_DWORD_INFO(INTERNAL_CreateDumpDiagnostics, W("CreateDumpDiagnostics"), 0, "Enable crash dump generation diagnostic logging") +RETAIL_CONFIG_DWORD_INFO(INTERNAL_EnableDumpOnSigTerm, W("EnableDumpOnSigTerm"), 0, "Enable crash dump generation on SIGTERM") /// /// Zap diff --git a/src/coreclr/pal/src/exception/machexception.cpp b/src/coreclr/pal/src/exception/machexception.cpp index f9973b74e19fe..e5aebdf652c6a 100644 --- a/src/coreclr/pal/src/exception/machexception.cpp +++ b/src/coreclr/pal/src/exception/machexception.cpp @@ -1411,13 +1411,6 @@ SEHInitializeMachExceptions(DWORD flags) #endif // _DEBUG } - // Tell the system to ignore SIGPIPE signals rather than use the default - // behavior of terminating the process. Ignoring SIGPIPE will cause - // calls that would otherwise raise that signal to return EPIPE instead. - // The PAL expects EPIPE from those functions and won't handle a - // SIGPIPE signal. - signal(SIGPIPE, SIG_IGN); - // We're done return TRUE; } diff --git a/src/coreclr/pal/src/exception/machmessage.h b/src/coreclr/pal/src/exception/machmessage.h index ff288ad6f25b2..c24a979f52089 100644 --- a/src/coreclr/pal/src/exception/machmessage.h +++ b/src/coreclr/pal/src/exception/machmessage.h @@ -44,7 +44,8 @@ using namespace CorUnix; // This macro terminates the process with some useful debug info as above, but for the general failure points // that have nothing to do with Mach. #define NONPAL_RETAIL_ASSERT(_msg, ...) do { \ - printf("%s: %u: " _msg "\n", __FUNCTION__, __LINE__, ## __VA_ARGS__); \ + fprintf(stdout, "%s: %u: " _msg "\n", __FUNCTION__, __LINE__, ## __VA_ARGS__); \ + fflush(stdout); \ abort(); \ } while (false) @@ -67,7 +68,7 @@ using namespace CorUnix; // Debug-only output with printf-style formatting. #define NONPAL_TRACE(_format, ...) do { \ - if (NONPAL_TRACE_ENABLED) printf("NONPAL_TRACE: " _format, ## __VA_ARGS__); \ + if (NONPAL_TRACE_ENABLED) { fprintf(stdout, "NONPAL_TRACE: " _format, ## __VA_ARGS__); fflush(stdout); } \ } while (false) #else // _DEBUG diff --git a/src/coreclr/pal/src/exception/signal.cpp b/src/coreclr/pal/src/exception/signal.cpp index a942d1168a7c6..9ab675c8df6dc 100644 --- a/src/coreclr/pal/src/exception/signal.cpp +++ b/src/coreclr/pal/src/exception/signal.cpp @@ -72,7 +72,7 @@ static void sigterm_handler(int code, siginfo_t *siginfo, void *context); #ifdef INJECT_ACTIVATION_SIGNAL static void inject_activation_handler(int code, siginfo_t *siginfo, void *context); #endif -#if !HAVE_MACH_EXCEPTIONS + static void sigill_handler(int code, siginfo_t *siginfo, void *context); static void sigfpe_handler(int code, siginfo_t *siginfo, void *context); static void sigsegv_handler(int code, siginfo_t *siginfo, void *context); @@ -80,19 +80,18 @@ static void sigtrap_handler(int code, siginfo_t *siginfo, void *context); static void sigbus_handler(int code, siginfo_t *siginfo, void *context); static void sigint_handler(int code, siginfo_t *siginfo, void *context); static void sigquit_handler(int code, siginfo_t *siginfo, void *context); +static void sigabrt_handler(int code, siginfo_t *siginfo, void *context); static bool common_signal_handler(int code, siginfo_t *siginfo, void *sigcontext, int numParams, ...); -#endif // !HAVE_MACH_EXCEPTIONS - 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 *********************************************/ -#if !HAVE_MACH_EXCEPTIONS bool g_registered_signal_handlers = false; +#if !HAVE_MACH_EXCEPTIONS bool g_enable_alternate_stack_check = false; #endif // !HAVE_MACH_EXCEPTIONS @@ -104,7 +103,6 @@ struct sigaction g_previous_sigterm; struct sigaction g_previous_activation; #endif -#if !HAVE_MACH_EXCEPTIONS struct sigaction g_previous_sigill; struct sigaction g_previous_sigtrap; struct sigaction g_previous_sigfpe; @@ -112,6 +110,9 @@ struct sigaction g_previous_sigbus; struct sigaction g_previous_sigsegv; struct sigaction g_previous_sigint; struct sigaction g_previous_sigquit; +struct sigaction g_previous_sigabrt; + +#if !HAVE_MACH_EXCEPTIONS // Offset of the local variable containing pointer to windows style context in the common_signal_handler function. // This offset is relative to the frame pointer. @@ -141,13 +142,13 @@ Return : --*/ BOOL SEHInitializeSignals(CorUnix::CPalThread *pthrCurrent, DWORD flags) { - TRACE("Initializing signal handlers\n"); + TRACE("Initializing signal handlers %04x\n", flags); #if !HAVE_MACH_EXCEPTIONS - char* enableAlternateStackCheck = getenv("COMPlus_EnableAlternateStackCheck"); g_enable_alternate_stack_check = enableAlternateStackCheck && (strtoul(enableAlternateStackCheck, NULL, 10) != 0); +#endif if (flags & PAL_INITIALIZE_REGISTER_SIGNALS) { @@ -167,17 +168,22 @@ BOOL SEHInitializeSignals(CorUnix::CPalThread *pthrCurrent, DWORD flags) see sigaction man page for more details */ handle_signal(SIGILL, sigill_handler, &g_previous_sigill); - handle_signal(SIGTRAP, sigtrap_handler, &g_previous_sigtrap); handle_signal(SIGFPE, sigfpe_handler, &g_previous_sigfpe); handle_signal(SIGBUS, sigbus_handler, &g_previous_sigbus); - // SIGSEGV handler runs on a separate stack so that we can handle stack overflow - handle_signal(SIGSEGV, sigsegv_handler, &g_previous_sigsegv, SA_ONSTACK); + handle_signal(SIGABRT, sigabrt_handler, &g_previous_sigabrt); // We don't setup a handler for SIGINT/SIGQUIT when those signals are ignored. // Otherwise our child processes would reset to the default on exec causing them // to terminate on these signals. handle_signal(SIGINT, sigint_handler, &g_previous_sigint, 0 /* additionalFlags */, true /* skipIgnored */); handle_signal(SIGQUIT, sigquit_handler, &g_previous_sigquit, 0 /* additionalFlags */, true /* skipIgnored */); +#if HAVE_MACH_EXCEPTIONS + handle_signal(SIGSEGV, sigsegv_handler, &g_previous_sigsegv); +#else + handle_signal(SIGTRAP, sigtrap_handler, &g_previous_sigtrap); + // SIGSEGV handler runs on a separate stack so that we can handle stack overflow + handle_signal(SIGSEGV, sigsegv_handler, &g_previous_sigsegv, SA_ONSTACK); + if (!pthrCurrent->EnsureSignalAlternateStack()) { return FALSE; @@ -206,6 +212,7 @@ BOOL SEHInitializeSignals(CorUnix::CPalThread *pthrCurrent, DWORD flags) } g_stackOverflowHandlerStack = (void*)((size_t)g_stackOverflowHandlerStack + stackOverflowStackSize); +#endif // HAVE_MACH_EXCEPTIONS } /* The default action for SIGPIPE is process termination. @@ -217,7 +224,6 @@ BOOL SEHInitializeSignals(CorUnix::CPalThread *pthrCurrent, DWORD flags) issued a SIGPIPE will, instead, report an error and set errno to EPIPE. */ signal(SIGPIPE, SIG_IGN); -#endif // !HAVE_MACH_EXCEPTIONS if (flags & PAL_INITIALIZE_REGISTER_SIGTERM_HANDLER) { @@ -253,18 +259,19 @@ void SEHCleanupSignals() { TRACE("Restoring default signal handlers\n"); -#if !HAVE_MACH_EXCEPTIONS if (g_registered_signal_handlers) { restore_signal(SIGILL, &g_previous_sigill); +#if !HAVE_MACH_EXCEPTIONS restore_signal(SIGTRAP, &g_previous_sigtrap); +#endif restore_signal(SIGFPE, &g_previous_sigfpe); restore_signal(SIGBUS, &g_previous_sigbus); + restore_signal(SIGABRT, &g_previous_sigabrt); restore_signal(SIGSEGV, &g_previous_sigsegv); restore_signal(SIGINT, &g_previous_sigint); restore_signal(SIGQUIT, &g_previous_sigquit); } -#endif // !HAVE_MACH_EXCEPTIONS #ifdef INJECT_ACTIVATION_SIGNAL if (g_registered_activation_handler) @@ -279,9 +286,23 @@ void SEHCleanupSignals() } } -/* internal function definitions **********************************************/ +/*++ +Function : + SEHCleanupAbort() -#if !HAVE_MACH_EXCEPTIONS + Restore default SIGABORT signal handlers + + (no parameters, no return value) +--*/ +void SEHCleanupAbort() +{ + if (g_registered_signal_handlers) + { + restore_signal(SIGABRT, &g_previous_sigabrt); + } +} + +/* internal function definitions **********************************************/ /*++ Function : @@ -298,6 +319,9 @@ Return : --*/ bool IsRunningOnAlternateStack(void *context) { +#if HAVE_MACH_EXCEPTIONS + return false; +#else bool isRunningOnAlternateStack; if (g_enable_alternate_stack_check) { @@ -317,6 +341,7 @@ bool IsRunningOnAlternateStack(void *context) } return isRunningOnAlternateStack; +#endif // HAVE_MACH_EXCEPTIONS } /*++ @@ -429,6 +454,8 @@ static void sigfpe_handler(int code, siginfo_t *siginfo, void *context) invoke_previous_action(&g_previous_sigfpe, code, siginfo, context); } +#if !HAVE_MACH_EXCEPTIONS + /*++ Function : signal_handler_worker @@ -506,6 +533,8 @@ static bool SwitchStackAndExecuteHandler(int code, siginfo_t *siginfo, void *con return pReturnPoint->returnFromHandler; } +#endif // !HAVE_MACH_EXCEPTIONS + /*++ Function : sigsegv_handler @@ -519,6 +548,7 @@ Parameters : --*/ static void sigsegv_handler(int code, siginfo_t *siginfo, void *context) { +#if !HAVE_MACH_EXCEPTIONS if (PALIsInitialized()) { // First check if we have a stack overflow @@ -578,6 +608,7 @@ static void sigsegv_handler(int code, siginfo_t *siginfo, void *context) } } } +#endif // !HAVE_MACH_EXCEPTIONS invoke_previous_action(&g_previous_sigsegv, code, siginfo, context); } @@ -634,6 +665,22 @@ static void sigbus_handler(int code, siginfo_t *siginfo, void *context) invoke_previous_action(&g_previous_sigbus, code, siginfo, context); } +/*++ +Function : + sigabrt_handler + + handle SIGABRT signal - abort() API + +Parameters : + POSIX signal handler parameter list ("man sigaction" for details) + + (no return value) +--*/ +static void sigabrt_handler(int code, siginfo_t *siginfo, void *context) +{ + invoke_previous_action(&g_previous_sigabrt, code, siginfo, context); +} + /*++ Function : sigint_handler @@ -669,7 +716,6 @@ static void sigquit_handler(int code, siginfo_t *siginfo, void *context) restore_signal_and_resend(code, &g_previous_sigquit); } -#endif // !HAVE_MACH_EXCEPTIONS /*++ Function : @@ -795,8 +841,8 @@ PAL_ERROR InjectActivationInternal(CorUnix::CPalThread* pThread) #endif } -#if !HAVE_MACH_EXCEPTIONS +#if !HAVE_MACH_EXCEPTIONS /*++ Function : signal_ignore_handler @@ -811,6 +857,7 @@ Parameters : static void signal_ignore_handler(int code, siginfo_t *siginfo, void *context) { } +#endif // !HAVE_MACH_EXCEPTIONS void PAL_IgnoreProfileSignal(int signalNum) @@ -854,6 +901,7 @@ Parameters : __attribute__((noinline)) static bool common_signal_handler(int code, siginfo_t *siginfo, void *sigcontext, int numParams, ...) { +#if !HAVE_MACH_EXCEPTIONS sigset_t signal_set; CONTEXT signalContextRecord; EXCEPTION_RECORD exceptionRecord; @@ -919,10 +967,9 @@ static bool common_signal_handler(int code, siginfo_t *siginfo, void *sigcontext CONTEXTToNativeContext(exception.ExceptionPointers.ContextRecord, ucontext); return true; } - +#endif // !HAVE_MACH_EXCEPTIONS return false; } -#endif // !HAVE_MACH_EXCEPTIONS /*++ Function : diff --git a/src/coreclr/pal/src/include/pal/signal.hpp b/src/coreclr/pal/src/include/pal/signal.hpp index c5a1e276a05b5..a1b721b8fd3ce 100644 --- a/src/coreclr/pal/src/include/pal/signal.hpp +++ b/src/coreclr/pal/src/include/pal/signal.hpp @@ -117,4 +117,14 @@ Function : --*/ void SEHCleanupSignals(); +/*++ +Function : + SEHCleanupAbort() + + Restore default SIGABORT signal handlers + + (no parameters, no return value) +--*/ +void SEHCleanupAbort(); + #endif /* _PAL_SIGNAL_HPP_ */ diff --git a/src/coreclr/pal/src/thread/process.cpp b/src/coreclr/pal/src/thread/process.cpp index 464013243d4e9..54e08b7abd901 100644 --- a/src/coreclr/pal/src/thread/process.cpp +++ b/src/coreclr/pal/src/thread/process.cpp @@ -35,6 +35,7 @@ SET_DEFAULT_DEBUG_CHANNEL(PROCESS); // some headers have code with asserts, so d #include "pal/environ.h" #include "pal/virtual.h" #include "pal/stackstring.hpp" +#include "pal/signal.hpp" #include #if HAVE_POLL @@ -3257,9 +3258,9 @@ PROCAbortInitialize() char* dumpType = getenv("COMPlus_DbgMiniDumpType"); char* diagStr = getenv("COMPlus_CreateDumpDiagnostics"); BOOL diag = diagStr != nullptr && strcmp(diagStr, "1") == 0; + char* program = nullptr; char* pidarg = nullptr; - if (!PROCBuildCreateDumpCommandLine((const char **)g_argvCreateDump, &program, &pidarg, dumpName, dumpType, diag)) { return FALSE; @@ -3359,6 +3360,9 @@ PROCAbort() PROCCreateCrashDumpIfEnabled(); + // Restore the SIGABORT handler to prevent recursion + SEHCleanupAbort(); + // Abort the process after waiting for the core dump to complete abort(); } diff --git a/src/coreclr/vm/ceemain.h b/src/coreclr/vm/ceemain.h index 781b07097f36b..1f96cdea85f38 100644 --- a/src/coreclr/vm/ceemain.h +++ b/src/coreclr/vm/ceemain.h @@ -26,13 +26,16 @@ HRESULT EnsureEEStarted(); // shutdown normally ends. "Shutdown" methods that take this action as an argument // do not return when SCA_ExitProcessWhenShutdownComplete is passed. // -// 2. Return after performing all shutdown processing. This is a special case used +// 2. Terminate process and generate a dump if enabled. +// +// 3. Return after performing all shutdown processing. This is a special case used // by a shutdown initiated via the Shim, and is used to ensure that all runtimes // loaded SxS are shutdown gracefully. "Shutdown" methods that take this action // as an argument return when SCA_ReturnWhenShutdownComplete is passed. enum ShutdownCompleteAction { SCA_ExitProcessWhenShutdownComplete, + SCA_TerminateProcessWhenShutdownComplete, SCA_ReturnWhenShutdownComplete }; diff --git a/src/coreclr/vm/eepolicy.cpp b/src/coreclr/vm/eepolicy.cpp index 8abaf6f4ee7c8..1c250c7d40a09 100644 --- a/src/coreclr/vm/eepolicy.cpp +++ b/src/coreclr/vm/eepolicy.cpp @@ -27,9 +27,9 @@ #include "eventtrace.h" #undef ExitProcess -void SafeExitProcess(UINT exitCode, BOOL fAbort = FALSE, ShutdownCompleteAction sca = SCA_ExitProcessWhenShutdownComplete) +void SafeExitProcess(UINT exitCode, ShutdownCompleteAction sca = SCA_ExitProcessWhenShutdownComplete) { - STRESS_LOG2(LF_SYNC, LL_INFO10, "SafeExitProcess: exitCode = %d, fAbort = %d\n", exitCode, fAbort); + STRESS_LOG2(LF_SYNC, LL_INFO10, "SafeExitProcess: exitCode = %d sca = %d\n", exitCode, sca); CONTRACTL { DISABLED(GC_TRIGGERS); @@ -87,20 +87,19 @@ void SafeExitProcess(UINT exitCode, BOOL fAbort = FALSE, ShutdownCompleteAction g_fNoExceptions = true; LOG((LF_EH, LL_INFO10, "SafeExitProcess: turning off exceptions\n")); - if (sca == SCA_ExitProcessWhenShutdownComplete) + if (sca == SCA_TerminateProcessWhenShutdownComplete) { - // disabled because if we fault in this code path we will trigger our - // Watson code + // disabled because if we fault in this code path we will trigger our Watson code CONTRACT_VIOLATION(ThrowsViolation); - if (fAbort) - { - CrashDumpAndTerminateProcess(exitCode); - } - else - { - ExitProcess(exitCode); - } + CrashDumpAndTerminateProcess(exitCode); + } + else if (sca == SCA_ExitProcessWhenShutdownComplete) + { + // disabled because if we fault in this code path we will trigger our Watson code + CONTRACT_VIOLATION(ThrowsViolation); + + ExitProcess(exitCode); } } @@ -179,7 +178,7 @@ void EEPolicy::HandleExitProcess(ShutdownCompleteAction sca) { EEShutDown(FALSE); } - SafeExitProcess(GetLatchedExitCode(), FALSE, sca); + SafeExitProcess(GetLatchedExitCode(), sca); } @@ -797,7 +796,7 @@ int NOINLINE EEPolicy::HandleFatalError(UINT exitCode, UINT_PTR address, LPCWSTR STRESS_LOG0(LF_CORDB,LL_INFO100, "D::HFE: About to call LogFatalError\n"); LogFatalError(exitCode, address, pszMessage, pExceptionInfo, errorSource, argExceptionString); - SafeExitProcess(exitCode, TRUE); + SafeExitProcess(exitCode, SCA_TerminateProcessWhenShutdownComplete); } UNREACHABLE(); diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index e65b1cb96d513..03e87dd088742 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -212,7 +212,8 @@ void HandleTerminationRequest(int terminationExitCode) { SetLatchedExitCode(terminationExitCode); - ForceEEShutdown(SCA_ExitProcessWhenShutdownComplete); + DWORD enabled = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_EnableDumpOnSigTerm); + ForceEEShutdown(enabled == 1 ? SCA_TerminateProcessWhenShutdownComplete : SCA_ExitProcessWhenShutdownComplete); } } #endif