fix: guard against null thread handle for profiler sampling thread (#…

…4076)

Sources/Sentry/SentryBacktrace.cpp

@@ -102,11 +102,24 @@ namespace profiling {
     }
 
     void
-    enumerateBacktracesForAllThreads(const std::function<void(const Backtrace &)> &f,
-        const std::shared_ptr<ThreadMetadataCache> &cache)
+    enumerateBacktracesForThreads(const std::function<void(const Backtrace &)> &f,
+        const std::shared_ptr<ThreadMetadataCache> &cache,
+        const std::vector<std::unique_ptr<ThreadHandle>> &threads,
+        const std::unique_ptr<ThreadHandle> &currentThread)
     {
-        const auto pair = ThreadHandle::allExcludingCurrent();
-        for (const auto &thread : pair.first) {
+        if (currentThread == nullptr) {
+            SENTRY_PROF_LOG_WARN("Current thread handle (profiler's sampling thread) was null, "
+                                 "will not attempt to gather other threads' backtraces.");
+            return;
+        }
+
+        for (const auto &thread : threads) {
+            if (thread == nullptr) {
+                SENTRY_PROF_LOG_WARN(
+                    "Thread handle was null, will not attempt to gather its backtrace.");
+                continue;
+            }
+
             Backtrace bt;
             // This one is probably safe to call while the thread is suspended, but
             // being conservative here in case the platform time functions take any
@@ -156,7 +169,7 @@ namespace profiling {
 
             bool reachedEndOfStack = false;
             std::uintptr_t addresses[kMaxBacktraceDepth];
-            const auto depth = backtrace(*thread, *pair.second, addresses, stackBounds,
+            const auto depth = backtrace(*thread, *currentThread, addresses, stackBounds,
                 &reachedEndOfStack, kMaxBacktraceDepth, 0);
 
             // Retrieving queue metadata *must* be done after suspending the thread,

Sources/Sentry/SentrySamplingProfiler.cpp

@@ -64,7 +64,18 @@ namespace profiling {
                 }
 
                 params->numSamples.fetch_add(1, std::memory_order_relaxed);
-                enumerateBacktracesForAllThreads(params->callback, params->cache);
+
+                const auto current = ThreadHandle::current();
+                if (current == nullptr) {
+                    SENTRY_PROF_LOG_WARN(
+                        "Current thread (the profiler's sampling thread) handle returned as null, "
+                        "will not attempt to gather further backtraces.");
+                    break;
+                }
+
+                const auto threads = ThreadHandle::allExcludingCurrent();
+                enumerateBacktracesForThreads(
+                    params->callback, params->cache, std::move(threads), std::move(current));
             }
             pthread_cleanup_pop(1);
             pthread_cleanup_pop(1);

Sources/Sentry/SentryThreadHandle.cpp

@@ -70,15 +70,15 @@ namespace profiling {
         return threads;
     }
 
-    std::pair<std::vector<std::unique_ptr<ThreadHandle>>, std::unique_ptr<ThreadHandle>>
+    std::vector<std::unique_ptr<ThreadHandle>>
     ThreadHandle::allExcludingCurrent() noexcept
     {
         std::vector<std::unique_ptr<ThreadHandle>> threads;
         mach_msg_type_number_t count;
         thread_act_array_t list;
         auto current = ThreadHandle::current();
         if (current == nullptr) {
-            return std::make_pair(threads, nullptr);
+            return threads;
         }
 
         if (SENTRY_PROF_LOG_KERN_RETURN(task_threads(mach_task_self(), &list, &count))
@@ -95,7 +95,7 @@ namespace profiling {
         }
         SENTRY_PROF_LOG_KERN_RETURN(vm_deallocate(
             mach_task_self(), reinterpret_cast<vm_address_t>(list), sizeof(*list) * count));
-        return std::make_pair(std::move(threads), std::move(current));
+        return threads;
     }
 
     thread::TIDType

Sources/Sentry/include/SentryBacktrace.hpp

@@ -57,9 +57,13 @@ namespace profiling {
      *
      * @param f The function to call for each entry.
      * @param cache The cache used to look up thread metadata.
+     * @param threads A vector containing handles of all threads except the current thread.
+     * @param currentThread The handle of the current thread.
      */
-    void enumerateBacktracesForAllThreads(const std::function<void(const Backtrace &)> &f,
-        const std::shared_ptr<ThreadMetadataCache> &cache);
+    void enumerateBacktracesForThreads(const std::function<void(const Backtrace &)> &f,
+        const std::shared_ptr<ThreadMetadataCache> &cache,
+        const std::vector<std::unique_ptr<ThreadHandle>> &threads,
+        const std::unique_ptr<ThreadHandle> &currentThread);
 
 } // namespace profiling
 } // namespace sentry

Sources/Sentry/include/SentryThreadHandle.hpp

@@ -67,15 +67,13 @@ namespace profiling {
         static std::vector<std::unique_ptr<ThreadHandle>> all() noexcept;
 
         /**
-         * @return A pair, where the first element is a vector of handles for all of
+         * @return A vector of handles for all of
          * the threads in the current process, excluding the current thread (the
-         * thread that this function is being called on), and the second element
-         * is a handle to the current thread.
+         * thread that this function is being called on).
          *
-         * Returns ({}, nullptr) if the current thread could not be retrieved.
+         * @note Returns An empty vector if the current thread could not be retrieved.
          */
-        static std::pair<std::vector<std::unique_ptr<ThreadHandle>>, std::unique_ptr<ThreadHandle>>
-        allExcludingCurrent() noexcept;
+        static std::vector<std::unique_ptr<ThreadHandle>> allExcludingCurrent() noexcept;
 
         /**
          * @param handle The native handle to get the TID from.

Sources/SentryCrash/Recording/Monitors/SentryCrashMonitorType.c

@@ -31,10 +31,7 @@ static const struct {
     const SentryCrashMonitorType type;
     const char *const name;
 } g_monitorTypes[] = {
-#define MONITORTYPE(NAME)                                                                          \
-    {                                                                                              \
-        NAME, #NAME                                                                                \
-    }
+#define MONITORTYPE(NAME) { NAME, #NAME }
     MONITORTYPE(SentryCrashMonitorTypeMachException),
     MONITORTYPE(SentryCrashMonitorTypeSignal),
     MONITORTYPE(SentryCrashMonitorTypeCPPException),

Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_CPPException.cpp

@@ -135,8 +135,7 @@ CPPExceptionTerminate(void)
             strncpy(descriptionBuff, exc.what(), sizeof(descriptionBuff));
         }
 #define CATCH_VALUE(TYPE, PRINTFTYPE)                                                              \
-    catch (TYPE value)                                                                             \
-    {                                                                                              \
+    catch (TYPE value) {                                                                           \
         snprintf(descriptionBuff, sizeof(descriptionBuff), "%" #PRINTFTYPE, value);                \
     }
         CATCH_VALUE(char, d)

Sources/SentryCrash/Recording/Tools/SentryCrashSignalInfo.c

@@ -42,10 +42,7 @@ typedef struct {
     const int numCodes;
 } SentryCrashSignalInfo;
 
-#define ENUM_NAME_MAPPING(A)                                                                       \
-    {                                                                                              \
-        A, #A                                                                                      \
-    }
+#define ENUM_NAME_MAPPING(A) { A, #A }
 
 static const SentryCrashSignalCodeInfo g_sigIllCodes[] = {
 #ifdef ILL_NOOP
@@ -98,14 +95,8 @@ static const SentryCrashSignalCodeInfo g_sigSegVCodes[] = {
     ENUM_NAME_MAPPING(SEGV_ACCERR),
 };
 
-#define SIGNAL_INFO(SIGNAL, CODES)                                                                 \
-    {                                                                                              \
-        SIGNAL, #SIGNAL, CODES, sizeof(CODES) / sizeof(*CODES)                                     \
-    }
-#define SIGNAL_INFO_NOCODES(SIGNAL)                                                                \
-    {                                                                                              \
-        SIGNAL, #SIGNAL, 0, 0                                                                      \
-    }
+#define SIGNAL_INFO(SIGNAL, CODES) { SIGNAL, #SIGNAL, CODES, sizeof(CODES) / sizeof(*CODES) }
+#define SIGNAL_INFO_NOCODES(SIGNAL) { SIGNAL, #SIGNAL, 0, 0 }
 
 static const SentryCrashSignalInfo g_fatalSignalData[] = {
     SIGNAL_INFO_NOCODES(SIGABRT),

Tests/SentryProfilerTests/SentryBacktraceTests.mm

@@ -203,7 +203,7 @@ - (void)testCollectsMultiThreadBacktrace
     bool foundThread1 = false, foundThread2 = false;
     // Try up to 3 times.
     for (int i = 0; i < 3; i++) {
-        enumerateBacktracesForAllThreads(
+        enumerateBacktracesForThreads(
             [&](auto &backtrace) {
                 const auto thread = backtrace.threadMetadata.threadID;
                 if (thread == pthread_mach_thread_np(thread1)) {
@@ -230,7 +230,7 @@ - (void)testCollectsMultiThreadBacktrace
                     }
                 }
             },
-            cache);
+            cache, ThreadHandle::allExcludingCurrent(), ThreadHandle::current());
         if (foundThread1 && foundThread2) {
             break;
         }
@@ -247,6 +247,27 @@ - (void)testCollectsMultiThreadBacktrace
     XCTAssertTrue(foundThread2);
 }
 
+- (void)testDoesNotCollectBacktracesWhenCurrentThreadHandleIsNull
+{
+    pthread_t thread1, thread2;
+    XCTAssertEqual(
+        pthread_create(&thread1, nullptr, threadEntry, reinterpret_cast<void *>(bc_a)), 0);
+    XCTAssertEqual(
+        pthread_create(&thread2, nullptr, threadEntry, reinterpret_cast<void *>(bc_d)), 0);
+
+    const auto cache = std::make_shared<ThreadMetadataCache>();
+    enumerateBacktracesForThreads(
+        [&](__unused auto &backtrace) {
+            XCTFail("Should not attempt to collect backtraces if current thread handle is null");
+        },
+        cache, ThreadHandle::allExcludingCurrent(), nullptr /* currentThread */);
+
+    XCTAssertEqual(pthread_cancel(thread1), 0);
+    XCTAssertEqual(pthread_join(thread1, nullptr), 0);
+    XCTAssertEqual(pthread_cancel(thread2), 0);
+    XCTAssertEqual(pthread_join(thread2, nullptr), 0);
+}
+
 @end
 
 #endif

Tests/SentryProfilerTests/SentryThreadHandleTests.mm

@@ -104,9 +104,8 @@ - (void)testAllExcludingCurrent
     XCTAssertEqual(pthread_create(&thread2, nullptr, threadSpin, nullptr), 0);
 
     bool foundThread1 = false, foundThread2 = false, foundCurrentThread = false;
-    const auto pair = ThreadHandle::allExcludingCurrent();
-    XCTAssertEqual(pair.second->nativeHandle(), currentMachThread());
-    for (const auto &thread : pair.first) {
+    const auto threads = ThreadHandle::allExcludingCurrent();
+    for (const auto &thread : threads) {
         const auto pt = pthread_from_mach_thread_np(thread->nativeHandle());
         if (pthread_equal(pt, thread1)) {
             foundThread1 = true;

Tests/SentryTests/SentryCrash/CrashReport.swift

@@ -9,7 +9,7 @@ extension XCTestCase {
 
     func givenStoredSentryCrashReport(resource: String) throws {
         let jsonData = try jsonDataOfResource(resource: resource)
-        jsonData.withUnsafeBytes { ( bytes: UnsafeRawBufferPointer) -> Void in
+        jsonData.withUnsafeBytes { ( bytes: UnsafeRawBufferPointer) in
             let pointer = bytes.bindMemory(to: Int8.self)
             sentrycrashcrs_addUserReport(pointer.baseAddress, Int32(jsonData.count))
         }

Tests/SentryTests/SentryInterfacesTests.m

@@ -177,7 +177,7 @@ - (void)testTransactionEvent
             @1 : @"1",
             @2 : @2,
             @3 : @ { @"a" : @0 },
-            @4 : @[ @"1", @2, @{ @"a" : @0 }, @[ @"a" ], testDate, testURL ],
+            @4 : @[ @"1", @2, @ { @"a" : @0 }, @[ @"a" ], testDate, testURL ],
             @5 : testDate,
             @6 : testURL
         }
@@ -191,7 +191,7 @@ - (void)testTransactionEvent
                 @"2" : @2,
                 @"3" : @ { @"a" : @0 },
                 @"4" : @[
-                    @"1", @2, @{ @"a" : @0 }, @[ @"a" ], @"2020-02-27T11:35:26.000Z",
+                    @"1", @2, @ { @"a" : @0 }, @[ @"a" ], @"2020-02-27T11:35:26.000Z",
                     @"https://sentry.io"
                 ],
                 @"5" : @"2020-02-27T11:35:26.000Z",