From 8eefb0157546dd14e71d90faee8a86df30c8e2d3 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Sun, 17 Mar 2019 01:08:57 -0700 Subject: [PATCH] Extract SignalHandler from SignalCatcher and also use it for backtrace --- .circleci/config.yml | 2 +- src/cpp-utils/CMakeLists.txt | 1 + src/cpp-utils/assert/backtrace_nonwindows.cpp | 19 +-- src/cpp-utils/process/SignalCatcher.cpp | 114 +------------- src/cpp-utils/process/SignalCatcher.h | 1 + src/cpp-utils/process/SignalHandler.cpp | 1 + src/cpp-utils/process/SignalHandler.h | 140 ++++++++++++++++++ test/cpp-utils/CMakeLists.txt | 1 + test/cpp-utils/process/SignalHandlerTest.cpp | 132 +++++++++++++++++ 9 files changed, 286 insertions(+), 125 deletions(-) create mode 100644 src/cpp-utils/process/SignalHandler.cpp create mode 100644 src/cpp-utils/process/SignalHandler.h create mode 100644 test/cpp-utils/process/SignalHandlerTest.cpp diff --git a/.circleci/config.yml b/.circleci/config.yml index 3b6127d6..df33cec3 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -479,7 +479,7 @@ jobs: OMP_NUM_THREADS: "1" CXXFLAGS: "-O2 -fsanitize=thread -fno-omit-frame-pointer" BUILD_TYPE: "Debug" - GTEST_ARGS: "--gtest_filter=-LoggingTest.LoggingAlsoWorksAfterFork:AssertTest_DebugBuild.*:SignalCatcherTest.*_thenDies:CliTest_Setup.*:CliTest_IntegrityCheck.*:*/CliTest_WrongEnvironment.*:CliTest_Unmount.*" + GTEST_ARGS: "--gtest_filter=-LoggingTest.LoggingAlsoWorksAfterFork:AssertTest_DebugBuild.*:SignalCatcherTest.*_thenDies:SignalHandlerTest.*_thenDies:SignalHandlerTest.givenMultipleSigIntHandlers_whenRaising_thenCatchesCorrectSignal:CliTest_Setup.*:CliTest_IntegrityCheck.*:*/CliTest_WrongEnvironment.*:CliTest_Unmount.*" CMAKE_FLAGS: "" RUN_TESTS: true clang_tidy: diff --git a/src/cpp-utils/CMakeLists.txt b/src/cpp-utils/CMakeLists.txt index 500e6ec1..2ecd690c 100644 --- a/src/cpp-utils/CMakeLists.txt +++ b/src/cpp-utils/CMakeLists.txt @@ -12,6 +12,7 @@ set(SOURCES process/daemonize.cpp process/subprocess.cpp process/SignalCatcher.cpp + process/SignalHandler.cpp tempfile/TempFile.cpp tempfile/TempDir.cpp network/HttpClient.cpp diff --git a/src/cpp-utils/assert/backtrace_nonwindows.cpp b/src/cpp-utils/assert/backtrace_nonwindows.cpp index 8e26df5a..c495d0b6 100644 --- a/src/cpp-utils/assert/backtrace_nonwindows.cpp +++ b/src/cpp-utils/assert/backtrace_nonwindows.cpp @@ -11,6 +11,7 @@ #include #include #include "../logging/logging.h" +#include // TODO Add file and line number on non-windows @@ -95,21 +96,15 @@ namespace { LOG(ERR, "SIGABRT\n{}", backtrace()); exit(1); } - void set_handler(int signum, void(*handler)(int)) { - auto result = signal(signum, handler); -#pragma GCC diagnostic push // SIG_ERR uses old style casts -#pragma GCC diagnostic ignored "-Wold-style-cast" - if (SIG_ERR == result) { - LOG(ERR, "Failed to set signal {} handler. Errno: {}", signum, errno); - } -#pragma GCC diagnostic pop - } } void showBacktraceOnCrash() { - set_handler(SIGSEGV, &sigsegv_handler); - set_handler(SIGABRT, &sigabrt_handler); - set_handler(SIGILL, &sigill_handler); + // the signal handler RAII objects will be initialized on first call (which will register the signal handler) + // and destroyed on program exit (which will unregister the signal handler) + + static SignalHandlerRAII<&sigsegv_handler> segv(SIGSEGV); + static SignalHandlerRAII<&sigabrt_handler> abrt(SIGABRT); + static SignalHandlerRAII<&sigill_handler> ill(SIGILL); } } diff --git a/src/cpp-utils/process/SignalCatcher.cpp b/src/cpp-utils/process/SignalCatcher.cpp index a89345e3..ae37d2d9 100644 --- a/src/cpp-utils/process/SignalCatcher.cpp +++ b/src/cpp-utils/process/SignalCatcher.cpp @@ -1,4 +1,5 @@ #include "SignalCatcher.h" +#include "SignalHandler.h" #include #include @@ -16,117 +17,6 @@ namespace { void got_signal(int signal); -using SignalHandlerFunction = void(int); - -constexpr SignalHandlerFunction* signal_catcher_function = &got_signal; - -#if !defined(_MSC_VER) - -class SignalHandlerRAII final { -public: - SignalHandlerRAII(int signal) - : _old_handler(), _signal(signal) { - struct sigaction new_signal_handler{}; - std::memset(&new_signal_handler, 0, sizeof(new_signal_handler)); - new_signal_handler.sa_handler = signal_catcher_function; // NOLINT(cppcoreguidelines-pro-type-union-access) - new_signal_handler.sa_flags = SA_RESTART; - int error = sigfillset(&new_signal_handler.sa_mask); // block all signals while signal handler is running - if (0 != error) { - throw std::runtime_error("Error calling sigfillset. Errno: " + std::to_string(errno)); - } - _sigaction(_signal, &new_signal_handler, &_old_handler); - } - - ~SignalHandlerRAII() { - // reset to old signal handler - struct sigaction removed_handler{}; - _sigaction(_signal, &_old_handler, &removed_handler); - if (signal_catcher_function != removed_handler.sa_handler) { // NOLINT(cppcoreguidelines-pro-type-union-access) - ASSERT(false, "Signal handler screwup. We just replaced a signal handler that wasn't our own."); - } - } - -private: - static void _sigaction(int signal, struct sigaction *new_handler, struct sigaction *old_handler) { - int error = sigaction(signal, new_handler, old_handler); - if (0 != error) { - throw std::runtime_error("Error calling sigaction. Errno: " + std::to_string(errno)); - } - } - - struct sigaction _old_handler; - int _signal; - - DISALLOW_COPY_AND_ASSIGN(SignalHandlerRAII); -}; - -#else - -class SignalHandlerRAII final { -public: - SignalHandlerRAII(int signal) - : _old_handler(nullptr), _signal(signal) { - _old_handler = ::signal(_signal, signal_catcher_function); - if (_old_handler == SIG_ERR) { - throw std::logic_error("Error calling signal(). Errno: " + std::to_string(errno)); - } - } - - ~SignalHandlerRAII() { - // reset to old signal handler - SignalHandlerFunction* error = ::signal(_signal, _old_handler); - if (error == SIG_ERR) { - throw std::logic_error("Error resetting signal(). Errno: " + std::to_string(errno)); - } - if (error != signal_catcher_function) { - throw std::logic_error("Signal handler screwup. We just replaced a signal handler that wasn't our own."); - } - } - -private: - - SignalHandlerFunction* _old_handler; - int _signal; - - DISALLOW_COPY_AND_ASSIGN(SignalHandlerRAII); -}; - -// The Linux default behavior (i.e. the way we set up sigaction above) is to disable signal processing while the signal -// handler is running and to re-enable the custom handler once processing is finished. The Windows default behavior -// is to reset the handler to the default handler directly before executing the handler, i.e. the handler will only -// be called once. To fix this, we use this RAII class on Windows, of which an instance will live in the signal handler. -// In its constructor, it disables signal handling, and in its destructor it re-sets the custom handler. -// This is not perfect since there is a small time window between calling the signal handler and calling the constructor -// of this class, but it's the best we can do. -class SignalHandlerRunningRAII final { -public: - SignalHandlerRunningRAII(int signal) : _signal(signal) { - SignalHandlerFunction* old_handler = ::signal(_signal, SIG_IGN); - if (old_handler == SIG_ERR) { - throw std::logic_error("Error disabling signal(). Errno: " + std::to_string(errno)); - } - if (old_handler != SIG_DFL) { - // see description above, we expected the signal handler to be reset. - throw std::logic_error("We expected windows to reset the signal handler but it didn't. Did the Windows API change?"); - } - } - - ~SignalHandlerRunningRAII() { - SignalHandlerFunction* old_handler = ::signal(_signal, signal_catcher_function); - if (old_handler == SIG_ERR) { - throw std::logic_error("Error resetting signal() after calling handler. Errno: " + std::to_string(errno)); - } - if (old_handler != SIG_IGN) { - throw std::logic_error("Weird, we just did set the signal handler to ignore. Why isn't it still ignore?"); - } - } - -private: - int _signal; -}; - -#endif - class SignalCatcherRegistry final { public: void add(int signal, details::SignalCatcherImpl* signal_occurred_flag) { @@ -211,7 +101,7 @@ public: private: std::atomic* _signal_occurred_flag; SignalCatcherRegisterer _registerer; - SignalHandlerRAII _handler; + SignalHandlerRAII<&got_signal> _handler; DISALLOW_COPY_AND_ASSIGN(SignalCatcherImpl); }; diff --git a/src/cpp-utils/process/SignalCatcher.h b/src/cpp-utils/process/SignalCatcher.h index 87d3c948..e8ae4614 100644 --- a/src/cpp-utils/process/SignalCatcher.h +++ b/src/cpp-utils/process/SignalCatcher.h @@ -37,6 +37,7 @@ private: DISALLOW_COPY_AND_ASSIGN(SignalCatcher); }; + } #endif diff --git a/src/cpp-utils/process/SignalHandler.cpp b/src/cpp-utils/process/SignalHandler.cpp new file mode 100644 index 00000000..26eafe4d --- /dev/null +++ b/src/cpp-utils/process/SignalHandler.cpp @@ -0,0 +1 @@ +#include "SignalHandler.h" diff --git a/src/cpp-utils/process/SignalHandler.h b/src/cpp-utils/process/SignalHandler.h new file mode 100644 index 00000000..ed8752d9 --- /dev/null +++ b/src/cpp-utils/process/SignalHandler.h @@ -0,0 +1,140 @@ +#pragma once +#ifndef MESSMER_CPPUTILS_PROCESS_SIGNALHANDLER_H_ +#define MESSMER_CPPUTILS_PROCESS_SIGNALHANDLER_H_ + +#include +#include +#include + +// TODO Test SignalHandler + +/** + * A SignalHandlerRAII instance replaces the signal handler for the given signal with the given handler + * as long as it is alive and sets it to the previous handler once it dies. + * This way, it can be used for stacking different signal handlers on top of each other. + */ + +namespace cpputils { + +using SignalHandlerFunction = void(int); + +#if !defined(_MSC_VER) + +template +class SignalHandlerRAII final { +public: + explicit SignalHandlerRAII(int signal) + : _old_handler(), _signal(signal) { + struct sigaction new_signal_handler{}; + std::memset(&new_signal_handler, 0, sizeof(new_signal_handler)); + new_signal_handler.sa_handler = handler; // NOLINT(cppcoreguidelines-pro-type-union-access) + new_signal_handler.sa_flags = SA_RESTART; + int error = sigfillset(&new_signal_handler.sa_mask); // block all signals while signal handler is running + if (0 != error) { + throw std::runtime_error("Error calling sigfillset. Errno: " + std::to_string(errno)); + } + _sigaction(_signal, &new_signal_handler, &_old_handler); + } + + ~SignalHandlerRAII() { + // reset to old signal handler + struct sigaction removed_handler{}; + _sigaction(_signal, &_old_handler, &removed_handler); + if (handler != removed_handler.sa_handler) { // NOLINT(cppcoreguidelines-pro-type-union-access) + ASSERT(false, "Signal handler screwup. We just replaced a signal handler that wasn't our own."); + } + } + +private: + static void _sigaction(int signal, struct sigaction *new_handler, struct sigaction *old_handler) { + int error = sigaction(signal, new_handler, old_handler); + if (0 != error) { + throw std::runtime_error("Error calling sigaction. Errno: " + std::to_string(errno)); + } + } + + struct sigaction _old_handler; + int _signal; + + DISALLOW_COPY_AND_ASSIGN(SignalHandlerRAII); +}; + +#else +namespace details { +// The Linux default behavior (i.e. the way we set up sigaction above) is to disable signal processing while the signal +// handler is running and to re-enable the custom handler once processing is finished. The Windows default behavior +// is to reset the handler to the default handler directly before executing the handler, i.e. the handler will only +// be called once. To fix this, we use this RAII class on Windows, of which an instance will live in the signal handler. +// In its constructor, it disables signal handling, and in its destructor it re-sets the custom handler. +// This is not perfect since there is a small time window between calling the signal handler and calling the constructor +// of this class, but it's the best we can do. +template +class SignalHandlerRunningRAII final { +public: + explicit SignalHandlerRunningRAII(int signal) : _signal(signal) { + SignalHandlerFunction* old_handler = ::signal(_signal, SIG_IGN); + if (old_handler == SIG_ERR) { + throw std::logic_error("Error disabling signal(). Errno: " + std::to_string(errno)); + } + if (old_handler != SIG_DFL) { + // see description above, we expected the signal handler to be reset. + throw std::logic_error("We expected windows to reset the signal handler but it didn't. Did the Windows API change?"); + } + } + + ~SignalHandlerRunningRAII() { + SignalHandlerFunction* old_handler = ::signal(_signal, &details::wrap_signal_handler); + if (old_handler == SIG_ERR) { + throw std::logic_error("Error resetting signal() after calling handler. Errno: " + std::to_string(errno)); + } + if (old_handler != SIG_IGN) { + throw std::logic_error("Weird, we just did set the signal handler to ignore. Why isn't it still ignore?"); + } + } + +private: + int _signal; +}; + +template +void wrap_signal_handler(int signal) { + SignalHandlerRunningRAII disable_signal_processing_while_handler_running_and_reset_handler_afterwards(signal); + (*handler)(signal); +} +} + +template +class SignalHandlerRAII final { +public: + explicit SignalHandlerRAII(int signal) + : _old_handler(nullptr), _signal(signal) { + _old_handler = ::signal(_signal, &details::wrap_signal_handler); + if (_old_handler == SIG_ERR) { + throw std::logic_error("Error calling signal(). Errno: " + std::to_string(errno)); + } + } + + ~SignalHandlerRAII() { + // reset to old signal handler + SignalHandlerFunction* error = ::signal(_signal, _old_handler); + if (error == SIG_ERR) { + throw std::logic_error("Error resetting signal(). Errno: " + std::to_string(errno)); + } + if (error != &details::wrap_signal_handler) { + throw std::logic_error("Signal handler screwup. We just replaced a signal handler that wasn't our own."); + } + } + +private: + + SignalHandlerFunction* _old_handler; + int _signal; + + DISALLOW_COPY_AND_ASSIGN(SignalHandlerRAII); +}; + +#endif + +} + +#endif diff --git a/test/cpp-utils/CMakeLists.txt b/test/cpp-utils/CMakeLists.txt index 8c28de45..eab350f3 100644 --- a/test/cpp-utils/CMakeLists.txt +++ b/test/cpp-utils/CMakeLists.txt @@ -17,6 +17,7 @@ set(SOURCES process/subprocess_include_test.cpp process/SubprocessTest.cpp process/SignalCatcherTest.cpp + process/SignalHandlerTest.cpp tempfile/TempFileTest.cpp tempfile/TempFileIncludeTest.cpp tempfile/TempDirIncludeTest.cpp diff --git a/test/cpp-utils/process/SignalHandlerTest.cpp b/test/cpp-utils/process/SignalHandlerTest.cpp new file mode 100644 index 00000000..194b2550 --- /dev/null +++ b/test/cpp-utils/process/SignalHandlerTest.cpp @@ -0,0 +1,132 @@ +#include +#include + +using namespace cpputils; + +namespace { +std::atomic triggered; + +void trigger(int signal) { + triggered = signal; +} + +void raise_signal(int signal) { + int error = ::raise(signal); + if (error != 0) { + throw std::runtime_error("Error raising signal"); + } +} + +TEST(SignalHandlerTest, givenNoSignalHandler_whenRaisingSigint_thenDies) { + EXPECT_DEATH( + raise_signal(SIGINT), + "" + ); +} + +TEST(SignalHandlerTest, givenNoSignalHandler_whenRaisingSigterm_thenDies) { + EXPECT_DEATH( + raise_signal(SIGTERM), + "" + ); +} + +TEST(SignalHandlerTest, givenSigIntHandler_whenRaisingSigInt_thenCatches) { + triggered = 0; + + SignalHandlerRAII<&trigger> handler(SIGINT); + + raise_signal(SIGINT); + EXPECT_EQ(SIGINT, triggered); +} + +TEST(SignalHandlerTest, givenSigIntHandler_whenRaisingSigTerm_thenDies) { + SignalHandlerRAII<&trigger> handler(SIGINT); + + EXPECT_DEATH( + raise_signal(SIGTERM), + "" + ); +} + +TEST(SignalHandlerTest, givenSigTermHandler_whenRaisingSigTerm_thenCatches) { + triggered = 0; + + SignalHandlerRAII<&trigger> handler(SIGTERM); + + raise_signal(SIGTERM); + EXPECT_EQ(SIGTERM, triggered); +} + +TEST(SignalHandlerTest, givenSigTermHandler_whenRaisingSigInt_thenDies) { + SignalHandlerRAII<&trigger> handler(SIGTERM); + + EXPECT_DEATH( + raise_signal(SIGINT), + "" + ); +} + +TEST(SignalHandlerTest, givenSigIntAndSigTermHandlers_whenRaising_thenCatchesCorrectSignal) { + triggered = 0; + + SignalHandlerRAII<&trigger> handler1(SIGINT); + SignalHandlerRAII<&trigger> handler2(SIGTERM); + + raise_signal(SIGINT); + EXPECT_EQ(SIGINT, triggered); + + raise_signal(SIGTERM); + EXPECT_EQ(SIGTERM, triggered); + + raise_signal(SIGINT); + EXPECT_EQ(SIGINT, triggered); +} + +std::atomic triggered_count_1; +std::atomic triggered_count_2; + +void trigger1(int) { + ++triggered_count_1; +} + +void trigger2(int) { + ++triggered_count_2; +} + +TEST(SignalHandlerTest, givenMultipleSigIntHandlers_whenRaising_thenCatchesCorrectSignal) { + triggered_count_1 = 0; + triggered_count_2 = 0; + + { + SignalHandlerRAII<&trigger1> handler1(SIGINT); + + { + SignalHandlerRAII<&trigger2> handler2(SIGINT); + + raise_signal(SIGINT); + EXPECT_EQ(0, triggered_count_1); + EXPECT_EQ(1, triggered_count_2); + + raise_signal(SIGINT); + EXPECT_EQ(0, triggered_count_1); + EXPECT_EQ(2, triggered_count_2); + } + + raise_signal(SIGINT); + EXPECT_EQ(1, triggered_count_1); + EXPECT_EQ(2, triggered_count_2); + + raise_signal(SIGINT); + EXPECT_EQ(2, triggered_count_1); + EXPECT_EQ(2, triggered_count_2); + + } + + EXPECT_DEATH( + raise_signal(SIGINT), + "" + ); +} + +}