Check MACH_PORT_VALID on return from pthread_mach_thread_np

CHANGELOG.md

@@ -1,6 +1,10 @@
 # Changelog
 
 ## Unreleased
+
+### Fixes
+
+- Check MACH_PORT_VALID on return from pthread_mach_thread_np (#3520)
 
 ### Features
 

Sources/Sentry/PrivateSentrySDKOnly.mm

@@ -140,11 +140,10 @@ + (uint64_t)startProfilerForTrace:(SentryId *)traceId;
                                         onHub:[SentrySDK currentHub]];
 
     if (payload != nil) {
+        const auto thread = sentry::profiling::ThreadHandle::current();
         payload[@"platform"] = SentryPlatformName;
-        payload[@"transaction"] = @{
-            @"active_thread_id" :
-                [NSNumber numberWithLongLong:sentry::profiling::ThreadHandle::current()->tid()]
-        };
+        payload[@"transaction"] =
+            @{ @"active_thread_id" : (thread == nullptr) ? @0 : @(thread->tid()) };
     }
 
     return payload;

Sources/Sentry/SentryBacktrace.cpp

@@ -102,11 +102,24 @@
     }
 
     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 @@
 
             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 @@
                 }
 
                 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

@@ -45,6 +45,9 @@ namespace profiling {
     ThreadHandle::current() noexcept
     {
         const auto thread = pthread_mach_thread_np(pthread_self());
+        if (!MACH_PORT_VALID(thread)) {
+            return nullptr;
+        }
         return std::make_unique<ThreadHandle>(thread);
     }
 
@@ -67,13 +70,17 @@ 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 threads;
+        }
+
         if (SENTRY_PROF_LOG_KERN_RETURN(task_threads(mach_task_self(), &list, &count))
             == KERN_SUCCESS) {
             for (decltype(count) i = 0; i < count; i++) {
@@ -88,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/SentryTransactionContext.mm

@@ -124,8 +124,9 @@ - (instancetype)initWithName:(NSString *)name
 - (void)getThreadInfo
 {
 #if SENTRY_TARGET_PROFILING_SUPPORTED
-    const auto threadID = sentry::profiling::ThreadHandle::current()->tid();
-    self.threadInfo = [[SentryThread alloc] initWithThreadId:@(threadID)];
+    const auto thread = sentry::profiling::ThreadHandle::current();
+    self.threadInfo =
+        [[SentryThread alloc] initWithThreadId:(thread == nullptr) ? @0 : @(thread->tid())];
 #endif
 }
 

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

@@ -56,6 +56,8 @@ namespace profiling {
         /**
          * @return A handle to the currently executing thread, which is acquired
          * in a non async-signal-safe manner.
+         *
+         * Returns nullptr if the current thread could not be retrieved.
          */
         static std::unique_ptr<ThreadHandle> current() noexcept;
 
@@ -65,13 +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).
+         *
+         * @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 @@
             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 @@
     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 @@
                     }
                 }
             },
-            cache);
+            cache, ThreadHandle::allExcludingCurrent(), ThreadHandle::current());
         if (foundThread1 && foundThread2) {
             break;
         }
@@ -247,6 +247,27 @@
     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",