From 4b8cfd715d65200369cd7d86b490c3b34b1a621d Mon Sep 17 00:00:00 2001 From: Indragie Karunaratne Date: Wed, 27 Dec 2023 12:47:34 -0800 Subject: [PATCH 1/4] Check MACH_PORT_VALID on return from pthread_mach_thread_np pthread_mach_thread_np can return invalid values so check for those and return nullptr from ThreadHandle --- Sources/Sentry/PrivateSentrySDKOnly.mm | 4 ++-- Sources/Sentry/SentryThreadHandle.cpp | 7 +++++++ Sources/Sentry/SentryTransactionContext.mm | 4 ++-- Sources/Sentry/include/SentryThreadHandle.hpp | 4 ++++ 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/Sources/Sentry/PrivateSentrySDKOnly.mm b/Sources/Sentry/PrivateSentrySDKOnly.mm index bff864d12b3..60f1435d32e 100644 --- a/Sources/Sentry/PrivateSentrySDKOnly.mm +++ b/Sources/Sentry/PrivateSentrySDKOnly.mm @@ -140,10 +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()] + @"active_thread_id" : (thread == nullptr) ? @0 : @(thread->tid()) }; } diff --git a/Sources/Sentry/SentryThreadHandle.cpp b/Sources/Sentry/SentryThreadHandle.cpp index 2aa44ac6530..2b7688043d0 100644 --- a/Sources/Sentry/SentryThreadHandle.cpp +++ b/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(thread); } @@ -74,6 +77,10 @@ namespace profiling { mach_msg_type_number_t count; thread_act_array_t list; auto current = ThreadHandle::current(); + if (current == nullptr) { + return std::make_pair(threads, nullptr); + } + if (SENTRY_PROF_LOG_KERN_RETURN(task_threads(mach_task_self(), &list, &count)) == KERN_SUCCESS) { for (decltype(count) i = 0; i < count; i++) { diff --git a/Sources/Sentry/SentryTransactionContext.mm b/Sources/Sentry/SentryTransactionContext.mm index d0b94b9fe66..b3a903bacff 100644 --- a/Sources/Sentry/SentryTransactionContext.mm +++ b/Sources/Sentry/SentryTransactionContext.mm @@ -124,8 +124,8 @@ - (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 } diff --git a/Sources/Sentry/include/SentryThreadHandle.hpp b/Sources/Sentry/include/SentryThreadHandle.hpp index fb0caa7e090..181b0193ffb 100644 --- a/Sources/Sentry/include/SentryThreadHandle.hpp +++ b/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 current() noexcept; @@ -69,6 +71,8 @@ namespace profiling { * 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. + * + * Returns ({}, nullptr) if the current thread could not be retrieved. */ static std::pair>, std::unique_ptr> allExcludingCurrent() noexcept; From b656bdeacbf17381dd2ce707b5c15ebcfd2fecbd Mon Sep 17 00:00:00 2001 From: Indragie Karunaratne Date: Wed, 27 Dec 2023 13:16:39 -0800 Subject: [PATCH 2/4] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 110234facbe..dfe816769ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - TTFD waits for next drawn frame (#3505) - Fix TTID/TTFD for app start transactions (#3512): TTID/TTFD spans and measurements for app start transaction now include the app start duration. +- Check MACH_PORT_VALID on return from pthread_mach_thread_np (#3520) ## 8.17.2 From c4496eecd28260b77e8b868f9fba876843a13075 Mon Sep 17 00:00:00 2001 From: Sentry Github Bot Date: Wed, 27 Dec 2023 21:17:44 +0000 Subject: [PATCH 3/4] Format code --- Sources/Sentry/PrivateSentrySDKOnly.mm | 5 ++--- Sources/Sentry/SentryThreadHandle.cpp | 2 +- Sources/Sentry/SentryTransactionContext.mm | 3 ++- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Sources/Sentry/PrivateSentrySDKOnly.mm b/Sources/Sentry/PrivateSentrySDKOnly.mm index 60f1435d32e..14f97a01099 100644 --- a/Sources/Sentry/PrivateSentrySDKOnly.mm +++ b/Sources/Sentry/PrivateSentrySDKOnly.mm @@ -142,9 +142,8 @@ + (uint64_t)startProfilerForTrace:(SentryId *)traceId; if (payload != nil) { const auto thread = sentry::profiling::ThreadHandle::current(); payload[@"platform"] = SentryPlatformName; - payload[@"transaction"] = @{ - @"active_thread_id" : (thread == nullptr) ? @0 : @(thread->tid()) - }; + payload[@"transaction"] = + @{ @"active_thread_id" : (thread == nullptr) ? @0 : @(thread->tid()) }; } return payload; diff --git a/Sources/Sentry/SentryThreadHandle.cpp b/Sources/Sentry/SentryThreadHandle.cpp index 2b7688043d0..35b1ade6792 100644 --- a/Sources/Sentry/SentryThreadHandle.cpp +++ b/Sources/Sentry/SentryThreadHandle.cpp @@ -80,7 +80,7 @@ namespace profiling { if (current == nullptr) { return std::make_pair(threads, nullptr); } - + if (SENTRY_PROF_LOG_KERN_RETURN(task_threads(mach_task_self(), &list, &count)) == KERN_SUCCESS) { for (decltype(count) i = 0; i < count; i++) { diff --git a/Sources/Sentry/SentryTransactionContext.mm b/Sources/Sentry/SentryTransactionContext.mm index b3a903bacff..a8483f8bf70 100644 --- a/Sources/Sentry/SentryTransactionContext.mm +++ b/Sources/Sentry/SentryTransactionContext.mm @@ -125,7 +125,8 @@ - (void)getThreadInfo { #if SENTRY_TARGET_PROFILING_SUPPORTED const auto thread = sentry::profiling::ThreadHandle::current(); - self.threadInfo = [[SentryThread alloc] initWithThreadId:(thread == nullptr) ? @0 : @(thread->tid())]; + self.threadInfo = + [[SentryThread alloc] initWithThreadId:(thread == nullptr) ? @0 : @(thread->tid())]; #endif } From 6fac4d604bae0ea614f7476e6705c20baa79b21e Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Thu, 20 Jun 2024 11:48:49 -0800 Subject: [PATCH 4/4] fix: guard against null thread handle for profiler sampling thread (#4076) --- Sources/Sentry/SentryBacktrace.cpp | 23 +++++++++++++---- Sources/Sentry/SentrySamplingProfiler.cpp | 13 +++++++++- Sources/Sentry/SentryThreadHandle.cpp | 6 ++--- Sources/Sentry/include/SentryBacktrace.hpp | 8 ++++-- Sources/Sentry/include/SentryThreadHandle.hpp | 10 +++----- .../Monitors/SentryCrashMonitorType.c | 5 +--- .../SentryCrashMonitor_CPPException.cpp | 3 +-- .../Recording/Tools/SentryCrashSignalInfo.c | 15 +++-------- .../SentryBacktraceTests.mm | 25 +++++++++++++++++-- .../SentryThreadHandleTests.mm | 5 ++-- .../SentryTests/SentryCrash/CrashReport.swift | 2 +- Tests/SentryTests/SentryInterfacesTests.m | 4 +-- 12 files changed, 76 insertions(+), 43 deletions(-) diff --git a/Sources/Sentry/SentryBacktrace.cpp b/Sources/Sentry/SentryBacktrace.cpp index ad7d65a1d22..46167b80de8 100644 --- a/Sources/Sentry/SentryBacktrace.cpp +++ b/Sources/Sentry/SentryBacktrace.cpp @@ -102,11 +102,24 @@ namespace profiling { } void - enumerateBacktracesForAllThreads(const std::function &f, - const std::shared_ptr &cache) + enumerateBacktracesForThreads(const std::function &f, + const std::shared_ptr &cache, + const std::vector> &threads, + const std::unique_ptr ¤tThread) { - 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, diff --git a/Sources/Sentry/SentrySamplingProfiler.cpp b/Sources/Sentry/SentrySamplingProfiler.cpp index 5a86de8131c..7ac747fe809 100644 --- a/Sources/Sentry/SentrySamplingProfiler.cpp +++ b/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); diff --git a/Sources/Sentry/SentryThreadHandle.cpp b/Sources/Sentry/SentryThreadHandle.cpp index 35b1ade6792..4f03c3c3fba 100644 --- a/Sources/Sentry/SentryThreadHandle.cpp +++ b/Sources/Sentry/SentryThreadHandle.cpp @@ -70,7 +70,7 @@ namespace profiling { return threads; } - std::pair>, std::unique_ptr> + std::vector> ThreadHandle::allExcludingCurrent() noexcept { std::vector> threads; @@ -78,7 +78,7 @@ namespace profiling { 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(list), sizeof(*list) * count)); - return std::make_pair(std::move(threads), std::move(current)); + return threads; } thread::TIDType diff --git a/Sources/Sentry/include/SentryBacktrace.hpp b/Sources/Sentry/include/SentryBacktrace.hpp index 272573adc96..2ced192c331 100644 --- a/Sources/Sentry/include/SentryBacktrace.hpp +++ b/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 &f, - const std::shared_ptr &cache); + void enumerateBacktracesForThreads(const std::function &f, + const std::shared_ptr &cache, + const std::vector> &threads, + const std::unique_ptr ¤tThread); } // namespace profiling } // namespace sentry diff --git a/Sources/Sentry/include/SentryThreadHandle.hpp b/Sources/Sentry/include/SentryThreadHandle.hpp index 181b0193ffb..d9442f81e02 100644 --- a/Sources/Sentry/include/SentryThreadHandle.hpp +++ b/Sources/Sentry/include/SentryThreadHandle.hpp @@ -67,15 +67,13 @@ namespace profiling { static std::vector> 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::unique_ptr> - allExcludingCurrent() noexcept; + static std::vector> allExcludingCurrent() noexcept; /** * @param handle The native handle to get the TID from. diff --git a/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitorType.c b/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitorType.c index b14aead36d9..f40c7327ada 100644 --- a/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitorType.c +++ b/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), diff --git a/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_CPPException.cpp b/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_CPPException.cpp index 784289a8622..ec93b882a73 100644 --- a/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_CPPException.cpp +++ b/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) diff --git a/Sources/SentryCrash/Recording/Tools/SentryCrashSignalInfo.c b/Sources/SentryCrash/Recording/Tools/SentryCrashSignalInfo.c index 8d27ed32597..c52897917b1 100644 --- a/Sources/SentryCrash/Recording/Tools/SentryCrashSignalInfo.c +++ b/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), diff --git a/Tests/SentryProfilerTests/SentryBacktraceTests.mm b/Tests/SentryProfilerTests/SentryBacktraceTests.mm index 2246c610f5b..107f16c064e 100644 --- a/Tests/SentryProfilerTests/SentryBacktraceTests.mm +++ b/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(bc_a)), 0); + XCTAssertEqual( + pthread_create(&thread2, nullptr, threadEntry, reinterpret_cast(bc_d)), 0); + + const auto cache = std::make_shared(); + 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 diff --git a/Tests/SentryProfilerTests/SentryThreadHandleTests.mm b/Tests/SentryProfilerTests/SentryThreadHandleTests.mm index fe6ac1ccb67..8ba9b828c74 100644 --- a/Tests/SentryProfilerTests/SentryThreadHandleTests.mm +++ b/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; diff --git a/Tests/SentryTests/SentryCrash/CrashReport.swift b/Tests/SentryTests/SentryCrash/CrashReport.swift index 6c99c34eef7..1c252c46500 100644 --- a/Tests/SentryTests/SentryCrash/CrashReport.swift +++ b/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)) } diff --git a/Tests/SentryTests/SentryInterfacesTests.m b/Tests/SentryTests/SentryInterfacesTests.m index 5d01ae94745..eaa4902954b 100644 --- a/Tests/SentryTests/SentryInterfacesTests.m +++ b/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",