fix: resolve EXC_BAD_ACCESS in SentryTracer/SentryNetworkTracker span lifecycle

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Fixes
 
+- Fix EXC_BAD_ACCESS in SentryTracer/SentryNetworkTracker span lifecycle caused by concurrent access to volatile properties (#8058)
 - Fix dropped `platform` item header in profile-chunk envelopes (#8269)
 - Fix crash report ID generation so reports created at certain timestamps are not ignored (#8216)
 - Fix C++ exception capture on newer OS versions by page-aligning `mprotect` calls in the `__cxa_throw` swapper (#8221)

Sources/Sentry/SentryNetworkTracker.m

@@ -123,7 +123,10 @@ - (void)urlSessionTaskResume:(NSURLSessionTask *)sessionTask
         return;
     }
 
-    NSURL *url = [[sessionTask currentRequest] URL];
+    // Snapshot currentRequest once — the property is volatile and can return a freed
+    // object if the task completes on another thread between repeated accesses.
+    NSURLRequest *currentRequest = sessionTask.currentRequest;
+    NSURL *url = currentRequest.URL;
 
     if (url == nil) {
         return;
@@ -167,14 +170,13 @@ - (void)urlSessionTaskResume:(NSURLSessionTask *)sessionTask
         id<SentrySpan> _Nullable currentSpan = [SentrySDKInternal.currentHub.scope span];
         if (currentSpan != nil) {
             span = currentSpan;
-            netSpan = [span startChildWithOperation:SentrySpanOperationNetworkRequestOperation
-                                        description:[NSString stringWithFormat:@"%@ %@",
-                                                        sessionTask.currentRequest.HTTPMethod,
-                                                        safeUrl.sanitizedUrl]];
+            netSpan =
+                [span startChildWithOperation:SentrySpanOperationNetworkRequestOperation
+                                  description:[NSString stringWithFormat:@"%@ %@",
+                                                  currentRequest.HTTPMethod, safeUrl.sanitizedUrl]];
             netSpan.origin = SentryTraceOriginAutoHttpNSURLSession;
 
-            [netSpan setDataValue:sessionTask.currentRequest.HTTPMethod
-                           forKey:@"http.request.method"];
+            [netSpan setDataValue:currentRequest.HTTPMethod forKey:@"http.request.method"];
             [netSpan setDataValue:safeUrl.sanitizedUrl forKey:@"url"];
             [netSpan setDataValue:@"fetch" forKey:@"type"];
 

Sources/Sentry/SentryTimeToDisplayTracker.m

@@ -89,32 +89,35 @@ - (BOOL)startForTracer:(SentryTracer *)tracer
     [tracer setFinishCallback:^(SentryTracer *_tracer) {
         [SentryDependencyContainer.sharedInstance.framesTracker removeListener:self];
 
-        // The tracer finishes when the screen is fully displayed. Therefore, we must also finish
-        // the TTID span.
-        if (self.initialDisplaySpan.isFinished == NO) {
-            [self.initialDisplaySpan finish];
+        // Strongify the weak span references so they can't become dangling
+        // mid-callback if the tracer's children are released concurrently.
+        id<SentrySpan> initialSpan = self.initialDisplaySpan;
+        id<SentrySpan> fullSpan = self.fullDisplaySpan;
+
+        if (initialSpan != nil && initialSpan.isFinished == NO) {
+            [initialSpan finish];
         }
 
-        // If the start time of the tracer changes, which is the case for app start transactions, we
-        // also need to adapt the start time of our spans.
-        self.initialDisplaySpan.startTimestamp = _tracer.startTimestamp;
-        [self addTimeToDisplayMeasurement:self.initialDisplaySpan name:@"time_to_initial_display"];
+        if (initialSpan != nil) {
+            initialSpan.startTimestamp = _tracer.startTimestamp;
+            [self addTimeToDisplayMeasurement:initialSpan name:@"time_to_initial_display"];
+        }
 
-        if (self.fullDisplaySpan == nil) {
+        if (fullSpan == nil) {
             return;
         }
 
-        self.fullDisplaySpan.startTimestamp = _tracer.startTimestamp;
-        [self addTimeToDisplayMeasurement:self.fullDisplaySpan name:@"time_to_full_display"];
+        fullSpan.startTimestamp = _tracer.startTimestamp;
+        [self addTimeToDisplayMeasurement:fullSpan name:@"time_to_full_display"];
 
-        if (self.fullDisplaySpan.status != kSentrySpanStatusDeadlineExceeded) {
+        if (fullSpan.status != kSentrySpanStatusDeadlineExceeded) {
             return;
         }
 
-        self.fullDisplaySpan.timestamp = self.initialDisplaySpan.timestamp;
-        self.fullDisplaySpan.spanDescription = [NSString
-            stringWithFormat:@"%@ - Deadline Exceeded", self.fullDisplaySpan.spanDescription];
-        [self addTimeToDisplayMeasurement:self.fullDisplaySpan name:@"time_to_full_display"];
+        fullSpan.timestamp = initialSpan.timestamp;
+        fullSpan.spanDescription =
+            [NSString stringWithFormat:@"%@ - Deadline Exceeded", fullSpan.spanDescription];
+        [self addTimeToDisplayMeasurement:fullSpan name:@"time_to_full_display"];
     }];
 
     return YES;

Sources/Sentry/SentryTracePropagation.m

@@ -15,40 +15,40 @@ + (void)addBaggageHeader:(SentryBaggage *)baggage
     tracePropagationTargets:(NSArray *_Nullable)tracePropagationTargets
                   toRequest:(NSURLSessionTask *)sessionTask
 {
-    if (![SentryTracePropagation sessionTaskRequiresPropagation:sessionTask
-                                        tracePropagationTargets:tracePropagationTargets]) {
-        SENTRY_LOG_DEBUG(@"Not adding trace_id and baggage headers for %@",
-            sessionTask.currentRequest.URL.absoluteString);
+    // Snapshot currentRequest once — the property is volatile and can become a zombie
+    // between repeated accesses if the task completes on another thread.
+    NSURLRequest *request = sessionTask.currentRequest;
+    if (request == nil) {
+        return;
+    }
+
+    if (![SentryTracePropagation isTargetMatch:SENTRY_UNWRAP_NULLABLE(NSURL, request.URL)
+                                   withTargets:tracePropagationTargets ?: @[]]) {
+        SENTRY_LOG_DEBUG(
+            @"Not adding trace_id and baggage headers for %@", request.URL.absoluteString);
         return;
     }
     NSString *baggageHeader = @"";
 
     if (baggage != nil) {
-        NSString *_Nullable rawHeader = SENTRY_UNWRAP_NULLABLE(
-            NSString, sessionTask.currentRequest.allHTTPHeaderFields[SENTRY_BAGGAGE_HEADER]);
+        NSString *_Nullable rawHeader
+            = SENTRY_UNWRAP_NULLABLE(NSString, request.allHTTPHeaderFields[SENTRY_BAGGAGE_HEADER]);
         NSDictionary *originalBaggage = [SentryBaggageSerialization decode:rawHeader ?: @""];
         if (originalBaggage[@"sentry-trace_id"] == nil) {
             baggageHeader = [baggage toHTTPHeaderWithOriginalBaggage:originalBaggage];
         }
     }
 
-    // First we check if the current request is mutable, so we could easily add a new
-    // header. Otherwise we try to change the current request for a new one with the extra
-    // header.
-    if ([sessionTask.currentRequest isKindOfClass:[NSMutableURLRequest class]]) {
-        NSMutableURLRequest *currentRequest = (NSMutableURLRequest *)sessionTask.currentRequest;
-        [SentryTracePropagation addHeaderFieldsToRequest:currentRequest
+    if ([request isKindOfClass:[NSMutableURLRequest class]]) {
+        NSMutableURLRequest *mutableRequest = (NSMutableURLRequest *)request;
+        [SentryTracePropagation addHeaderFieldsToRequest:mutableRequest
                                              traceHeader:traceHeader
                                            baggageHeader:baggageHeader
                                     propagateTraceparent:propagateTraceparent];
     } else {
-        // Even though NSURLSessionTask doesn't have 'setCurrentRequest', some subclasses
-        // do. For those subclasses we replace the currentRequest with a mutable one with
-        // the additional trace header. Since NSURLSessionTask is a public class and can be
-        // overridden, we believe this is not considered a private api.
         SEL setCurrentRequestSelector = NSSelectorFromString(@"setCurrentRequest:");
         if ([sessionTask respondsToSelector:setCurrentRequestSelector]) {
-            NSMutableURLRequest *newRequest = [sessionTask.currentRequest mutableCopy];
+            NSMutableURLRequest *newRequest = [request mutableCopy];
             [SentryTracePropagation addHeaderFieldsToRequest:newRequest
                                                  traceHeader:traceHeader
                                                baggageHeader:baggageHeader

Sources/Sentry/SentryTracer.m

@@ -500,9 +500,9 @@ - (void)canBeFinished
         return;
     }
 
-    BOOL hasUnfinishedChildSpansToWaitFor = [self hasUnfinishedChildSpansToWaitFor];
-
     @synchronized(self) {
+        BOOL hasUnfinishedChildSpansToWaitFor = [self hasUnfinishedChildSpansToWaitFor];
+
         if (!self.wasFinishCalled && !hasUnfinishedChildSpansToWaitFor && [self hasIdleTimeout]) {
             SENTRY_LOG_DEBUG(
                 @"Span with id %@ isn't waiting on children and needs idle timeout dispatched.",

Tests/SentryTests/Integrations/Performance/Network/SentryNetworkTrackerTests.swift

@@ -1486,4 +1486,46 @@ class SentryNetworkTrackerTests: XCTestCase {
         request.httpMethod = method
         return URLSessionStreamTaskMock(request: request)
     }
+
+    // MARK: - Concurrent resume + setState race (issue #8012)
+
+    func testResumeConcurrentWithSetState_DoesNotCrash() {
+        let sut = fixture.getSut()
+
+        let queue = DispatchQueue(label: "resume-setState-race", qos: .userInteractive, attributes: [.concurrent, .initiallyInactive])
+        let iterations = 500
+        let expectation = XCTestExpectation(description: "Concurrent resume and setState")
+        expectation.expectedFulfillmentCount = iterations * 2
+        expectation.assertForOverFulfill = true
+
+        for _ in 0..<iterations {
+            let task = createDataTask()
+            _ = startTransaction()
+
+            queue.async {
+                sut.urlSessionTaskResume(task)
+                expectation.fulfill()
+            }
+            queue.async {
+                task.state = .completed
+                sut.urlSessionTask(task, setState: .completed)
+                expectation.fulfill()
+            }
+        }
+
+        queue.activate()
+        wait(for: [expectation], timeout: 10)
+    }
+
+    func testResumeAfterTaskCompleted_DoesNotCrash() {
+        let sut = fixture.getSut()
+        let transaction = startTransaction()
+        let task = createDataTask()
+
+        task.state = .completed
+        sut.urlSessionTaskResume(task)
+
+        let spans = Dynamic(transaction).children as [Span]?
+        XCTAssertEqual(spans?.count ?? 0, 0)
+    }
 }

Tests/SentryTests/Integrations/Performance/Network/SentryTracePropagationTests.swift

@@ -147,6 +147,29 @@ final class SentryTracePropagationTests: XCTestCase {
         XCTAssertTrue(SentryTracePropagation.isTargetMatch(localhostURL, withTargets: targetsWithInvalidType))
     }
 
+    // MARK: - currentRequest snapshot safety (issue #8012)
+
+    func testAddBaggageHeader_TaskWithNoCurrentRequest_DoesNotCrash() throws {
+        let emptyBaggage = Baggage()
+        let traceHeader = TraceHeader(
+            trace: SentryId(),
+            spanId: SpanId(),
+            sampled: .yes
+        )
+
+        let task = URLSessionDataTaskMock()
+
+        SentryTracePropagation.addBaggageHeader(
+            emptyBaggage,
+            traceHeader: traceHeader,
+            propagateTraceparent: true,
+            tracePropagationTargets: [try XCTUnwrap(NSRegularExpression(pattern: ".*"))],
+            toRequest: task
+        )
+
+        XCTAssertNil(task.currentRequest?.value(forHTTPHeaderField: "sentry-trace"))
+    }
+
     private func createSessionTask(method: String = "GET") throws -> URLSessionDownloadTaskMock {
         let url = try XCTUnwrap(URL(string: "https://www.domain.com/api?query=value&query2=value2#fragment"))
         var request = URLRequest(url: url)

Tests/SentryTests/Integrations/Performance/UIViewController/SentryTimeToDisplayTrackerTest.swift

@@ -591,6 +591,61 @@ class SentryTimeToDisplayTrackerTest: XCTestCase {
         XCTAssertEqual(tracer.measurements[name]?.value, NSNumber(value: duration), file: file, line: line)
         XCTAssertEqual(tracer.measurements[name]?.unit?.unit, "millisecond", file: file, line: line)
     }
+
+    // MARK: - Weak span safety in finishCallback (issue #8012)
+
+    func testFinishCallback_ConcurrentTracerFinish_DoesNotCrash() throws {
+        for _ in 0..<10 {
+            let sut = fixture.getSut(name: "UIViewController", waitForFullDisplay: true)
+            let tracer = try fixture.getTracer()
+
+            fixture.dateProvider.setDate(date: Date(timeIntervalSince1970: 7))
+            XCTAssertTrue(sut.start(for: tracer))
+
+            fixture.dateProvider.setDate(date: Date(timeIntervalSince1970: 8))
+            sut.reportInitialDisplay()
+            fixture.displayLinkWrapper.normalFrame()
+
+            fixture.dateProvider.setDate(date: Date(timeIntervalSince1970: 9))
+            sut.reportFullyDisplayed()
+            fixture.displayLinkWrapper.normalFrame()
+
+            let queue = DispatchQueue(label: "ttd-finish-race", attributes: [.concurrent, .initiallyInactive])
+            let expectation = expectation(description: "concurrent finish")
+            expectation.expectedFulfillmentCount = 2
+            expectation.assertForOverFulfill = true
+
+            queue.async {
+                let child = tracer.startChild(operation: "test.op")
+                child.finish()
+                expectation.fulfill()
+            }
+            queue.async {
+                tracer.finish()
+                expectation.fulfill()
+            }
+
+            queue.activate()
+            wait(for: [expectation], timeout: 5.0)
+
+            XCTAssertTrue(tracer.isFinished)
+        }
+    }
+
+    func testFinishCallback_SpansAlreadyNil_DoesNotCrash() throws {
+        let sut = fixture.getSut(name: "UIViewController", waitForFullDisplay: false)
+        let tracer = try fixture.getTracer()
+
+        fixture.dateProvider.setDate(date: Date(timeIntervalSince1970: 7))
+        XCTAssertTrue(sut.start(for: tracer))
+
+        fixture.dateProvider.setDate(date: Date(timeIntervalSince1970: 8))
+        sut.reportInitialDisplay()
+        fixture.displayLinkWrapper.normalFrame()
+
+        tracer.finish()
+        XCTAssertTrue(tracer.isFinished)
+    }
 }
 
 #endif // os(iOS) || os(tvOS)

Tests/SentryTests/Transaction/SentryTracerTests.swift

@@ -1829,6 +1829,65 @@ class SentryTracerTests: XCTestCase {
         XCTAssertEqual(expected, appContext?["start_type"])
     }
 
+    // MARK: - canBeFinished TOCTOU race (issue #8012)
+
+    func testCanBeFinished_ConcurrentChildFinish_DoesNotCrash() throws {
+        for _ in 0..<10 {
+            let sut = fixture.getSut()
+
+            let queue = DispatchQueue(label: "canBeFinished-race", attributes: [.concurrent, .initiallyInactive])
+            let expectation = expectation(description: "concurrent finish")
+            expectation.expectedFulfillmentCount = 101
+            expectation.assertForOverFulfill = true
+
+            for _ in 0..<100 {
+                let child = sut.startChild(operation: fixture.transactionOperation)
+                queue.async {
+                    child.finish()
+                    expectation.fulfill()
+                }
+            }
+
+            queue.async {
+                sut.finish()
+                expectation.fulfill()
+            }
+
+            queue.activate()
+            wait(for: [expectation], timeout: 10.0)
+
+            XCTAssertTrue(sut.isFinished)
+            XCTAssertLessThanOrEqual(fixture.hub.capturedEventsWithScopes.count, 1)
+
+            fixture.hub.capturedEventsWithScopes.removeAll()
+        }
+    }
+
+    func testCanBeFinished_AllChildrenFinishConcurrently_TransactionCapturedExactlyOnce() throws {
+        let sut = fixture.getSut()
+        let children = (0..<50).map { _ in sut.startChild(operation: fixture.transactionOperation) }
+
+        let queue = DispatchQueue(label: "finish-all-children", attributes: [.concurrent, .initiallyInactive])
+        let expectation = expectation(description: "all children finished")
+        expectation.expectedFulfillmentCount = 51
+        expectation.assertForOverFulfill = true
+
+        sut.finish()
+        expectation.fulfill()
+
+        for child in children {
+            queue.async {
+                child.finish()
+                expectation.fulfill()
+            }
+        }
+
+        queue.activate()
+        wait(for: [expectation], timeout: 10.0)
+
+        assertOneTransactionCaptured(sut)
+    }
+
 }
 
 // swiftlint:enable file_length