From c0d0a50bdb2ae2f749443c0386c2b25379bdbf76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Sat, 27 Mar 2021 17:54:22 +0100 Subject: [PATCH] Significantly refactor fatal error handling Because new glibc has changed `MINSIGSTKSZ` to be a syscall instead of being constant, the signal posix handling needed changes, as it used the value in constexpr context, for deciding size of an array. It would be simple to fix it by having the handler determine the signal handling stack size and allocate the memory every time the handler is being installed, but that would add another allocation and a syscall every time a test case is entered. Instead, I split apart the idea of preparing fatal error handlers, and engaging them, so that the memory can be allocated only once and still be guarded by RAII. Also turns out that Catch2's use of `MINSIGSTKSZ` was wrong, and we should've been using `SIGSTKSZ` the whole time, which we use now. Closes #2178 --- .../catch_fatal_condition_handler.cpp | 196 +++++++++++++----- .../catch_fatal_condition_handler.hpp | 89 ++++---- src/catch2/internal/catch_run_context.cpp | 4 +- src/catch2/internal/catch_run_context.hpp | 2 + 4 files changed, 190 insertions(+), 101 deletions(-) diff --git a/src/catch2/internal/catch_fatal_condition_handler.cpp b/src/catch2/internal/catch_fatal_condition_handler.cpp index 1b1e5c7fd2..6d0f46bff0 100644 --- a/src/catch2/internal/catch_fatal_condition_handler.cpp +++ b/src/catch2/internal/catch_fatal_condition_handler.cpp @@ -5,30 +5,73 @@ // https://www.boost.org/LICENSE_1_0.txt) // SPDX-License-Identifier: BSL-1.0 + +/** \file + * This file provides platform specific implementations of FatalConditionHandler + * + * This means that there is a lot of conditional compilation, and platform + * specific code. Currently, Catch2 supports a dummy handler (if no + * handler is desired), and 2 platform specific handlers: + * * Windows' SEH + * * POSIX signals + * + * Consequently, various pieces of code below are compiled if either of + * the platform specific handlers is enabled, or if none of them are + * enabled. It is assumed that both cannot be enabled at the same time, + * and doing so should cause a compilation error. + * + * If another platform specific handler is added, the compile guards + * below will need to be updated taking these assumptions into account. + */ + #include #include +#include #include +#include -#if defined(__GNUC__) -# pragma GCC diagnostic push -# pragma GCC diagnostic ignored "-Wmissing-field-initializers" -#endif +#include + +#if !defined( CATCH_CONFIG_WINDOWS_SEH ) && !defined( CATCH_CONFIG_POSIX_SIGNALS ) + +namespace Catch { + + // If neither SEH nor signal handling is required, the handler impls + // do not have to do anything, and can be empty. + FatalConditionHandler::engage_platform() {} + FatalConditionHandler::disengage_platform() {} + FatalConditionHandler::FatalConditionHandler() = default; + FatalConditionHandler::~FatalConditionHandler() = default; + +} // end namespace Catch + +#endif // !CATCH_CONFIG_WINDOWS_SEH && !CATCH_CONFIG_POSIX_SIGNALS + +#if defined( CATCH_CONFIG_WINDOWS_SEH ) && defined( CATCH_CONFIG_POSIX_SIGNALS ) +#error "Inconsistent configuration: Windows' SEH handling and POSIX signals cannot be enabled at the same time" +#endif // CATCH_CONFIG_WINDOWS_SEH && CATCH_CONFIG_POSIX_SIGNALS #if defined( CATCH_CONFIG_WINDOWS_SEH ) || defined( CATCH_CONFIG_POSIX_SIGNALS ) namespace { - // Report the error condition + //! Signals fatal error message to the run context void reportFatal( char const * const message ) { Catch::getCurrentContext().getResultCapture()->handleFatalErrorCondition( message ); } -} -#endif // signals/SEH handling + //! Minimal size Catch2 needs for its own fatal error handling. + //! Picked empirically, so it might not be sufficient on all + //! platforms, and for all configurations. + constexpr std::size_t minStackSizeForErrors = 32 * 1024; +} // end unnamed namespace + +#endif // CATCH_CONFIG_WINDOWS_SEH || CATCH_CONFIG_POSIX_SIGNALS #if defined( CATCH_CONFIG_WINDOWS_SEH ) namespace Catch { + struct SignalDefs { DWORD id; const char* name; }; // There is no 1-1 mapping between signals and windows exceptions. @@ -41,7 +84,7 @@ namespace Catch { { static_cast(EXCEPTION_INT_DIVIDE_BY_ZERO), "Divide by zero error" }, }; - LONG CALLBACK FatalConditionHandler::handleVectoredException(PEXCEPTION_POINTERS ExceptionInfo) { + static LONG CALLBACK handleVectoredException(PEXCEPTION_POINTERS ExceptionInfo) { for (auto const& def : signalDefs) { if (ExceptionInfo->ExceptionRecord->ExceptionCode == def.id) { reportFatal(def.name); @@ -52,35 +95,52 @@ namespace Catch { return EXCEPTION_CONTINUE_SEARCH; } + // Since we do not support multiple instantiations, we put these + // into global variables and rely on cleaning them up in outlined + // constructors/destructors + static PVOID exceptionHandlerHandle = nullptr; + + + // For MSVC, we reserve part of the stack memory for handling + // memory overflow structured exception. FatalConditionHandler::FatalConditionHandler() { - isSet = true; - // 32k seems enough for Catch to handle stack overflow, - // but the value was found experimentally, so there is no strong guarantee - guaranteeSize = 32 * 1024; - exceptionHandlerHandle = nullptr; + ULONG guaranteeSize = static_cast(minStackSizeForErrors); + if (!SetThreadStackGuarantee(&guaranteeSize)) { + // We do not want to fully error out, because needing + // the stack reserve should be rare enough anyway. + Catch::cerr() + << "Failed to reserve piece of stack." + << " Stack overflows will not be reported successfully."; + } + } + + // We do not attempt to unset the stack guarantee, because + // Windows does not support lowering the stack size guarantee. + FatalConditionHandler::~FatalConditionHandler() = default; + + + void FatalConditionHandler::engage_platform() { // Register as first handler in current chain exceptionHandlerHandle = AddVectoredExceptionHandler(1, handleVectoredException); - // Pass in guarantee size to be filled - SetThreadStackGuarantee(&guaranteeSize); + if (!exceptionHandlerHandle) { + CATCH_RUNTIME_ERROR("Could not register vectored exception handler"); + } } - void FatalConditionHandler::reset() { - if (isSet) { - RemoveVectoredExceptionHandler(exceptionHandlerHandle); - SetThreadStackGuarantee(&guaranteeSize); - exceptionHandlerHandle = nullptr; - isSet = false; + void FatalConditionHandler::disengage_platform() { + if (!RemoveVectoredExceptionHandler(exceptionHandlerHandle)) { + CATCH_RUNTIME_ERROR("Could not unregister vectored exception handler"); } + exceptionHandlerHandle = nullptr; } -bool FatalConditionHandler::isSet = false; -ULONG FatalConditionHandler::guaranteeSize = 0; -PVOID FatalConditionHandler::exceptionHandlerHandle = nullptr; +} // end namespace Catch +#endif // CATCH_CONFIG_WINDOWS_SEH -} // namespace Catch +#if defined( CATCH_CONFIG_POSIX_SIGNALS ) -#elif defined( CATCH_CONFIG_POSIX_SIGNALS ) +#include namespace Catch { @@ -89,10 +149,6 @@ namespace Catch { const char* name; }; - // 32kb for the alternate stack seems to be sufficient. However, this value - // is experimentally determined, so that's not guaranteed. - static constexpr std::size_t sigStackSize = 32768 >= MINSIGSTKSZ ? 32768 : MINSIGSTKSZ; - static SignalDefs signalDefs[] = { { SIGINT, "SIGINT - Terminal interrupt signal" }, { SIGILL, "SIGILL - Illegal instruction signal" }, @@ -102,8 +158,32 @@ namespace Catch { { SIGABRT, "SIGABRT - Abort (abnormal termination) signal" } }; +// Older GCCs trigger -Wmissing-field-initializers for T foo = {} +// which is zero initialization, but not explicit. We want to avoid +// that. +#if defined(__GNUC__) +# pragma GCC diagnostic push +# pragma GCC diagnostic ignored "-Wmissing-field-initializers" +#endif + + static char* altStackMem = nullptr; + static std::size_t altStackSize = 0; + static stack_t oldSigStack{}; + static struct sigaction oldSigActions[sizeof(signalDefs) / sizeof(SignalDefs)]{}; + + static void restorePreviousSignalHandlers() { + // We set signal handlers back to the previous ones. Hopefully + // nobody overwrote them in the meantime, and doesn't expect + // their signal handlers to live past ours given that they + // installed them after ours.. + for (std::size_t i = 0; i < sizeof(signalDefs) / sizeof(SignalDefs); ++i) { + sigaction(signalDefs[i].id, &oldSigActions[i], nullptr); + } + // Return the old stack + sigaltstack(&oldSigStack, nullptr); + } - void FatalConditionHandler::handleSignal( int sig ) { + static void handleSignal( int sig ) { char const * name = ""; for (auto const& def : signalDefs) { if (sig == def.id) { @@ -111,16 +191,33 @@ namespace Catch { break; } } - reset(); - reportFatal(name); + // We need to restore previous signal handlers and let them do + // their thing, so that the users can have the debugger break + // when a signal is raised, and so on. + restorePreviousSignalHandlers(); + reportFatal( name ); raise( sig ); } FatalConditionHandler::FatalConditionHandler() { - isSet = true; + assert(!altStackMem && "Cannot initialize POSIX signal handler when one already exists"); + if (altStackSize == 0) { + altStackSize = std::max(static_cast(SIGSTKSZ), minStackSizeForErrors); + } + altStackMem = new char[altStackSize](); + } + + FatalConditionHandler::~FatalConditionHandler() { + delete[] altStackMem; + // We signal that another instance can be constructed by zeroing + // out the pointer. + altStackMem = nullptr; + } + + void FatalConditionHandler::engage_platform() { stack_t sigStack; sigStack.ss_sp = altStackMem; - sigStack.ss_size = sigStackSize; + sigStack.ss_size = altStackSize; sigStack.ss_flags = 0; sigaltstack(&sigStack, &oldSigStack); struct sigaction sa = { }; @@ -132,28 +229,15 @@ namespace Catch { } } - void FatalConditionHandler::reset() { - if( isSet ) { - // Set signals back to previous values -- hopefully nobody overwrote them in the meantime - for( std::size_t i = 0; i < sizeof(signalDefs)/sizeof(SignalDefs); ++i ) { - sigaction(signalDefs[i].id, &oldSigActions[i], nullptr); - } - // Return the old stack - sigaltstack(&oldSigStack, nullptr); - isSet = false; - } - } - - bool FatalConditionHandler::isSet = false; - struct sigaction FatalConditionHandler::oldSigActions[sizeof(signalDefs)/sizeof(SignalDefs)] = {}; - stack_t FatalConditionHandler::oldSigStack = {}; - char FatalConditionHandler::altStackMem[sigStackSize] = {}; +#if defined(__GNUC__) +# pragma GCC diagnostic pop +#endif -} // namespace Catch + void FatalConditionHandler::disengage_platform() { + restorePreviousSignalHandlers(); + } -#endif // signals/SEH handling +} // end namespace Catch -#if defined(__GNUC__) -# pragma GCC diagnostic pop -#endif +#endif // CATCH_CONFIG_POSIX_SIGNALS diff --git a/src/catch2/internal/catch_fatal_condition_handler.hpp b/src/catch2/internal/catch_fatal_condition_handler.hpp index 1b23c0b88f..4a2818cb56 100644 --- a/src/catch2/internal/catch_fatal_condition_handler.hpp +++ b/src/catch2/internal/catch_fatal_condition_handler.hpp @@ -10,57 +10,60 @@ #include #include -#include - -#if defined( CATCH_CONFIG_WINDOWS_SEH ) +#include namespace Catch { - struct FatalConditionHandler { - - static LONG CALLBACK handleVectoredException(PEXCEPTION_POINTERS ExceptionInfo); + /** + * Wrapper for platform-specific fatal error (signals/SEH) handlers + * + * Tries to be cooperative with other handlers, and not step over + * other handlers. This means that unknown structured exceptions + * are passed on, previous signal handlers are called, and so on. + * + * Can only be instantiated once, and assumes that once a signal + * is caught, the binary will end up terminating. Thus, there + */ + class FatalConditionHandler { + bool m_started = false; + + // Install/disengage implementation for specific platform. + // Should be if-defed to work on current platform, can assume + // engage-disengage 1:1 pairing. + void engage_platform(); + void disengage_platform(); + public: + // Should also have platform-specific implementations as needed FatalConditionHandler(); - static void reset(); - ~FatalConditionHandler() { reset(); } - - private: - static bool isSet; - static ULONG guaranteeSize; - static PVOID exceptionHandlerHandle; + ~FatalConditionHandler(); + + void engage() { + assert(!m_started && "Handler cannot be installed twice."); + m_started = true; + engage_platform(); + } + + void disengage() { + assert(m_started && "Handler cannot be uninstalled without being installed first"); + m_started = false; + disengage_platform(); + } }; -} // namespace Catch - -#elif defined ( CATCH_CONFIG_POSIX_SIGNALS ) - -#include - -namespace Catch { - - struct FatalConditionHandler { - - static bool isSet; - static struct sigaction oldSigActions[]; - static stack_t oldSigStack; - static char altStackMem[]; - - static void handleSignal( int sig ); - - FatalConditionHandler(); - ~FatalConditionHandler() { reset(); } - static void reset(); + //! Simple RAII guard for (dis)engaging the FatalConditionHandler + class FatalConditionHandlerGuard { + FatalConditionHandler* m_handler; + public: + FatalConditionHandlerGuard(FatalConditionHandler* handler): + m_handler(handler) { + m_handler->engage(); + } + ~FatalConditionHandlerGuard() { + m_handler->disengage(); + } }; -} // namespace Catch - - -#else - -namespace Catch { - struct FatalConditionHandler {}; -} - -#endif +} // end namespace Catch #endif // CATCH_FATAL_CONDITION_HANDLER_HPP_INCLUDED diff --git a/src/catch2/internal/catch_run_context.cpp b/src/catch2/internal/catch_run_context.cpp index 8379a7aa02..5df386517c 100644 --- a/src/catch2/internal/catch_run_context.cpp +++ b/src/catch2/internal/catch_run_context.cpp @@ -459,10 +459,10 @@ namespace Catch { } void RunContext::invokeActiveTestCase() { - // We need to register a handler for signals/structured exceptions + // We need to engage a handler for signals/structured exceptions // before running the tests themselves, or the binary can crash // without failed test being reported. - FatalConditionHandler _; + FatalConditionHandlerGuard _(&m_fatalConditionhandler); m_activeTestCase->invoke(); } diff --git a/src/catch2/internal/catch_run_context.hpp b/src/catch2/internal/catch_run_context.hpp index 24e0c99a3e..d4f913da06 100644 --- a/src/catch2/internal/catch_run_context.hpp +++ b/src/catch2/internal/catch_run_context.hpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -139,6 +140,7 @@ namespace Catch { std::vector m_unfinishedSections; std::vector m_activeSections; TrackerContext m_trackerContext; + FatalConditionHandler m_fatalConditionhandler; bool m_lastAssertionPassed = false; bool m_shouldReportUnexpected = true; bool m_includeSuccessfulResults;