fix: App Hang report crashes with too many threads (#2811)

Add a limit of 70 threads to report App hang. More than 100 is causing the SDK to crash, we add an extra safety margin. Co-authored-by: Dhiogo Ramos Brustolin <dhiogo.brustolin@sentry.io> Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
getsentry · Mar 20, 2023 · cd3bfeb · cd3bfeb
1 parent 228a909
commit cd3bfeb
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 1 deletion.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 ### Fixes
 
 - Updating AppHang state on main thread (#2793)
+- App Hang report crashes with too many threads (#2811)
 
 ### Improvements
 

diff --git a/Sources/Sentry/SentryThreadInspector.m b/Sources/Sentry/SentryThreadInspector.m
@@ -119,10 +119,20 @@ - (SentryStacktrace *)stacktraceForCurrentThreadAsyncUnsafe
         thread_act_array_t suspendedThreads = NULL;
         mach_msg_type_number_t numSuspendedThreads = 0;
 
-        sentrycrashmc_suspendEnvironment(&suspendedThreads, &numSuspendedThreads);
+        // SentryThreadInspector is crashing when there is too many threads.
+        // We add a limit of 70 threads because in test with up to 100 threads it seems fine.
+        // We are giving it an extra safety margin.
+        sentrycrashmc_suspendEnvironment_upToMaxSupportedThreads(
+            &suspendedThreads, &numSuspendedThreads, 70);
         // DANGER: Do not try to allocate memory in the heap or call Objective-C code in this
         // section Doing so when the threads are suspended may lead to deadlocks or crashes.
 
+        // If no threads was suspended we don't need to do anything.
+        // This may happen if there is more than max amount of threads (70).
+        if (numSuspendedThreads == 0) {
+            return threads;
+        }
+
         SentryThreadInfo threadsInfos[numSuspendedThreads];
 
         for (int i = 0; i < numSuspendedThreads; i++) {

diff --git a/Sources/SentryCrash/Recording/Tools/SentryCrashMachineContext.c b/Sources/SentryCrash/Recording/Tools/SentryCrashMachineContext.c
@@ -146,6 +146,14 @@ sentrycrashmc_getContextForSignal(
 void
 sentrycrashmc_suspendEnvironment(
     thread_act_array_t *suspendedThreads, mach_msg_type_number_t *numSuspendedThreads)
+{
+    sentrycrashmc_suspendEnvironment_upToMaxSupportedThreads(
+        suspendedThreads, numSuspendedThreads, UINT32_MAX);
+}
+
+void
+sentrycrashmc_suspendEnvironment_upToMaxSupportedThreads(thread_act_array_t *suspendedThreads,
+    mach_msg_type_number_t *numSuspendedThreads, mach_msg_type_number_t maxSupportedThreads)
 {
 #if SentryCrashCRASH_HAS_THREADS_API
     SentryCrashLOG_DEBUG("Suspending environment.");
@@ -158,6 +166,12 @@ sentrycrashmc_suspendEnvironment(
         return;
     }
 
+    if (*numSuspendedThreads > maxSupportedThreads) {
+        *numSuspendedThreads = 0;
+        SentryCrashLOG_DEBUG("Too many threads to suspend. Aborting operation.");
+        return;
+    }
+
     for (mach_msg_type_number_t i = 0; i < *numSuspendedThreads; i++) {
         thread_t thread = (*suspendedThreads)[i];
         if (thread != thisThread && !sentrycrashcm_isReservedThread(thread)) {

diff --git a/Sources/SentryCrash/Recording/Tools/SentryCrashMachineContext.h b/Sources/SentryCrash/Recording/Tools/SentryCrashMachineContext.h
@@ -40,6 +40,13 @@ extern "C" {
 void sentrycrashmc_suspendEnvironment(
     thread_act_array_t *suspendedThreads, mach_msg_type_number_t *numSuspendedThreads);
 
+/**
+ Suspend the runtime environment only if the amount of threads is not higher than
+ maxSupportedThreads.
+ */
+void sentrycrashmc_suspendEnvironment_upToMaxSupportedThreads(thread_act_array_t *suspendedThreads,
+    mach_msg_type_number_t *numSuspendedThreads, mach_msg_type_number_t maxSupportedThreads);
+
 /** Resume the runtime environment.
  */
 void sentrycrashmc_resumeEnvironment(thread_act_array_t threads, mach_msg_type_number_t numThreads);

diff --git a/Tests/SentryTests/SentryCrash/SentryThreadInspectorTests.swift b/Tests/SentryTests/SentryCrash/SentryThreadInspectorTests.swift
@@ -5,6 +5,7 @@ class SentryThreadInspectorTests: XCTestCase {
     private class Fixture {
         var testMachineContextWrapper = TestMachineContextWrapper()
         var stacktraceBuilder = TestSentryStacktraceBuilder(crashStackEntryMapper: SentryCrashStackEntryMapper(inAppLogic: SentryInAppLogic(inAppIncludes: [], inAppExcludes: [])))
+        var keepThreadAlive = true
 
         func getSut(testWithRealMachineContextWrapper: Bool = false) -> SentryThreadInspector {
 
@@ -80,6 +81,27 @@ class SentryThreadInspectorTests: XCTestCase {
         wait(for: [expect], timeout: 10)
     }
 
+    func testGetCurrentThreadWithStackTrack_TooManyThreads() {
+        let expect = expectation(description: "Wait all Threads")
+        expect.expectedFulfillmentCount = 70
+
+        let sut = self.fixture.getSut(testWithRealMachineContextWrapper: true)
+
+        for _ in 0..<expect.expectedFulfillmentCount {
+            Thread.detachNewThread {
+                expect.fulfill()
+                while self.fixture.keepThreadAlive {
+                    Thread.sleep(forTimeInterval: 0.001)
+                }
+            }
+        }
+
+        wait(for: [expect], timeout: 5)
+        let suspendedThreads = sut.getCurrentThreadsWithStackTrace()
+        fixture.keepThreadAlive = false
+        XCTAssertEqual(suspendedThreads.count, 0)
+    }
+
     func testStackTrackForCurrentThreadAsyncUnsafe() {
         guard let stackTrace = fixture.getSut(testWithRealMachineContextWrapper: true).stacktraceForCurrentThreadAsyncUnsafe() else {
             XCTFail("Stack Trace not found")