From b36386cfe9ecea3af01a55c6be53d9438cf3259e Mon Sep 17 00:00:00 2001 From: Teon Banek Date: Tue, 31 Oct 2017 09:49:33 +0100 Subject: [PATCH] Prevent double termination signals causing crashes Summary: Use sigaction to register signal handlers. This is preferred over `signal` function, according to `man 3p signal`. Add global sig_atomic_t flag when shutting down. Block other signal handlers when shutting down. Reviewers: mislav.bradac, mferencevic Reviewed By: mferencevic Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D943 --- src/memgraph_bolt.cpp | 43 ++++++++++++++++++++-------------- src/utils/signals/handler.hpp | 32 +++++++++++++++---------- tests/manual/antlr_sigsegv.cpp | 4 ++-- tests/unit/signal_handler.cpp | 2 +- 4 files changed, 48 insertions(+), 33 deletions(-) diff --git a/src/memgraph_bolt.cpp b/src/memgraph_bolt.cpp index 65a64e5be..1b7263021 100644 --- a/src/memgraph_bolt.cpp +++ b/src/memgraph_bolt.cpp @@ -1,3 +1,4 @@ +#include #include #include @@ -40,6 +41,13 @@ DEFINE_uint64(memory_warning_threshold, 1024, "less available RAM available it will log a warning. Set to 0 to " "disable."); +// Needed to correctly handle memgraph destruction from a signal handler. +// Without having some sort of a flag, it is possible that a signal is handled +// when we are exiting main, inside destructors of GraphDb and similar. The +// signal handler may then initiate another shutdown on memgraph which is in +// half destructed state, causing invalid memory access and crash. +volatile sig_atomic_t is_shutting_down = 0; + int main(int argc, char **argv) { google::SetUsageMessage("Memgraph database server"); gflags::SetVersionString(version_string); @@ -56,20 +64,6 @@ int main(int argc, char **argv) { // Unhandled exception handler init. std::set_terminate(&terminate_handler); - // Signal handling init. - SignalHandler::register_handler(Signal::SegmentationFault, []() { - // Log that we got SIGSEGV and abort the program, because returning from - // SIGSEGV handler is undefined behaviour. - std::cerr << "SegmentationFault signal raised" << std::endl; - std::abort(); // This will continue into our SIGABRT handler. - }); - SignalHandler::register_handler(Signal::Abort, []() { - // Log the stacktrace and let the abort continue. - Stacktrace stacktrace; - std::cerr << "Abort signal raised" << std::endl - << stacktrace.dump() << std::endl; - }); - // Initialize bolt session data (GraphDb and Interpreter). SessionData session_data; @@ -85,17 +79,30 @@ int main(int argc, char **argv) { // Initialize server. ServerT server(endpoint, session_data); + // Handler for regular termination signals auto shutdown = [&server, &session_data]() { + if (is_shutting_down) return; + is_shutting_down = 1; // Server needs to be shutdown first and then the database. This prevents a // race condition when a transaction is accepted during server shutdown. server.Shutdown(); session_data.db.Shutdown(); }; - // register SIGTERM handler - SignalHandler::register_handler(Signal::Terminate, shutdown); - // register SIGINT handler - SignalHandler::register_handler(Signal::Interupt, shutdown); + // Prevent handling shutdown inside a shutdown. For example, SIGINT handler + // being interrupted by SIGTERM before is_shutting_down is set, thus causing + // double shutdown. + sigset_t block_shutdown_signals; + sigemptyset(&block_shutdown_signals); + sigaddset(&block_shutdown_signals, SIGTERM); + sigaddset(&block_shutdown_signals, SIGINT); + + CHECK(SignalHandler::RegisterHandler(Signal::Terminate, shutdown, + block_shutdown_signals)) + << "Unable to register SIGTERM handler!"; + CHECK(SignalHandler::RegisterHandler(Signal::Interupt, shutdown, + block_shutdown_signals)) + << "Unable to register SIGINT handler!"; // Start memory warning logger. Scheduler mem_log_scheduler; diff --git a/src/utils/signals/handler.hpp b/src/utils/signals/handler.hpp index 2fef13266..40dccc4ef 100644 --- a/src/utils/signals/handler.hpp +++ b/src/utils/signals/handler.hpp @@ -23,22 +23,30 @@ class SignalHandler { private: static std::map> handlers_; - static void handle(int signal) { handlers_[signal](); } + static void Handle(int signal) { handlers_[signal](); } public: - static void register_handler(Signal signal, Function func) { - int signal_number = static_cast(signal); - handlers_[signal_number] = func; - std::signal(signal_number, SignalHandler::handle); + /// Install a signal handler. + static bool RegisterHandler(Signal signal, Function func) { + sigset_t signal_mask; + sigemptyset(&signal_mask); + return RegisterHandler(signal, func, signal_mask); } - // TODO possible changes if signelton needed later - /* - static SignalHandler& instance() { - static SignalHandler instance; - return instance; - } - */ + /// Like RegisterHandler, but takes a `signal_mask` argument for blocking + /// signals during execution of the handler. `signal_mask` should be created + /// using `sigemptyset` and `sigaddset` functions from ``. + static bool RegisterHandler(Signal signal, Function func, + sigset_t signal_mask) { + int signal_number = static_cast(signal); + handlers_[signal_number] = func; + struct sigaction action; + action.sa_handler = SignalHandler::Handle; + action.sa_mask = signal_mask; + action.sa_flags = SA_RESTART; + if (sigaction(signal_number, &action, NULL) == -1) return false; + return true; + } }; std::map> SignalHandler::handlers_ = {}; diff --git a/tests/manual/antlr_sigsegv.cpp b/tests/manual/antlr_sigsegv.cpp index 615b91053..19e98d7c9 100644 --- a/tests/manual/antlr_sigsegv.cpp +++ b/tests/manual/antlr_sigsegv.cpp @@ -89,13 +89,13 @@ int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv); // Signal handling init. - SignalHandler::register_handler(Signal::SegmentationFault, []() { + SignalHandler::RegisterHandler(Signal::SegmentationFault, []() { // Log that we got SIGSEGV and abort the program, because returning from // SIGSEGV handler is undefined behaviour. std::cerr << "SegmentationFault signal raised" << std::endl; std::abort(); // This will continue into our SIGABRT handler. }); - SignalHandler::register_handler(Signal::Abort, []() { + SignalHandler::RegisterHandler(Signal::Abort, []() { // Log the stacktrace and let the abort continue. Stacktrace stacktrace; std::cerr << "Abort signal raised" << std::endl diff --git a/tests/unit/signal_handler.cpp b/tests/unit/signal_handler.cpp index 82f2eb690..96ab3b231 100644 --- a/tests/unit/signal_handler.cpp +++ b/tests/unit/signal_handler.cpp @@ -8,7 +8,7 @@ #include "utils/stacktrace.hpp" TEST(SignalHandler, SegmentationFaultTest) { - SignalHandler::register_handler(Signal::SegmentationFault, []() { + SignalHandler::RegisterHandler(Signal::SegmentationFault, []() { std::cout << "Segmentation Fault" << std::endl; Stacktrace stacktrace; std::cout << stacktrace.dump() << std::endl;