From a281173099e4c1634ab6ee709e1d838312aee980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Sat, 26 Nov 2016 14:10:11 +0100 Subject: [PATCH 1/5] Fix for sigsegv stack overflow behavior Also stops Catch from assuming its the only signal user in the binary, and makes it restore the signal handlers it has replaced. Same goes for the signal stack. The signal stack itself probably shouldn't be always reallocated for fragmentation reasons, but that can be fixed later on. --- include/internal/catch_fatal_condition.hpp | 29 ++++++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/include/internal/catch_fatal_condition.hpp b/include/internal/catch_fatal_condition.hpp index dd21d590..e57d8810 100644 --- a/include/internal/catch_fatal_condition.hpp +++ b/include/internal/catch_fatal_condition.hpp @@ -60,22 +60,41 @@ namespace Catch { fatal( "", -sig ); } - FatalConditionHandler() : m_isSet( true ) { - for( std::size_t i = 0; i < sizeof(signalDefs)/sizeof(SignalDefs); ++i ) - signal( signalDefs[i].id, handleSignal ); + FatalConditionHandler(): m_isSet(true), m_altStackMem(new char[SIGSTKSZ]) { + stack_t sigStack; + sigStack.ss_sp = m_altStackMem; + sigStack.ss_size = SIGSTKSZ; + sigStack.ss_flags = 0; + sigaltstack(&sigStack, &m_oldSigStack); + struct sigaction sa = { 0 }; + + sa.sa_handler = handleSignal; + sa.sa_flags = SA_ONSTACK; + for (std::size_t i = 0; i < sizeof(signalDefs)/sizeof(SignalDefs); ++i) { + sigaction(signalDefs[i].id, &sa, &m_oldSigActions[i]); + } } ~FatalConditionHandler() { reset(); + delete[] m_altStackMem; } void reset() { if( m_isSet ) { - for( std::size_t i = 0; i < sizeof(signalDefs)/sizeof(SignalDefs); ++i ) - signal( signalDefs[i].id, SIG_DFL ); + // 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, &m_oldSigActions[i], CATCH_NULL); + } + // Return the old stack + sigaltstack(&m_oldSigStack, CATCH_NULL); m_isSet = false; } } bool m_isSet; + // C++03 doesn't allow auto_ptr, so we have manage the memory ourselves + char* m_altStackMem; + struct sigaction m_oldSigActions [sizeof(signalDefs)/sizeof(SignalDefs)]; + stack_t m_oldSigStack; }; } // namespace Catch From 70f43d719bfd13e4c1d385e56801139d1a518273 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Fri, 16 Dec 2016 15:00:19 +0100 Subject: [PATCH 2/5] Added signal handling under Windows. Only some "signals" are handled under Windows, because Windows does not use signals per-se and the mechanics are different. For now, we handle sigsegv, stack overflow, div-by-zero and sigill. We can also meaningfully add various floating point errors, but not sigterm and family, because sigterm is not a structured exception under Windows. There is also no catch-all, because that would also catch various debugger-related exceptions, like EXCEPTION_BREAKPOINT. --- include/internal/catch_fatal_condition.hpp | 58 +++++++++++++++++++++- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/include/internal/catch_fatal_condition.hpp b/include/internal/catch_fatal_condition.hpp index e57d8810..c6c3489f 100644 --- a/include/internal/catch_fatal_condition.hpp +++ b/include/internal/catch_fatal_condition.hpp @@ -26,11 +26,65 @@ namespace Catch { #if defined ( CATCH_PLATFORM_WINDOWS ) ///////////////////////////////////////// +#define NOMINMAX +#define WIN32_LEAN_AND_MEAN +#include +#undef WIN32_LEAN_AND_MEAN +#undef NOMINMAX + + namespace Catch { + struct SignalDefs { DWORD id; const char* name; }; + extern SignalDefs signalDefs[]; + // There is no 1-1 mapping between signals and windows exceptions. + // Windows can easily distinguish between SO and SigSegV, + // but SigInt, SigTerm, etc are handled differently. + SignalDefs signalDefs[] = { + { EXCEPTION_ILLEGAL_INSTRUCTION, "SIGILL - Illegal instruction signal" }, + { EXCEPTION_STACK_OVERFLOW, "SIGSEGV - Stack overflow" }, + { EXCEPTION_ACCESS_VIOLATION, "SIGSEGV - Segmentation violation signal" }, + { EXCEPTION_INT_DIVIDE_BY_ZERO, "Divide by zero error" }, + }; + struct FatalConditionHandler { - void reset() {} - }; + + static LONG CALLBACK handleVectoredException(PEXCEPTION_POINTERS ExceptionInfo) { + for (int i = 0; i < sizeof(signalDefs) / sizeof(SignalDefs); ++i) { + if (ExceptionInfo->ExceptionRecord->ExceptionCode == signalDefs[i].id) { + fatal(signalDefs[i].name, -i); + } + } + // If its not an exception we care about, pass it along. + // This stops us from eating debugger breaks etc. + return EXCEPTION_CONTINUE_SEARCH; + } + + // 32k seems enough for Catch to handle stack overflow, + // but the value was found experimentally, so there is no strong guarantee + FatalConditionHandler():m_isSet(true), m_guaranteeSize(32 * 1024) { + // Register as first handler in current chain + AddVectoredExceptionHandler(1, handleVectoredException); + // Pass in guarantee size to be filled + SetThreadStackGuarantee(&m_guaranteeSize); + } + + void reset() { + if (m_isSet) { + // Unregister handler and restore the old guarantee + RemoveVectoredExceptionHandler(handleVectoredException); + SetThreadStackGuarantee(&m_guaranteeSize); + m_isSet = false; + } + } + + ~FatalConditionHandler() { + reset(); + } + private: + bool m_isSet; + ULONG m_guaranteeSize; + }; } // namespace Catch From 40dbdf6cb2d34d2d056f4292e4b70f0ff6afd562 Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Thu, 12 Jan 2017 08:44:00 +0000 Subject: [PATCH 3/5] Reset signals immediately after use and re-raise orginal signal instead of just exiting --- include/internal/catch_fatal_condition.hpp | 66 ++++++++++++++-------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/include/internal/catch_fatal_condition.hpp b/include/internal/catch_fatal_condition.hpp index c6c3489f..1a130a45 100644 --- a/include/internal/catch_fatal_condition.hpp +++ b/include/internal/catch_fatal_condition.hpp @@ -12,14 +12,11 @@ namespace Catch { - // Report the error condition then exit the process - inline void fatal( std::string const& message, int exitCode ) { + // Report the error condition + inline void reportFatal( std::string const& message, int exitCode ) { IContext& context = Catch::getCurrentContext(); IResultCapture* resultCapture = context.getResultCapture(); resultCapture->handleFatalErrorCondition( message ); - - if( Catch::alwaysTrue() ) // avoids "no return" warnings - exit( exitCode ); } } // namespace Catch @@ -52,7 +49,7 @@ namespace Catch { static LONG CALLBACK handleVectoredException(PEXCEPTION_POINTERS ExceptionInfo) { for (int i = 0; i < sizeof(signalDefs) / sizeof(SignalDefs); ++i) { if (ExceptionInfo->ExceptionRecord->ExceptionCode == signalDefs[i].id) { - fatal(signalDefs[i].name, -i); + reportFatal(signalDefs[i].name, -i); } } // If its not an exception we care about, pass it along. @@ -94,7 +91,10 @@ namespace Catch { namespace Catch { - struct SignalDefs { int id; const char* name; }; + struct SignalDefs { + int id; + const char* name; + }; extern SignalDefs signalDefs[]; SignalDefs signalDefs[] = { { SIGINT, "SIGINT - Terminal interrupt signal" }, @@ -103,53 +103,69 @@ namespace Catch { { SIGSEGV, "SIGSEGV - Segmentation violation signal" }, { SIGTERM, "SIGTERM - Termination request signal" }, { SIGABRT, "SIGABRT - Abort (abnormal termination) signal" } - }; + }; struct FatalConditionHandler { + static bool isSet; + static struct sigaction oldSigActions [sizeof(signalDefs)/sizeof(SignalDefs)]; + static stack_t oldSigStack; + static char altStackMem[SIGSTKSZ]; + static void handleSignal( int sig ) { - for( std::size_t i = 0; i < sizeof(signalDefs)/sizeof(SignalDefs); ++i ) - if( sig == signalDefs[i].id ) - fatal( signalDefs[i].name, -sig ); - fatal( "", -sig ); + std::string name = ""; + for (std::size_t i = 0; i < sizeof(signalDefs) / sizeof(SignalDefs); ++i) { + SignalDefs &def = signalDefs[i]; + if (sig == def.id) { + name = def.name; + sigaction(def.id, &oldSigActions[i], CATCH_NULL); + break; + } + } + reportFatal(name, -sig); + raise( sig ); } - FatalConditionHandler(): m_isSet(true), m_altStackMem(new char[SIGSTKSZ]) { + FatalConditionHandler() { + isSet = true; stack_t sigStack; - sigStack.ss_sp = m_altStackMem; + sigStack.ss_sp = altStackMem; sigStack.ss_size = SIGSTKSZ; sigStack.ss_flags = 0; - sigaltstack(&sigStack, &m_oldSigStack); + sigaltstack(&sigStack, &oldSigStack); struct sigaction sa = { 0 }; sa.sa_handler = handleSignal; sa.sa_flags = SA_ONSTACK; for (std::size_t i = 0; i < sizeof(signalDefs)/sizeof(SignalDefs); ++i) { - sigaction(signalDefs[i].id, &sa, &m_oldSigActions[i]); + sigaction(signalDefs[i].id, &sa, &oldSigActions[i]); } } + + ~FatalConditionHandler() { reset(); - delete[] m_altStackMem; } void reset() { - if( m_isSet ) { + 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, &m_oldSigActions[i], CATCH_NULL); + sigaction(signalDefs[i].id, &oldSigActions[i], CATCH_NULL); } // Return the old stack - sigaltstack(&m_oldSigStack, CATCH_NULL); - m_isSet = false; + sigaltstack(&oldSigStack, CATCH_NULL); + isSet = false; } } - bool m_isSet; // C++03 doesn't allow auto_ptr, so we have manage the memory ourselves - char* m_altStackMem; - struct sigaction m_oldSigActions [sizeof(signalDefs)/sizeof(SignalDefs)]; - stack_t m_oldSigStack; }; + + bool FatalConditionHandler::isSet = false; + struct sigaction FatalConditionHandler::oldSigActions[sizeof(signalDefs)/sizeof(SignalDefs)] = {} ;//[sizeof(signalDefs)/sizeof(SignalDefs)]; + stack_t FatalConditionHandler::oldSigStack = {}; + char FatalConditionHandler::altStackMem[SIGSTKSZ] = {}; + } // namespace Catch From 7c8b93eac3e63397d334b91755321bfb2982e1d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Sat, 14 Jan 2017 15:08:00 +0100 Subject: [PATCH 4/5] Removed superfluous comments (bad merge after cherry pick). --- include/internal/catch_fatal_condition.hpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/include/internal/catch_fatal_condition.hpp b/include/internal/catch_fatal_condition.hpp index 1a130a45..895b662d 100644 --- a/include/internal/catch_fatal_condition.hpp +++ b/include/internal/catch_fatal_condition.hpp @@ -157,12 +157,10 @@ namespace Catch { isSet = false; } } - - // C++03 doesn't allow auto_ptr, so we have manage the memory ourselves }; bool FatalConditionHandler::isSet = false; - struct sigaction FatalConditionHandler::oldSigActions[sizeof(signalDefs)/sizeof(SignalDefs)] = {} ;//[sizeof(signalDefs)/sizeof(SignalDefs)]; + struct sigaction FatalConditionHandler::oldSigActions[sizeof(signalDefs)/sizeof(SignalDefs)] = {}; stack_t FatalConditionHandler::oldSigStack = {}; char FatalConditionHandler::altStackMem[SIGSTKSZ] = {}; From ffc4a9dc143bc66d2e1fd883e1395a3397a68e9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Sat, 14 Jan 2017 15:21:44 +0100 Subject: [PATCH 5/5] If we receive a signal, we re-register ALL previous signal handlers. This fixes the case when we pass signal to previously registered handler, and it needs to transform the signal into different one. Still problematic: What if the signal handler we replaced does not terminate the application? We can end up in a weird state and loop forever. Possible solution: Deregister our signal handlers, CALL the previous signal handler explicitly and if control returns, abort. This would however complicate our code quite a bit, as we would have to parse the sigaction we delegate to, decide whether to use signal handler or signal action, etc... --- include/internal/catch_fatal_condition.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/internal/catch_fatal_condition.hpp b/include/internal/catch_fatal_condition.hpp index 895b662d..f99c8f26 100644 --- a/include/internal/catch_fatal_condition.hpp +++ b/include/internal/catch_fatal_condition.hpp @@ -118,10 +118,10 @@ namespace Catch { SignalDefs &def = signalDefs[i]; if (sig == def.id) { name = def.name; - sigaction(def.id, &oldSigActions[i], CATCH_NULL); break; } } + reset(); reportFatal(name, -sig); raise( sig ); } @@ -146,7 +146,7 @@ namespace Catch { ~FatalConditionHandler() { reset(); } - void reset() { + static void 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 ) {