Extract SignalHandler from SignalCatcher and also use it for backtrace

.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:

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

src/cpp-utils/assert/backtrace_nonwindows.cpp

@@ -11,6 +11,7 @@
 #include <string>
 #include <dlfcn.h>
 #include "../logging/logging.h"
+#include <cpp-utils/process/SignalHandler.h>
 
 // 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);
 	}
 }
 

src/cpp-utils/process/SignalCatcher.cpp

@@ -1,4 +1,5 @@
 #include "SignalCatcher.h"
+#include "SignalHandler.h"
 
 #include <algorithm>
 #include <stdexcept>
@@ -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<bool>* _signal_occurred_flag;
     SignalCatcherRegisterer _registerer;
-    SignalHandlerRAII _handler;
+    SignalHandlerRAII<&got_signal> _handler;
 
     DISALLOW_COPY_AND_ASSIGN(SignalCatcherImpl);
 };

src/cpp-utils/process/SignalCatcher.h

@@ -37,6 +37,7 @@ private:
     DISALLOW_COPY_AND_ASSIGN(SignalCatcher);
 };
 
+
 }
 
 #endif

src/cpp-utils/process/SignalHandler.cpp

@@ -0,0 +1 @@
+#include "SignalHandler.h"

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 <memory>
+#include <csignal>
+#include <cpp-utils/assert/assert.h>
+
+// 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<SignalHandlerFunction* handler>
+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<SignalHandlerFunction* handler>
+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<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<SignalHandlerFunction* handler>
+void wrap_signal_handler(int signal) {
+    SignalHandlerRunningRAII<handler> disable_signal_processing_while_handler_running_and_reset_handler_afterwards(signal);
+    (*handler)(signal);
+}
+}
+
+template<SignalHandlerFunction* handler>
+class SignalHandlerRAII final {
+public:
+    explicit SignalHandlerRAII(int signal)
+    : _old_handler(nullptr), _signal(signal) {
+        _old_handler = ::signal(_signal, &details::wrap_signal_handler<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<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

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

test/cpp-utils/process/SignalHandlerTest.cpp

@@ -0,0 +1,132 @@
+#include <gtest/gtest.h>
+#include <cpp-utils/process/SignalHandler.h>
+
+using namespace cpputils;
+
+namespace {
+std::atomic<int> 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<int> triggered_count_1;
+std::atomic<int> 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),
+        ""
+    );
+}
+
+}