signal: Add hooks for calling fatal error handlers from non-envoy signal handlers (envoyproxy#12062)

Add hooks for calling fatal error handlers from non-Envoy signal handlers. Move register/removeFatalErrorHandler from SignalAction into a new FatalErrorHandler namespace, and add a new function, callFatalErrorHandlers, which runs the registered error handlers. This makes the crash logging from issue envoyproxy#7300 available for builds that don't use ENVOY_HANDLE_SIGNALS, as long as they do use ENVOY_OBJECT_TRACE_ON_DUMP.

Risk Level: Low
Testing: bazel test //test/...
Docs Changes: N/A
Release Notes: Added
Fixes envoyproxy#11984 

Signed-off-by: Michael Behr <[email protected]>

Author: mkbehr

docs/root/version_history/current.rst

@@ -63,6 +63,7 @@ New Features
 * router: added new
   :ref:`envoy-ratelimited<config_http_filters_router_retry_policy-envoy-ratelimited>`
   retry policy, which allows retrying envoy's own rate limited responses.
+* signal: added support for calling fatal error handlers without envoy's signal handler, via FatalErrorHandler::callFatalErrorHandlers().
 * stats: added optional histograms to :ref:`cluster stats <config_cluster_manager_cluster_stats_request_response_sizes>`
   that track headers and body sizes of requests and responses.
 * stats: allow configuring histogram buckets for stats sinks and admin endpoints that support it.

source/common/event/dispatcher_impl.cc

@@ -45,19 +45,13 @@ DispatcherImpl::DispatcherImpl(const std::string& name, Buffer::WatermarkFactory
       post_cb_(base_scheduler_.createSchedulableCallback([this]() -> void { runPostCallbacks(); })),
       current_to_delete_(&to_delete_1_) {
   ASSERT(!name_.empty());
-#ifdef ENVOY_HANDLE_SIGNALS
-  SignalAction::registerFatalErrorHandler(*this);
-#endif
+  FatalErrorHandler::registerFatalErrorHandler(*this);
   updateApproximateMonotonicTimeInternal();
   base_scheduler_.registerOnPrepareCallback(
       std::bind(&DispatcherImpl::updateApproximateMonotonicTime, this));
 }
 
-DispatcherImpl::~DispatcherImpl() {
-#ifdef ENVOY_HANDLE_SIGNALS
-  SignalAction::removeFatalErrorHandler(*this);
-#endif
-}
+DispatcherImpl::~DispatcherImpl() { FatalErrorHandler::removeFatalErrorHandler(*this); }
 
 void DispatcherImpl::initializeStats(Stats::Scope& scope,
                                      const absl::optional<std::string>& prefix) {

source/common/event/dispatcher_impl.h

@@ -80,12 +80,12 @@ class DispatcherImpl : Logger::Loggable<Logger::Id::main>,
   void updateApproximateMonotonicTime() override;
 
   // FatalErrorInterface
-  void onFatalError() const override {
+  void onFatalError(std::ostream& os) const override {
     // Dump the state of the tracked object if it is in the current thread. This generally results
     // in dumping the active state only for the thread which caused the fatal error.
     if (isThreadSafe()) {
       if (current_object_) {
-        current_object_->dumpState(std::cerr);
+        current_object_->dumpState(os);
       }
     }
   }

source/common/signal/BUILD

@@ -10,6 +10,7 @@ envoy_package()
 
 envoy_cc_library(
     name = "fatal_error_handler_lib",
+    srcs = ["fatal_error_handler.cc"],
     hdrs = ["fatal_error_handler.h"],
 )
 

source/common/signal/fatal_error_handler.cc

@@ -0,0 +1,72 @@
+#include "common/signal/fatal_error_handler.h"
+
+#include <list>
+
+#include "absl/base/attributes.h"
+#include "absl/synchronization/mutex.h"
+
+namespace Envoy {
+namespace FatalErrorHandler {
+
+namespace {
+
+ABSL_CONST_INIT static absl::Mutex failure_mutex(absl::kConstInit);
+// Since we can't grab the failure mutex on fatal error (snagging locks under
+// fatal crash causing potential deadlocks) access the handler list as an atomic
+// operation, which is async-signal-safe. If the crash handler runs at the same
+// time as another thread tries to modify the list, one of them will get the
+// list and the other will get nullptr instead. If the crash handler loses the
+// race and gets nullptr, it won't run any of the registered error handlers.
+using FailureFunctionList = std::list<const FatalErrorHandlerInterface*>;
+ABSL_CONST_INIT std::atomic<FailureFunctionList*> fatal_error_handlers{nullptr};
+
+} // namespace
+
+void registerFatalErrorHandler(const FatalErrorHandlerInterface& handler) {
+#ifdef ENVOY_OBJECT_TRACE_ON_DUMP
+  absl::MutexLock l(&failure_mutex);
+  FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed);
+  if (list == nullptr) {
+    list = new FailureFunctionList;
+  }
+  list->push_back(&handler);
+  fatal_error_handlers.store(list, std::memory_order_release);
+#else
+  UNREFERENCED_PARAMETER(handler);
+#endif
+}
+
+void removeFatalErrorHandler(const FatalErrorHandlerInterface& handler) {
+#ifdef ENVOY_OBJECT_TRACE_ON_DUMP
+  absl::MutexLock l(&failure_mutex);
+  FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed);
+  if (list == nullptr) {
+    // removeFatalErrorHandler() may see an empty list of fatal error handlers
+    // if it's called at the same time as callFatalErrorHandlers(). In that case
+    // Envoy is in the middle of crashing anyway, but don't add a segfault on
+    // top of the crash.
+    return;
+  }
+  list->remove(&handler);
+  if (list->empty()) {
+    delete list;
+  } else {
+    fatal_error_handlers.store(list, std::memory_order_release);
+  }
+#else
+  UNREFERENCED_PARAMETER(handler);
+#endif
+}
+
+void callFatalErrorHandlers(std::ostream& os) {
+  FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed);
+  if (list != nullptr) {
+    for (const auto* handler : *list) {
+      handler->onFatalError(os);
+    }
+    delete list;
+  }
+}
+
+} // namespace FatalErrorHandler
+} // namespace Envoy

source/common/signal/fatal_error_handler.h

@@ -1,18 +1,39 @@
 #pragma once
 
+#include <ostream>
+
 #include "envoy/common/pure.h"
 
 namespace Envoy {
 
 // A simple class which allows registering functions to be called when Envoy
-// receives one of the fatal signals, documented below.
-//
-// This is split out of signal_action.h because it is exempted from various
-// builds.
+// receives one of the fatal signals, documented in signal_action.h.
 class FatalErrorHandlerInterface {
 public:
   virtual ~FatalErrorHandlerInterface() = default;
-  virtual void onFatalError() const PURE;
+  // Called when Envoy receives a fatal signal. Must be async-signal-safe: in
+  // particular, it can't allocate memory.
+  virtual void onFatalError(std::ostream& os) const PURE;
 };
 
+namespace FatalErrorHandler {
+/**
+ * Add this handler to the list of functions which will be called if Envoy
+ * receives a fatal signal.
+ */
+void registerFatalErrorHandler(const FatalErrorHandlerInterface& handler);
+
+/**
+ * Removes this handler from the list of functions which will be called if Envoy
+ * receives a fatal signal.
+ */
+void removeFatalErrorHandler(const FatalErrorHandlerInterface& handler);
+
+/**
+ * Calls and unregisters the fatal error handlers registered with
+ * registerFatalErrorHandler. This is async-signal-safe and intended to be
+ * called from a fatal signal handler.
+ */
+void callFatalErrorHandlers(std::ostream& os);
+} // namespace FatalErrorHandler
 } // namespace Envoy

source/common/signal/signal_action.cc

@@ -9,46 +9,6 @@
 
 namespace Envoy {
 
-ABSL_CONST_INIT static absl::Mutex failure_mutex(absl::kConstInit);
-// Since we can't grab the failure mutex on fatal error (snagging locks under
-// fatal crash causing potential deadlocks) access the handler list as an atomic
-// operation, to minimize the chance that one thread is operating on the list
-// while the crash handler is attempting to access it.
-// This basically makes edits to the list thread-safe - if one thread is
-// modifying the list rather than crashing in the crash handler due to accessing
-// the list in a non-thread-safe manner, it simply won't log crash traces.
-using FailureFunctionList = std::list<const FatalErrorHandlerInterface*>;
-ABSL_CONST_INIT std::atomic<FailureFunctionList*> fatal_error_handlers{nullptr};
-
-void SignalAction::registerFatalErrorHandler(const FatalErrorHandlerInterface& handler) {
-#ifdef ENVOY_OBJECT_TRACE_ON_DUMP
-  absl::MutexLock l(&failure_mutex);
-  FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed);
-  if (list == nullptr) {
-    list = new FailureFunctionList;
-  }
-  list->push_back(&handler);
-  fatal_error_handlers.store(list, std::memory_order_release);
-#else
-  UNREFERENCED_PARAMETER(handler);
-#endif
-}
-
-void SignalAction::removeFatalErrorHandler(const FatalErrorHandlerInterface& handler) {
-#ifdef ENVOY_OBJECT_TRACE_ON_DUMP
-  absl::MutexLock l(&failure_mutex);
-  FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed);
-  list->remove(&handler);
-  if (list->empty()) {
-    delete list;
-  } else {
-    fatal_error_handlers.store(list, std::memory_order_release);
-  }
-#else
-  UNREFERENCED_PARAMETER(handler);
-#endif
-}
-
 constexpr int SignalAction::FATAL_SIGS[];
 
 void SignalAction::sigHandler(int sig, siginfo_t* info, void* context) {
@@ -62,13 +22,8 @@ void SignalAction::sigHandler(int sig, siginfo_t* info, void* context) {
   }
   tracer.logTrace();
 
-  FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed);
-  if (list) {
-    // Finally after logging the stack trace, call any registered crash handlers.
-    for (const auto* handler : *list) {
-      handler->onFatalError();
-    }
-  }
+  // Finally after logging the stack trace, call any registered crash handlers.
+  FatalErrorHandler::callFatalErrorHandlers(std::cerr);
 
   signal(sig, SIG_DFL);
   raise(sig);

source/common/signal/signal_action.h

@@ -73,18 +73,6 @@ class SignalAction : NonCopyable {
    */
   static void sigHandler(int sig, siginfo_t* info, void* context);
 
-  /**
-   * Add this handler to the list of functions which will be called if Envoy
-   * receives a fatal signal.
-   */
-  static void registerFatalErrorHandler(const FatalErrorHandlerInterface& handler);
-
-  /**
-   * Removes this handler from the list of functions which will be called if Envoy
-   * receives a fatal signal.
-   */
-  static void removeFatalErrorHandler(const FatalErrorHandlerInterface& handler);
-
 private:
   /**
    * Allocate this many bytes on each side of the area used for alt stack.
@@ -142,7 +130,6 @@ class SignalAction : NonCopyable {
   char* altstack_{};
   std::array<struct sigaction, sizeof(FATAL_SIGS) / sizeof(int)> previous_handlers_;
   stack_t previous_altstack_;
-  std::list<const FatalErrorHandlerInterface*> fatal_error_handlers_;
 };
 
 } // namespace Envoy

test/common/event/dispatcher_impl_test.cc

@@ -375,7 +375,7 @@ TEST_F(DispatcherImplTest, TimerWithScope) {
       timer = dispatcher_->createTimer([this]() {
         {
           Thread::LockGuard lock(mu_);
-          static_cast<DispatcherImpl*>(dispatcher_.get())->onFatalError();
+          static_cast<DispatcherImpl*>(dispatcher_.get())->onFatalError(std::cerr);
           work_finished_ = true;
         }
         cv_.notifyOne();

test/common/signal/BUILD

@@ -17,7 +17,9 @@ envoy_cc_test(
         "skip_on_windows",
     ],
     deps = [
+        "//source/common/signal:fatal_error_handler_lib",
         "//source/common/signal:sigaction_lib",
+        "//test/common/stats:stat_test_utility_lib",
         "//test/test_common:utility_lib",
     ],
 )

test/common/signal/signals_test.cc

@@ -2,8 +2,10 @@
 
 #include <csignal>
 
+#include "common/signal/fatal_error_handler.h"
 #include "common/signal/signal_action.h"
 
+#include "test/common/stats/stat_test_utility.h"
 #include "test/test_common/utility.h"
 
 #include "gtest/gtest.h"
@@ -19,6 +21,12 @@ namespace Envoy {
 #define ASANITIZED /* Sanitized by GCC */
 #endif
 
+// Use this test handler instead of a mock, because fatal error handlers must be
+// signal-safe and a mock might allocate memory.
+class TestFatalErrorHandler : public FatalErrorHandlerInterface {
+  void onFatalError(std::ostream& os) const override { os << "HERE!"; }
+};
+
 // Death tests that expect a particular output are disabled under address sanitizer.
 // The sanitizer does its own special signal handling and prints messages that are
 // not ours instead of what this test expects. As of latest Clang this appears
@@ -35,13 +43,9 @@ TEST(SignalsDeathTest, InvalidAddressDeathTest) {
       "backtrace.*Segmentation fault");
 }
 
-class TestFatalErrorHandler : public FatalErrorHandlerInterface {
-  void onFatalError() const override { std::cerr << "HERE!"; }
-};
-
 TEST(SignalsDeathTest, RegisteredHandlerTest) {
   TestFatalErrorHandler handler;
-  SignalAction::registerFatalErrorHandler(handler);
+  FatalErrorHandler::registerFatalErrorHandler(handler);
   SignalAction actions;
   // Make sure the fatal error log "HERE" registered above is logged on fatal error.
   EXPECT_DEATH(
@@ -51,7 +55,7 @@ TEST(SignalsDeathTest, RegisteredHandlerTest) {
         *(nasty_ptr) = 0; // NOLINT(clang-analyzer-core.NullDereference)
       }(),
       "HERE");
-  SignalAction::removeFatalErrorHandler(handler);
+  FatalErrorHandler::removeFatalErrorHandler(handler);
 }
 
 TEST(SignalsDeathTest, BusDeathTest) {
@@ -145,4 +149,60 @@ TEST(Signals, HandlerTest) {
   SignalAction::sigHandler(SIGURG, &fake_si, nullptr);
 }
 
+TEST(FatalErrorHandler, CallHandler) {
+  // Reserve space in advance so that the handler doesn't allocate memory.
+  std::string s;
+  s.reserve(1024);
+  std::ostringstream os(std::move(s));
+
+  TestFatalErrorHandler handler;
+  FatalErrorHandler::registerFatalErrorHandler(handler);
+
+  FatalErrorHandler::callFatalErrorHandlers(os);
+  EXPECT_EQ(os.str(), "HERE!");
+
+  // callFatalErrorHandlers() will unregister the handler, so this isn't
+  // necessary for cleanup. Call it anyway, to simulate the case when one thread
+  // tries to remove the handler while another thread crashes.
+  FatalErrorHandler::removeFatalErrorHandler(handler);
+}
+
+// Use this specialized test handler instead of a mock, because fatal error
+// handlers must be signal-safe and a mock might allocate memory.
+class MemoryCheckingFatalErrorHandler : public FatalErrorHandlerInterface {
+public:
+  MemoryCheckingFatalErrorHandler(const Stats::TestUtil::MemoryTest& memory_test,
+                                  uint64_t& allocated_after_call)
+      : memory_test_(memory_test), allocated_after_call_(allocated_after_call) {}
+  void onFatalError(std::ostream& os) const override {
+    UNREFERENCED_PARAMETER(os);
+    allocated_after_call_ = memory_test_.consumedBytes();
+  }
+
+private:
+  const Stats::TestUtil::MemoryTest& memory_test_;
+  uint64_t& allocated_after_call_;
+};
+
+// FatalErrorHandler::callFatalErrorHandlers shouldn't allocate any heap memory,
+// so that it's safe to call from a signal handler. Test by comparing the
+// allocated memory before a call with the allocated memory during a handler.
+TEST(FatalErrorHandler, DontAllocateMemory) {
+  // Reserve space in advance so that the handler doesn't allocate memory.
+  std::string s;
+  s.reserve(1024);
+  std::ostringstream os(std::move(s));
+
+  Stats::TestUtil::MemoryTest memory_test;
+
+  uint64_t allocated_after_call;
+  MemoryCheckingFatalErrorHandler handler(memory_test, allocated_after_call);
+  FatalErrorHandler::registerFatalErrorHandler(handler);
+
+  uint64_t allocated_before_call = memory_test.consumedBytes();
+  FatalErrorHandler::callFatalErrorHandlers(os);
+
+  EXPECT_MEMORY_EQ(allocated_after_call, allocated_before_call);
+}
+
 } // namespace Envoy