fix: edge case crashes with profiled traces (#3365)

Co-authored-by: Philipp Hofmann <[email protected]>

Author: armcknight

CHANGELOG.md

@@ -12,6 +12,7 @@
 - Stop sending empty thread names (#3361)
 - Work around edge case with a thread info kernel call sometimes returning invalid data, leading to a crash (#3364)
 - Thread sanitizer data race warnings in ANR tracker, network tracker and span finish (#3303)
+- Crashes when trace ID is externally modified or profiler fails to initialize (#3365)
 
 ## 8.14.2
 

Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm

@@ -25,7 +25,7 @@
     /* number of in-flight tracers */ NSNumber *> *_gProfilersToTracers;
 
 /** provided for fast access to a profiler given a tracer */
-static NSMutableDictionary</* SentryTracer.tracerId */ NSString *, SentryProfiler *>
+static NSMutableDictionary</* SentryTracer.internalTraceId */ NSString *, SentryProfiler *>
     *_gTracersToProfilers;
 
 namespace {
@@ -54,12 +54,12 @@
 std::mutex _gStateLock;
 
 void
-trackProfilerForTracer(SentryProfiler *profiler, SentryId *traceId)
+trackProfilerForTracer(SentryProfiler *profiler, SentryId *internalTraceId)
 {
     std::lock_guard<std::mutex> l(_gStateLock);
 
     const auto profilerKey = profiler.profilerId.sentryIdString;
-    const auto tracerKey = traceId.sentryIdString;
+    const auto tracerKey = internalTraceId.sentryIdString;
 
     SENTRY_LOG_DEBUG(
         @"Tracking relationship between profiler id %@ and tracer id %@", profilerKey, tracerKey);
@@ -73,7 +73,7 @@
             /* number of in-flight tracers */ NSNumber *>
             dictionary];
         _gTracersToProfilers =
-            [NSMutableDictionary</* SentryTracer.tracerId */ NSString *, SentryProfiler *>
+            [NSMutableDictionary</* SentryTracer.internalTraceId */ NSString *, SentryProfiler *>
                 dictionary];
     }
 
@@ -82,14 +82,14 @@
 }
 
 void
-discardProfilerForTracer(SentryId *traceId)
+discardProfilerForTracer(SentryId *internalTraceId)
 {
     std::lock_guard<std::mutex> l(_gStateLock);
 
     SENTRY_CASSERT(_gTracersToProfilers != nil && _gProfilersToTracers != nil,
         @"Structures should have already been initialized by the time they are being queried");
 
-    const auto tracerKey = traceId.sentryIdString;
+    const auto tracerKey = internalTraceId.sentryIdString;
     const auto profiler = _gTracersToProfilers[tracerKey];
 
     if (profiler == nil) {
@@ -105,14 +105,14 @@
 #    endif // SENTRY_HAS_UIKIT
 }
 
-SentryProfiler *_Nullable profilerForFinishedTracer(SentryId *traceId)
+SentryProfiler *_Nullable profilerForFinishedTracer(SentryId *internalTraceId)
 {
     std::lock_guard<std::mutex> l(_gStateLock);
 
     SENTRY_CASSERT(_gTracersToProfilers != nil && _gProfilersToTracers != nil,
         @"Structures should have already been initialized by the time they are being queried");
 
-    const auto tracerKey = traceId.sentryIdString;
+    const auto tracerKey = internalTraceId.sentryIdString;
     const auto profiler = _gTracersToProfilers[tracerKey];
 
     if (!SENTRY_CASSERT_RETURN(profiler != nil,

Sources/Sentry/SentryProfiler.mm

@@ -336,7 +336,7 @@ - (void)scheduleTimeoutTimer
 
 #    pragma mark - Public
 
-+ (void)startWithTracer:(SentryId *)traceId
++ (BOOL)startWithTracer:(SentryId *)traceId
 {
     std::lock_guard<std::mutex> l(_gProfilerLock);
 
@@ -345,16 +345,17 @@ + (void)startWithTracer:(SentryId *)traceId
         trackProfilerForTracer(_gCurrentProfiler, traceId);
         // record a new metric sample for every concurrent span start
         [_gCurrentProfiler->_metricProfiler recordMetrics];
-        return;
+        return YES;
     }
 
     _gCurrentProfiler = [[SentryProfiler alloc] init];
     if (_gCurrentProfiler == nil) {
         SENTRY_LOG_WARN(@"Profiler was not initialized, will not proceed.");
-        return;
+        return NO;
     }
 
     trackProfilerForTracer(_gCurrentProfiler, traceId);
+    return YES;
 }
 
 + (BOOL)isCurrentlyProfiling
@@ -377,7 +378,7 @@ + (nullable SentryEnvelopeItem *)createProfilingEnvelopeItemForTransaction:
 {
     const auto payload = [self collectProfileBetween:transaction.startSystemTime
                                                  and:transaction.endSystemTime
-                                            forTrace:transaction.trace.traceId
+                                            forTrace:transaction.trace.internalID
                                                onHub:transaction.trace.hub];
     if (payload == nil) {
         return nil;

Sources/Sentry/SentryTracer.m

@@ -163,9 +163,9 @@ - (instancetype)initWithTransactionContext:(SentryTransactionContext *)transacti
 
 #if SENTRY_TARGET_PROFILING_SUPPORTED
     if (_configuration.profilesSamplerDecision.decision == kSentrySampleDecisionYes) {
-        _isProfiling = YES;
         _startSystemTime = SentryDependencyContainer.sharedInstance.dateProvider.systemTime;
-        [SentryProfiler startWithTracer:self.traceId];
+        _internalID = [[SentryId alloc] init];
+        _isProfiling = [SentryProfiler startWithTracer:_internalID];
     }
 #endif // SENTRY_TARGET_PROFILING_SUPPORTED
 
@@ -176,7 +176,7 @@ - (void)dealloc
 {
 #if SENTRY_TARGET_PROFILING_SUPPORTED
     if (self.isProfiling) {
-        discardProfilerForTracer(self.traceId);
+        discardProfilerForTracer(self.internalID);
     }
 #endif // SENTRY_TARGET_PROFILING_SUPPORTED
 }

Sources/Sentry/include/SentryProfiledTracerConcurrency.h

@@ -15,21 +15,21 @@ SENTRY_EXTERN_C_BEGIN
  * Associate the provided profiler and tracer so that profiling data may be retrieved by the tracer
  * when it is ready to transmit its envelope.
  */
-void trackProfilerForTracer(SentryProfiler *profiler, SentryId *traceId);
+void trackProfilerForTracer(SentryProfiler *profiler, SentryId *internalTraceId);
 
 /**
  * For transactions that will be discarded, clean up the bookkeeping state associated with them to
  * reclaim the memory they're using.
  */
-void discardProfilerForTracer(SentryId *traceId);
+void discardProfilerForTracer(SentryId *internalTraceId);
 
 /**
  * Return the profiler instance associated with the tracer. If it was the last tracer for the
  * associated profiler, stop that profiler. Copy any recorded @c SentryScreenFrames data into the
  * profiler instance, and if this is the last profiler being tracked, reset the
  * @c SentryFramesTracker data.
  */
-SentryProfiler *_Nullable profilerForFinishedTracer(SentryId *traceId);
+SentryProfiler *_Nullable profilerForFinishedTracer(SentryId *internalTraceId);
 
 #    if defined(TEST) || defined(TESTCI)
 void resetConcurrencyTracking(void);

Sources/Sentry/include/SentryProfiler.h

@@ -50,7 +50,7 @@ SENTRY_EXTERN_C_END
 /**
  * Start a profiler, if one isn't already running.
  */
-+ (void)startWithTracer:(SentryId *)traceId;
++ (BOOL)startWithTracer:(SentryId *)traceId;
 
 /**
  * Stop the profiler if it is running.

Sources/Sentry/include/SentryTracer+Private.h

@@ -5,4 +5,11 @@ SentryTracer ()
 
 @property (nonatomic, strong) SentryHub *hub;
 
+/**
+ * We need an immutable identifier to e.g. track concurrent tracers against a static profiler where
+ * we can use the same ID as a key in the concurrent bookkeeping. @c SentryTracer.traceId can be
+ * changed by consumers so is unfit for this purpose.
+ */
+@property (nonatomic, strong, readonly) SentryId *internalID;
+
 @end

Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift

@@ -263,6 +263,15 @@ class SentryProfilerSwiftTests: XCTestCase {
         try self.assertMetricsPayload(expectedUsageReadings: fixture.mockUsageReadingsPerBatch + 2) // including one sample at the start and the end
     }
 
+    func testTransactionWithMutatedTracerID() throws {
+        let span = try fixture.newTransaction()
+        addMockSamples()
+        self.fixture.currentDateProvider.advanceBy(nanoseconds: 1.toNanoSeconds())
+        span.traceId = SentryId()
+        span.finish()
+        try self.assertValidProfileData()
+    }
+
     func testConcurrentProfilingTransactions() throws {
         let numberOfTransactions = 10
         var spans = [Span]()