fix: Remove dispatch queue metadata collection to fix crash (#3522)

* Remove dispatch queue metadata collection * Hardcode "main" as the name of the main thread * Format code * Update CHANGELOG.md * use thread wrapper main thread dispatch --------- Co-authored-by: Sentry Github Bot <[email protected]> Co-authored-by: Andrew McKnight <[email protected]>
getsentry · Jan 10, 2024 · c00cc0f · c00cc0f
1 parent fb3cb76
commit c00cc0f
Show file tree

Hide file tree

Showing 16 changed files with 58 additions and 222 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Fixes
+
+- Remove dispatch queue metadata collection to fix crash (#3522)
+
 ### Features
 
 - Send debug meta for app start transactions (#3543)

diff --git a/SentryTestUtils/SentryProfilerMocks.h b/SentryTestUtils/SentryProfilerMocks.h
@@ -10,8 +10,7 @@ using namespace sentry::profiling;
 NS_ASSUME_NONNULL_BEGIN
 
 Backtrace mockBacktrace(thread::TIDType threadID, const int threadPriority,
-    const char *_Nullable threadName, std::uint64_t queueAddress, std::string queueLabel,
-    std::vector<std::uintptr_t> addresses);
+    const char *_Nullable threadName, std::vector<std::uintptr_t> addresses);
 
 NS_ASSUME_NONNULL_END
 

diff --git a/SentryTestUtils/SentryProfilerMocks.mm b/SentryTestUtils/SentryProfilerMocks.mm
@@ -4,7 +4,7 @@
 
 Backtrace
 mockBacktrace(thread::TIDType threadID, const int threadPriority, const char *threadName,
-    std::uint64_t queueAddress, std::string queueLabel, std::vector<std::uintptr_t> addresses)
+    std::vector<std::uintptr_t> addresses)
 {
     ThreadMetadata threadMetadata;
     if (threadName != nullptr) {
@@ -13,13 +13,8 @@
     threadMetadata.threadID = threadID;
     threadMetadata.priority = threadPriority;
 
-    QueueMetadata queueMetadata;
-    queueMetadata.address = queueAddress;
-    queueMetadata.label = std::make_shared<std::string>(queueLabel);
-
     Backtrace backtrace;
     backtrace.threadMetadata = threadMetadata;
-    backtrace.queueMetadata = queueMetadata;
     backtrace.addresses = std::vector<std::uintptr_t>(addresses);
 
     return backtrace;

diff --git a/SentryTestUtils/SentryProfilerMocksSwiftCompatible.h b/SentryTestUtils/SentryProfilerMocksSwiftCompatible.h
@@ -17,8 +17,6 @@ NS_ASSUME_NONNULL_BEGIN
                           threadID:(uint64_t)threadID
                     threadPriority:(const int)threadPriority
                         threadName:(nullable NSString *)threadName
-                      queueAddress:(uint64_t)queueAddress
-                        queueLabel:(NSString *)queueLabel
                          addresses:(NSArray<NSNumber *> *)addresses;
 
 @end

diff --git a/SentryTestUtils/SentryProfilerMocksSwiftCompatible.mm b/SentryTestUtils/SentryProfilerMocksSwiftCompatible.mm
@@ -16,8 +16,6 @@ + (void)appendMockBacktraceToState:(SentryProfilerState *)state
                           threadID:(uint64_t)threadID
                     threadPriority:(const int)threadPriority
                         threadName:(nullable NSString *)threadName
-                      queueAddress:(uint64_t)queueAddress
-                        queueLabel:(NSString *)queueLabel
                          addresses:(NSArray<NSNumber *> *)addresses
 {
     auto backtraceAddresses = std::vector<std::uintptr_t>();
@@ -27,8 +25,7 @@ + (void)appendMockBacktraceToState:(SentryProfilerState *)state
     }
 
     auto backtrace = mockBacktrace(threadID, threadPriority,
-        [threadName cStringUsingEncoding:NSUTF8StringEncoding], queueAddress,
-        [queueLabel cStringUsingEncoding:NSUTF8StringEncoding], backtraceAddresses);
+        [threadName cStringUsingEncoding:NSUTF8StringEncoding], backtraceAddresses);
     backtrace.absoluteTimestamp = SentryDependencyContainer.sharedInstance.dateProvider.systemTime;
     [state appendBacktrace:backtrace];
 }

diff --git a/Sources/Sentry/Profiling/SentryProfilerState.mm b/Sources/Sentry/Profiling/SentryProfilerState.mm
@@ -4,6 +4,9 @@
 #    import "SentryFormatter.h"
 #    import "SentryProfileTimeseries.h"
 #    import "SentrySample.h"
+#    import "SentryThreadWrapper.h"
+#    import <mach/mach_types.h>
+#    import <mach/port.h>
 #    import <mutex>
 
 #    if defined(DEBUG)
@@ -42,7 +45,6 @@ - (instancetype)init
         _stacks = [NSMutableArray<NSArray<NSNumber *> *> array];
         _frames = [NSMutableArray<NSDictionary<NSString *, id> *> array];
         _threadMetadata = [NSMutableDictionary<NSString *, NSMutableDictionary *> dictionary];
-        _queueMetadata = [NSMutableDictionary<NSString *, NSDictionary *> dictionary];
         _frameIndexLookup = [NSMutableDictionary<NSString *, NSNumber *> dictionary];
         _stackIndexLookup = [NSMutableDictionary<NSString *, NSNumber *> dictionary];
     }
@@ -54,12 +56,15 @@ - (instancetype)init
 @implementation SentryProfilerState {
     SentryProfilerMutableState *_mutableState;
     std::mutex _lock;
+    thread_t _mainThreadID;
 }
 
 - (instancetype)init
 {
     if (self = [super init]) {
         _mutableState = [[SentryProfilerMutableState alloc] init];
+        _mainThreadID = 0;
+        [SentryThreadWrapper onMainThread:^{ [self cacheMainThreadID]; }];
     }
     return self;
 }
@@ -71,36 +76,38 @@ - (void)mutate:(void (^)(SentryProfilerMutableState *))block
     block(_mutableState);
 }
 
+- (void)cacheMainThreadID
+{
+    std::lock_guard<std::mutex> l(_lock);
+    NSAssert([NSThread isMainThread], @"Must be called on main thread");
+    const auto currentThread = pthread_mach_thread_np(pthread_self());
+    if (MACH_PORT_VALID(currentThread)) {
+        _mainThreadID = currentThread;
+    }
+}
+
 - (void)appendBacktrace:(const Backtrace &)backtrace
 {
     [self mutate:^(SentryProfilerMutableState *state) {
         const auto threadID = sentry_stringForUInt64(backtrace.threadMetadata.threadID);
 
-        NSString *queueAddress = nil;
-        if (backtrace.queueMetadata.address != 0) {
-            queueAddress = sentry_formatHexAddressUInt64(backtrace.queueMetadata.address);
-        }
         NSMutableDictionary<NSString *, id> *metadata = state.threadMetadata[threadID];
         if (metadata == nil) {
             metadata = [NSMutableDictionary<NSString *, id> dictionary];
             state.threadMetadata[threadID] = metadata;
         }
-        if (!backtrace.threadMetadata.name.empty() && metadata[@"name"] == nil) {
-            metadata[@"name"] =
-                [NSString stringWithUTF8String:backtrace.threadMetadata.name.c_str()];
+        if (metadata[@"name"] == nil) {
+            if (!backtrace.threadMetadata.name.empty()) {
+                metadata[@"name"] =
+                    [NSString stringWithUTF8String:backtrace.threadMetadata.name.c_str()];
+            } else if (self->_mainThreadID != 0
+                && backtrace.threadMetadata.threadID == self->_mainThreadID) {
+                metadata[@"name"] = @"main";
+            }
         }
         if (backtrace.threadMetadata.priority != -1 && metadata[@"priority"] == nil) {
             metadata[@"priority"] = @(backtrace.threadMetadata.priority);
         }
-        if (queueAddress != nil && state.queueMetadata[queueAddress] == nil
-            && backtrace.queueMetadata.label != nullptr) {
-            NSString *const labelNSStr =
-                [NSString stringWithUTF8String:backtrace.queueMetadata.label->c_str()];
-            // -[NSString stringWithUTF8String:] can return `nil` for malformed string data
-            if (labelNSStr != nil) {
-                state.queueMetadata[queueAddress] = @ { @"label" : labelNSStr };
-            }
-        }
 #    if defined(DEBUG)
         const auto symbols
             = backtrace_symbols(reinterpret_cast<void *const *>(backtrace.addresses.data()),
@@ -136,9 +143,6 @@ - (void)appendBacktrace:(const Backtrace &)backtrace
         const auto sample = [[SentrySample alloc] init];
         sample.absoluteTimestamp = backtrace.absoluteTimestamp;
         sample.threadID = backtrace.threadMetadata.threadID;
-        if (queueAddress != nil) {
-            sample.queueAddress = queueAddress;
-        }
 
         const auto stackKey = [stack componentsJoinedByString:@"|"];
         const auto stackIndex = state.stackIndexLookup[stackKey];
@@ -162,8 +166,6 @@ - (void)appendBacktrace:(const Backtrace &)backtrace
     NSMutableArray<SentrySample *> *const samples = [_mutableState.samples copy];
     NSMutableArray<NSArray<NSNumber *> *> *const stacks = [_mutableState.stacks copy];
     NSMutableArray<NSDictionary<NSString *, id> *> *const frames = [_mutableState.frames copy];
-    NSMutableDictionary<NSString *, NSDictionary *> *const queueMetadata =
-        [_mutableState.queueMetadata copy];
 
     // thread metadata contains a mutable substructure, so it's not enough to perform a copy of
     // the top-level dictionary, we need to go deeper to copy the mutable subdictionaries
@@ -177,7 +179,6 @@ - (void)appendBacktrace:(const Backtrace &)backtrace
             @"stacks" : stacks,
             @"frames" : frames,
             @"thread_metadata" : threadMetadata,
-            @"queue_metadata" : queueMetadata
         }
     };
 }

diff --git a/Sources/Sentry/SentryBacktrace.cpp b/Sources/Sentry/SentryBacktrace.cpp
@@ -159,38 +159,7 @@ namespace profiling {
             const auto depth = backtrace(*thread, *pair.second, addresses, stackBounds,
                 &reachedEndOfStack, kMaxBacktraceDepth, 0);
 
-            // Retrieving queue metadata *must* be done after suspending the thread,
-            // because otherwise the queue could be deallocated in the middle of us
-            // trying to read from it. This doesn't use any of the pthread APIs, only
-            // thread_info, so the above comment about the deadlock does not apply here.
-            const auto queueAddress = thread->dispatchQueueAddress();
-            if (queueAddress != 0) {
-                // This operation is read-only and does not result in any heap allocations.
-                auto cachedMetadata = cache->metadataForQueue(queueAddress);
-
-                // Copy the queue label onto the stack to avoid a heap allocation.
-                char newQueueLabel[256];
-                *newQueueLabel = '\0';
-                if (cachedMetadata.address == 0) {
-                    // There's no cached metadata, so we should try to read it.
-                    const auto queue = reinterpret_cast<dispatch_queue_t *>(queueAddress);
-                    const auto queueLabel = dispatch_queue_get_label(*queue);
-                    strlcpy(newQueueLabel, queueLabel, sizeof(newQueueLabel));
-                }
-
-                thread->resume();
-
-                if (cachedMetadata.address == 0) {
-                    cachedMetadata.address = queueAddress;
-                    // These cause heap allocations but it's safe now since the thread has
-                    // been resumed above.
-                    cachedMetadata.label = std::make_shared<std::string>(newQueueLabel);
-                    cache->setQueueMetadata(cachedMetadata);
-                }
-                bt.queueMetadata = std::move(cachedMetadata);
-            } else {
-                thread->resume();
-            }
+            thread->resume();
 
             // ############################################
             // END DEADLOCK WARNING

diff --git a/Sources/Sentry/SentryThreadHandle.cpp b/Sources/Sentry/SentryThreadHandle.cpp
@@ -10,13 +10,6 @@
 #    include <mach/mach.h>
 #    include <pthread.h>
 
-/**
- * SentryCrashMemory.h uses the restrict keyword, which is valid in C99 but
- * invalid in C++; we can use __restrict as an alternative.
- * */
-#    define restrict __restrict
-#    include "SentryCrashMemory.h"
-
 namespace sentry {
 namespace profiling {
 
@@ -123,27 +116,6 @@ namespace profiling {
         return {};
     }
 
-    std::uint64_t
-    ThreadHandle::dispatchQueueAddress() const noexcept
-    {
-        if (handle_ == THREAD_NULL) {
-            return {};
-        }
-        integer_t infoBuffer[THREAD_IDENTIFIER_INFO_COUNT] = { 0 };
-        thread_info_t info = infoBuffer;
-        mach_msg_type_number_t count = THREAD_IDENTIFIER_INFO_COUNT;
-        const auto idInfo = reinterpret_cast<thread_identifier_info_t>(info);
-        if (thread_info(handle_, THREAD_IDENTIFIER_INFO, info, &count) == KERN_SUCCESS
-            && sentrycrashmem_isMemoryReadable(idInfo, sizeof(*idInfo))) {
-            const auto queuePtr = reinterpret_cast<dispatch_queue_t *>(idInfo->dispatch_qaddr);
-            if (queuePtr != nullptr && sentrycrashmem_isMemoryReadable(queuePtr, sizeof(*queuePtr))
-                && idInfo->thread_handle != 0 && *queuePtr != nullptr) {
-                return idInfo->dispatch_qaddr;
-            }
-        }
-        return 0;
-    }
-
     int
     ThreadHandle::priority() const noexcept
     {

diff --git a/Sources/Sentry/SentryThreadMetadataCache.cpp b/Sources/Sentry/SentryThreadMetadataCache.cpp
@@ -61,28 +61,6 @@ namespace profiling {
             return (*it).metadata;
         }
     }
-
-    QueueMetadata
-    ThreadMetadataCache::metadataForQueue(std::uint64_t address) const
-    {
-        if (address == 0) {
-            return {};
-        }
-        const auto it = std::find_if(queueMetadataCache_.cbegin(), queueMetadataCache_.cend(),
-            [address](const QueueMetadata &metadata) { return metadata.address == address; });
-        if (it == queueMetadataCache_.cend()) {
-            return { 0, {} };
-        } else {
-            return (*it);
-        }
-    }
-
-    void
-    ThreadMetadataCache::setQueueMetadata(QueueMetadata metadata)
-    {
-        queueMetadataCache_.push_back(std::move(metadata));
-    }
-
 } // namespace profiling
 } // namespace sentry
 

diff --git a/Sources/Sentry/include/SentryBacktrace.hpp b/Sources/Sentry/include/SentryBacktrace.hpp
@@ -21,7 +21,6 @@ namespace profiling {
 
     struct Backtrace {
         ThreadMetadata threadMetadata;
-        QueueMetadata queueMetadata;
         std::uint64_t absoluteTimestamp;
         std::vector<std::uintptr_t> addresses;
     };

diff --git a/Sources/Sentry/include/SentryProfilerState.h b/Sources/Sentry/include/SentryProfilerState.h
@@ -31,8 +31,6 @@ NSString *parseBacktraceSymbolsFunctionName(const char *symbol);
 @property (nonatomic, strong, readonly) NSMutableArray<NSDictionary<NSString *, id> *> *frames;
 @property (nonatomic, strong, readonly)
     NSMutableDictionary<NSString *, NSMutableDictionary *> *threadMetadata;
-@property (nonatomic, strong, readonly)
-    NSMutableDictionary<NSString *, NSDictionary *> *queueMetadata;
 
 /*
  * Maintain an index of unique frames to avoid duplicating large amounts of data. Every

diff --git a/Sources/Sentry/include/SentryThreadHandle.hpp b/Sources/Sentry/include/SentryThreadHandle.hpp
@@ -97,17 +97,6 @@ namespace profiling {
          */
         std::string name() const noexcept;
 
-        /**
-         * @return The address of the dispatch queue currently associated with
-         * the thread, or 0 if there is no valid queue. This function checks if
-         * the memory at the returned address is readable, but there is no guarantee
-         * that the queue will continue to stay valid by the time you deference it,
-         * as queue deallocation can happen concurrently.
-         *
-         * @warning This function is not async-signal safe!
-         */
-        std::uint64_t dispatchQueueAddress() const noexcept;
-
         /**
          * @return The priority of the specified thread, or -1 if the thread priority
          * could not be successfully determined.

diff --git a/Sources/Sentry/include/SentryThreadMetadataCache.hpp b/Sources/Sentry/include/SentryThreadMetadataCache.hpp
@@ -18,13 +18,6 @@ namespace profiling {
         int priority;
     };
 
-    struct QueueMetadata {
-        std::uint64_t address;
-        // std::string always heap allocates a string buffer, since this data structure
-        // might be created while its unsafe to allocate, we wrap it in a pointer.
-        std::shared_ptr<std::string> label;
-    };
-
     /**
      * Caches thread and queue metadata (name, priority, etc.) for reuse while profiling,
      * since querying that metadata every time can be expensive.
@@ -42,21 +35,6 @@ namespace profiling {
          */
         ThreadMetadata metadataForThread(const ThreadHandle &thread);
 
-        /**
-         * Returns the metadata for the queue at the specified address.
-         * @param address The address of the queue.
-         * @return @c QueueMetadata for the queue at the specified address if a cached
-         * entry exists, or an empty @c QueueMetadata with the address set to 0 if it
-         * does not exist.
-         */
-        QueueMetadata metadataForQueue(std::uint64_t address) const;
-
-        /**
-         * Stores metadata for the queue at the address specified in `metadata.address`
-         * @param metadata The metadata to associate with the queue.
-         */
-        void setQueueMetadata(QueueMetadata metadata);
-
         ThreadMetadataCache() = default;
         ThreadMetadataCache(const ThreadMetadataCache &) = delete;
         ThreadMetadataCache &operator=(const ThreadMetadataCache &) = delete;
@@ -67,7 +45,6 @@ namespace profiling {
             ThreadMetadata metadata;
         };
         std::vector<const ThreadHandleMetadataPair> threadMetadataCache_;
-        std::vector<const QueueMetadata> queueMetadataCache_;
     };
 
 } // namespace profiling