From 37ed7ca8e084699740cff6ccee91ed577f3107bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bla=C5=BE=20Toma=C5=BEi=C4=8D?= <blaz.tomazic@outfit7.com>
Date: Mon, 18 Jan 2021 14:25:09 +0100
Subject: [PATCH] Fix uninstalling signal handlers

When uninstalling signal handlers disable them instead of uninstalling.
Do it this way because:

1. Signal handlers installed after ours can take our signal handling
function address and chain to it. In this case the function must always
work.
2. If multiple signal handlers for same signal are installed after ours
we can't remove ours without removing all but the last installed one.

Because we don't uninstall our signal handlers we now directly call
previously installed signal handlers.
---
 .../Sentry/BSG_KSCrashSentry_Signal.c         | 57 +++++++++++++------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_Signal.c b/Bugsnag/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_Signal.c
index 8e07bbba1..a1b158dd9 100644
--- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_Signal.c
+++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_Signal.c
@@ -50,6 +50,14 @@
  */
 static volatile sig_atomic_t bsg_g_installed = 0;
 
+/** Flag noting if we should handle signals.
+ * When other signal handlers are registered after ours we can't remove our
+ * signal handlers without removing the others.
+ * It's not fully thread safe, but it's safer than locking and slightly better
+ * than nothing.
+ */
+static volatile sig_atomic_t bsg_g_enabled = 0;
+
 #if !TARGET_OS_TV
 /** Our custom signal stack. The signal handler will use this as its stack. */
 static stack_t bsg_g_signalStack = {0};
@@ -82,7 +90,7 @@ static BSG_KSCrash_SentryContext *bsg_g_context;
 void bsg_kssighndl_i_handleSignal(int sigNum, siginfo_t *signalInfo,
                                   void *userContext) {
     BSG_KSLOG_DEBUG("Trapped signal %d", sigNum);
-    if (bsg_g_installed) {
+    if (bsg_g_enabled) {
         bool wasHandlingCrash = bsg_g_context->handlingCrash;
         bsg_kscrashsentry_beginHandlingCrash(bsg_g_context);
 
@@ -116,9 +124,17 @@ void bsg_kssighndl_i_handleSignal(int sigNum, siginfo_t *signalInfo,
         bsg_kscrashsentry_resumeThreads();
     }
 
-    BSG_KSLOG_DEBUG("Re-raising signal for regular handlers to catch.");
-    // This is technically not allowed, but it works in OSX and iOS.
-    raise(sigNum);
+    BSG_KSLOG_DEBUG(
+        "Re-raising or chaining signal for regular handlers to catch.");
+    struct sigaction previous = bsg_g_previousSignalHandlers[sigNum];
+    if (previous.sa_flags & SA_SIGINFO) {
+        previous.sa_sigaction(sigNum, signalInfo, userContext);
+    } else if (previous.sa_handler == SIG_DFL) {
+        // This is technically not allowed, but it works in OSX and iOS.
+        raise(sigNum);
+    } else if (previous.sa_handler != SIG_IGN) {
+        previous.sa_handler(sigNum);
+    }
 }
 
 // ============================================================================
@@ -129,6 +145,10 @@ bool bsg_kscrashsentry_installSignalHandler(
     BSG_KSCrash_SentryContext *context) {
     BSG_KSLOG_DEBUG("Installing signal handler.");
 
+    if (!bsg_g_enabled) {
+        bsg_g_enabled = 1;
+        BSG_KSLOG_DEBUG("Signal handlers enabled.");
+    }
     if (bsg_g_installed) {
         BSG_KSLOG_DEBUG("Signal handler already installed.");
         return true;
@@ -194,26 +214,29 @@ bool bsg_kscrashsentry_installSignalHandler(
 
 failed:
     BSG_KSLOG_DEBUG("Failed to install signal handlers.");
+    bsg_g_enabled = 0;
     bsg_g_installed = 0;
     return false;
 }
 
 void bsg_kscrashsentry_uninstallSignalHandler(void) {
     BSG_KSLOG_DEBUG("Uninstalling signal handlers.");
-    if (!bsg_g_installed) {
-        BSG_KSLOG_DEBUG("Signal handlers were already uninstalled.");
+    // We only disable signal handling but don't uninstall the signal handlers.
+    //
+    // The probblem is that we can safely uninstall signal handlers only when we
+    // are the last one registered. If we are not we can't know how many
+    // handlers were registered after us to re-register them. Also other
+    // handlers could save our handler to chain the signal and our handler will
+    // be called even when "uninstalled".
+    //
+    // Therefore keep the signal handlers installed and just disable the
+    // handling. The installed signal handlers still chains the signal even when
+    // not handling.
+    if (!bsg_g_enabled) {
+        BSG_KSLOG_DEBUG("Signal handlers were already disabled.");
         return;
     }
 
-    const int *fatalSignals = bsg_kssignal_fatalSignals();
-    int fatalSignalsCount = bsg_kssignal_numFatalSignals();
-
-    for (int i = 0; i < fatalSignalsCount; i++) {
-        BSG_KSLOG_DEBUG("Restoring original handler for signal %d",
-                        fatalSignals[i]);
-        sigaction(fatalSignals[i], &bsg_g_previousSignalHandlers[i], NULL);
-    }
-
-    BSG_KSLOG_DEBUG("Signal handlers uninstalled.");
-    bsg_g_installed = 0;
+    BSG_KSLOG_DEBUG("Signal handlers disabled.");
+    bsg_g_enabled = 0;
 }