From 527d69d382be6cb1ed122945109be68f7c5cb7d0 Mon Sep 17 00:00:00 2001
From: Teon Banek <teon.banek@memgraph.io>
Date: Wed, 14 Jun 2017 16:37:23 +0200
Subject: [PATCH] Fix errors with handling SIGSEGV and SIGABRT

Summary:
There were a couple of issues with handling the above 2 signals.

1) Calling `std::exit` from a signal handler is undefined behaviour.

The only defined way for a signal handler to stop the program is calling
one of: `std::_Exit`, `std::abort` or `std::quick_exit`. Neither of them
will completely clean the resources, so a clean exit is not possible.
Since SIGSEGV and SIGABRT happen in extraordinary circumstances that we
wish to debug 99% of the time, it makes sense to generate a core dump
which can be inspected by a debugger. Of the 3 termination functions,
only `std::abort` will generate a core dump, so it makes sense to use
that to stop the program.

Also, since we are now aborting as is the default behaviour on SIGSEGV
and SIGABRT, it becomes questionable why have a custom handler at all.

2) Raising an exception inside a signal handler is undefined behaviour

Although the handler by itself does not raise an exception, it is
possible for the logging facility to raise one. This is a real case when
logging a stack trace in particular. Stack trace is generated by
creating a string "<function name> <line location>". It is possible that
a function name will contain '{}' somewhere inside. This is usually the
case with anonymous functions. The generated string is then passed to
logging, which uses the `fmt` library to fill '{}' with remaining
arguments. Since only a single argument (the stack trace string) is
passed for formatting, naturally the `fmt::format` throws an exception,
that it is missing a format argument.

We could provide an overload which takes a single string, but that
defeats the purpose of `fmt::format` raising an exception in regular
code if we forget to pass an argument. Another solution is to escape the
whole stack trace string, so it is valid for formatting, but this only
complicates the handler even further. The simplest solution is to send
the stack trace to `stderr` and avoid logging altogether.

Simplify Shutdown, so it can be used in a signal handler

Reviewers: florijan, mferencevic, buda, mislav.bradac

Reviewed By: mferencevic, buda

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D474
---
 src/communication/server.hpp |  8 +++++---
 src/memgraph_bolt.cpp        | 20 ++++++++------------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/src/communication/server.hpp b/src/communication/server.hpp
index f70a086ed..3c1f9a0b4 100644
--- a/src/communication/server.hpp
+++ b/src/communication/server.hpp
@@ -69,13 +69,15 @@ class Server
     while (alive_) {
       this->WaitAndProcessEvents();
     }
+
+    logger_.info("Shutting down...");
+    for (auto &worker : workers_) worker->thread_.join();
   }
 
   void Shutdown() {
-    logger_.info("Shutting down...");
+    // This should be as simple as possible, so that it can be called inside a
+    // signal handler.
     alive_.store(false);
-
-    for (auto &worker : workers_) worker->thread_.join();
   }
 
   void OnConnect() {
diff --git a/src/memgraph_bolt.cpp b/src/memgraph_bolt.cpp
index 3fcf8a653..3e15dcc61 100644
--- a/src/memgraph_bolt.cpp
+++ b/src/memgraph_bolt.cpp
@@ -80,14 +80,6 @@ void load_config(int &argc, char **&argv) {
   gflags::ParseCommandLineFlags(&argc, &argv, true);
 }
 
-void log_stacktrace(const std::string &title) {
-  Stacktrace stacktrace;
-  // TODO: Change this from 'info' to 'error', when default error logging is
-  // added.
-  logging::info(title);
-  logging::info(stacktrace.dump());
-}
-
 int main(int argc, char **argv) {
   fs::current_path(fs::path(argv[0]).parent_path());
   load_config(argc, argv);
@@ -109,12 +101,16 @@ int main(int argc, char **argv) {
 
   // Signal handling init.
   SignalHandler::register_handler(Signal::SegmentationFault, []() {
-    log_stacktrace("SegmentationFault signal raised");
-    std::exit(EXIT_FAILURE);
+    // 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_stacktrace("Abort signal raised");
-    std::exit(EXIT_FAILURE);
+    // Log the stacktrace and let the abort continue.
+    Stacktrace stacktrace;
+    std::cerr << "Abort signal raised" << std::endl
+              << stacktrace.dump() << std::endl;
   });
 
   // Initialize endpoint.