Skip to content

Commit b404da1

Browse files
itaybreitaybre
authored
fix: resolve EXC_BAD_ACCESS in SentryTracer/SentryNetworkTracker span lifecycle (#8058)
* fix: resolve EXC_BAD_ACCESS in SentryTracer/SentryNetworkTracker span lifecycle Fix two crash paths in the SentryTracer → SentryNetworkTracker call chain that cause use-after-free crashes (SDK-CRASHES-COCOA-5, ~140K events/90d): 1. TOCTOU race in canBeFinished: hasUnfinishedChildSpansToWaitFor was checked outside @synchronized(self), allowing concurrent threads to both see "no unfinished children" and call finishInternal. Moved the check inside the synchronized block. 2. Volatile currentRequest re-reads: SentryTracePropagation and SentryNetworkTracker accessed sessionTask.currentRequest multiple times without retaining. The property can return a freed object if the task completes on another thread between reads. Snapshot the request once into a local variable in both methods. 3. Weak span dangling refs in TTD callback: SentryTimeToDisplayTracker's finishCallback accessed weak initialDisplaySpan/fullDisplaySpan properties repeatedly. Strongify into locals at callback entry with nil guards. Closes #8012 * test: add concurrency tests for span lifecycle crash fixes Cover the three crash paths fixed in the previous commit: - SentryTracerTests: concurrent child span finish racing with tracer.finish() to verify canBeFinished atomicity - SentryNetworkTrackerTests: concurrent resume + setState on the same task, and resume after task already completed - SentryTimeToDisplayTrackerTest: concurrent tracer finish with child span operations, and finish with no full display span - SentryTracePropagationTests: addBaggageHeader with nil currentRequest (task with no request set) * chore: update CHANGELOG to include fix for EXC_BAD_ACCESS in SentryTracer/SentryNetworkTracker span lifecycle * fix: revert TOCTOU lock change in canBeFinished Moving hasUnfinishedChildSpansToWaitFor inside @synchronized(self) introduced an ABBA deadlock: canBeFinished takes self then _children, while finishTracer holds _children and re-enters canBeFinished via spanFinished, waiting on self. The original TOCTOU was already guarded by the double-check lock in finishTracer (lines 580-588), so the move was unnecessary. * fix: add nil check for initialSpan in SentryTimeToDisplayTracker
1 parent 7f28cd4 commit b404da1

10 files changed

Lines changed: 262 additions & 64 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@
3232
- Record log_byte client reports (#8186)
3333
- Add scope feature flag API (#8147)
3434

35+
### Fixes
36+
37+
- Fix EXC_BAD_ACCESS in SentryNetworkTracker caused by repeated reads of the volatile `NSURLSessionTask.currentRequest` property (#8058)
38+
3539
## 9.19.1
3640

3741
### Fixes

‎Sources/Sentry/SentryNetworkTracker.m‎

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,10 @@ - (void)urlSessionTaskResume:(NSURLSessionTask *)sessionTask
123123
return;
124124
}
125125

126-
NSURL *url = [[sessionTask currentRequest] URL];
126+
// Snapshot currentRequest once — the property is volatile and can return a freed
127+
// object if the task completes on another thread between repeated accesses.
128+
NSURLRequest *currentRequest = sessionTask.currentRequest;
129+
NSURL *url = currentRequest.URL;
127130

128131
if (url == nil) {
129132
return;
@@ -167,14 +170,13 @@ - (void)urlSessionTaskResume:(NSURLSessionTask *)sessionTask
167170
id<SentrySpan> _Nullable currentSpan = [SentrySDKInternal.currentHub.scope span];
168171
if (currentSpan != nil) {
169172
span = currentSpan;
170-
netSpan = [span startChildWithOperation:SentrySpanOperationNetworkRequestOperation
171-
description:[NSString stringWithFormat:@"%@ %@",
172-
sessionTask.currentRequest.HTTPMethod,
173-
safeUrl.sanitizedUrl]];
173+
netSpan =
174+
[span startChildWithOperation:SentrySpanOperationNetworkRequestOperation
175+
description:[NSString stringWithFormat:@"%@ %@",
176+
currentRequest.HTTPMethod, safeUrl.sanitizedUrl]];
174177
netSpan.origin = SentryTraceOriginAutoHttpNSURLSession;
175178

176-
[netSpan setDataValue:sessionTask.currentRequest.HTTPMethod
177-
forKey:@"http.request.method"];
179+
[netSpan setDataValue:currentRequest.HTTPMethod forKey:@"http.request.method"];
178180
[netSpan setDataValue:safeUrl.sanitizedUrl forKey:@"url"];
179181
[netSpan setDataValue:@"fetch" forKey:@"type"];
180182

@@ -257,7 +259,8 @@ - (void)urlSessionTask:(NSURLSessionTask *)sessionTask setState:(NSURLSessionTas
257259
return;
258260
}
259261

260-
NSURL *url = [[sessionTask currentRequest] URL];
262+
NSURLRequest *currentRequest = sessionTask.currentRequest;
263+
NSURL *url = currentRequest.URL;
261264

262265
if (url == nil) {
263266
return;
@@ -285,9 +288,9 @@ - (void)urlSessionTask:(NSURLSessionTask *)sessionTask setState:(NSURLSessionTas
285288
// - suspended → canceling (task cancelled while suspended)
286289
if (sessionTask.state == NSURLSessionTaskStateRunning
287290
|| sessionTask.state == NSURLSessionTaskStateSuspended) {
288-
[self captureFailedRequests:sessionTask];
291+
[self captureFailedRequests:sessionTask currentRequest:currentRequest];
289292

290-
[self addBreadcrumbForSessionTask:sessionTask];
293+
[self addBreadcrumbForSessionTask:sessionTask currentRequest:currentRequest];
291294

292295
NSInteger responseStatusCode = [self urlResponseStatusCode:sessionTask.response];
293296

@@ -307,15 +310,15 @@ - (void)urlSessionTask:(NSURLSessionTask *)sessionTask setState:(NSURLSessionTas
307310
}
308311

309312
- (void)captureFailedRequests:(NSURLSessionTask *)sessionTask
313+
currentRequest:(NSURLRequest *)currentRequest
310314
{
311315
if (!self.isCaptureFailedRequestsEnabled) {
312316
SENTRY_LOG_DEBUG(
313317
@"captureFailedRequestsEnabled is disabled, not capturing HTTP Client errors.");
314318
return;
315319
}
316320

317-
// if request or response are null, we can't raise the event
318-
if (sessionTask.currentRequest == nil || sessionTask.response == nil) {
321+
if (currentRequest == nil || sessionTask.response == nil) {
319322
SENTRY_LOG_DEBUG(@"Request or Response are null, not capturing HTTP Client errors.");
320323
return;
321324
}
@@ -327,7 +330,6 @@ - (void)captureFailedRequests:(NSURLSessionTask *)sessionTask
327330
return;
328331
}
329332
NSHTTPURLResponse *myResponse = (NSHTTPURLResponse *)sessionTask.response;
330-
NSURLRequest *myRequest = sessionTask.currentRequest;
331333
NSNumber *responseStatusCode = @(myResponse.statusCode);
332334

333335
if (![self containsStatusCode:myResponse.statusCode]) {
@@ -337,7 +339,7 @@ - (void)captureFailedRequests:(NSURLSessionTask *)sessionTask
337339
}
338340

339341
if (![SentryTracePropagation
340-
isTargetMatch:SENTRY_UNWRAP_NULLABLE(NSURL, myRequest.URL)
342+
isTargetMatch:SENTRY_UNWRAP_NULLABLE(NSURL, currentRequest.URL)
341343
withTargets:SentrySDKInternal.options.failedRequestTargets ?: @[]]) {
342344
SENTRY_LOG_DEBUG(
343345
@"Request url isn't within the request targets, not capturing HTTP Client errors.");
@@ -372,16 +374,16 @@ - (void)captureFailedRequests:(NSURLSessionTask *)sessionTask
372374

373375
SentryRequest *request = [[SentryRequest alloc] init];
374376

375-
UrlSanitized *url = [[UrlSanitized alloc]
376-
initWithURL:SENTRY_UNWRAP_NULLABLE(NSURL, [[sessionTask currentRequest] URL])];
377+
UrlSanitized *url =
378+
[[UrlSanitized alloc] initWithURL:SENTRY_UNWRAP_NULLABLE(NSURL, currentRequest.URL)];
377379

378380
request.url = url.sanitizedUrl;
379-
request.method = myRequest.HTTPMethod;
381+
request.method = currentRequest.HTTPMethod;
380382
request.fragment = url.fragment;
381383
request.queryString = url.query;
382384
request.bodySize = [NSNumber numberWithLongLong:sessionTask.countOfBytesSent];
383-
if (nil != myRequest.allHTTPHeaderFields) {
384-
NSDictionary<NSString *, NSString *> *headers = myRequest.allHTTPHeaderFields.copy;
385+
if (nil != currentRequest.allHTTPHeaderFields) {
386+
NSDictionary<NSString *, NSString *> *headers = currentRequest.allHTTPHeaderFields.copy;
385387
request.headers = [HTTPHeaderSanitizer sanitizeHeaders:headers];
386388
}
387389

@@ -428,6 +430,7 @@ - (BOOL)containsStatusCode:(NSInteger)statusCode
428430
}
429431

430432
- (void)addBreadcrumbForSessionTask:(NSURLSessionTask *)sessionTask
433+
currentRequest:(NSURLRequest *)currentRequest
431434
{
432435
if (!self.isNetworkBreadcrumbEnabled) {
433436
return;
@@ -449,13 +452,13 @@ - (void)addBreadcrumbForSessionTask:(NSURLSessionTask *)sessionTask
449452
SentryBreadcrumb *breadcrumb = [[SentryBreadcrumb alloc] initWithLevel:breadcrumbLevel
450453
category:@"http"];
451454

452-
UrlSanitized *urlComponents = [[UrlSanitized alloc]
453-
initWithURL:SENTRY_UNWRAP_NULLABLE(NSURL, sessionTask.currentRequest.URL)];
455+
UrlSanitized *urlComponents =
456+
[[UrlSanitized alloc] initWithURL:SENTRY_UNWRAP_NULLABLE(NSURL, currentRequest.URL)];
454457

455458
breadcrumb.type = @"http";
456459
NSMutableDictionary<NSString *, id> *breadcrumbData = [[NSMutableDictionary alloc] init];
457460
breadcrumbData[@"url"] = urlComponents.sanitizedUrl;
458-
breadcrumbData[@"method"] = sessionTask.currentRequest.HTTPMethod;
461+
breadcrumbData[@"method"] = currentRequest.HTTPMethod;
459462
breadcrumbData[@"request_start"] = requestStart;
460463
breadcrumbData[@"request_body_size"] =
461464
[NSNumber numberWithLongLong:sessionTask.countOfBytesSent];

‎Sources/Sentry/SentryTimeToDisplayTracker.m‎

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -89,32 +89,37 @@ - (BOOL)startForTracer:(SentryTracer *)tracer
8989
[tracer setFinishCallback:^(SentryTracer *_tracer) {
9090
[SentryDependencyContainer.sharedInstance.framesTracker removeListener:self];
9191

92-
// The tracer finishes when the screen is fully displayed. Therefore, we must also finish
93-
// the TTID span.
94-
if (self.initialDisplaySpan.isFinished == NO) {
95-
[self.initialDisplaySpan finish];
92+
// Strongify the weak span references so they can't become dangling
93+
// mid-callback if the tracer's children are released concurrently.
94+
id<SentrySpan> initialSpan = self.initialDisplaySpan;
95+
id<SentrySpan> fullSpan = self.fullDisplaySpan;
96+
97+
if (initialSpan != nil && initialSpan.isFinished == NO) {
98+
[initialSpan finish];
9699
}
97100

98-
// If the start time of the tracer changes, which is the case for app start transactions, we
99-
// also need to adapt the start time of our spans.
100-
self.initialDisplaySpan.startTimestamp = _tracer.startTimestamp;
101-
[self addTimeToDisplayMeasurement:self.initialDisplaySpan name:@"time_to_initial_display"];
101+
if (initialSpan != nil) {
102+
initialSpan.startTimestamp = _tracer.startTimestamp;
103+
[self addTimeToDisplayMeasurement:initialSpan name:@"time_to_initial_display"];
104+
}
102105

103-
if (self.fullDisplaySpan == nil) {
106+
if (fullSpan == nil) {
104107
return;
105108
}
106109

107-
self.fullDisplaySpan.startTimestamp = _tracer.startTimestamp;
108-
[self addTimeToDisplayMeasurement:self.fullDisplaySpan name:@"time_to_full_display"];
110+
fullSpan.startTimestamp = _tracer.startTimestamp;
111+
[self addTimeToDisplayMeasurement:fullSpan name:@"time_to_full_display"];
109112

110-
if (self.fullDisplaySpan.status != kSentrySpanStatusDeadlineExceeded) {
113+
if (fullSpan.status != kSentrySpanStatusDeadlineExceeded) {
111114
return;
112115
}
113116

114-
self.fullDisplaySpan.timestamp = self.initialDisplaySpan.timestamp;
115-
self.fullDisplaySpan.spanDescription = [NSString
116-
stringWithFormat:@"%@ - Deadline Exceeded", self.fullDisplaySpan.spanDescription];
117-
[self addTimeToDisplayMeasurement:self.fullDisplaySpan name:@"time_to_full_display"];
117+
if (initialSpan != nil) {
118+
fullSpan.timestamp = initialSpan.timestamp;
119+
}
120+
fullSpan.spanDescription =
121+
[NSString stringWithFormat:@"%@ - Deadline Exceeded", fullSpan.spanDescription];
122+
[self addTimeToDisplayMeasurement:fullSpan name:@"time_to_full_display"];
118123
}];
119124

120125
return YES;

‎Sources/Sentry/SentryTracePropagation.m‎

Lines changed: 17 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -15,40 +15,40 @@ + (void)addBaggageHeader:(SentryBaggage *)baggage
1515
tracePropagationTargets:(NSArray *_Nullable)tracePropagationTargets
1616
toRequest:(NSURLSessionTask *)sessionTask
1717
{
18-
if (![SentryTracePropagation sessionTaskRequiresPropagation:sessionTask
19-
tracePropagationTargets:tracePropagationTargets]) {
20-
SENTRY_LOG_DEBUG(@"Not adding trace_id and baggage headers for %@",
21-
sessionTask.currentRequest.URL.absoluteString);
18+
// Snapshot currentRequest once — the property is volatile and can become a zombie
19+
// between repeated accesses if the task completes on another thread.
20+
NSURLRequest *request = sessionTask.currentRequest;
21+
if (request == nil) {
22+
return;
23+
}
24+
25+
if (![SentryTracePropagation isTargetMatch:SENTRY_UNWRAP_NULLABLE(NSURL, request.URL)
26+
withTargets:tracePropagationTargets ?: @[]]) {
27+
SENTRY_LOG_DEBUG(
28+
@"Not adding trace_id and baggage headers for %@", request.URL.absoluteString);
2229
return;
2330
}
2431
NSString *baggageHeader = @"";
2532

2633
if (baggage != nil) {
27-
NSString *_Nullable rawHeader = SENTRY_UNWRAP_NULLABLE(
28-
NSString, sessionTask.currentRequest.allHTTPHeaderFields[SENTRY_BAGGAGE_HEADER]);
34+
NSString *_Nullable rawHeader
35+
= SENTRY_UNWRAP_NULLABLE(NSString, request.allHTTPHeaderFields[SENTRY_BAGGAGE_HEADER]);
2936
NSDictionary *originalBaggage = [SentryBaggageSerialization decode:rawHeader ?: @""];
3037
if (originalBaggage[@"sentry-trace_id"] == nil) {
3138
baggageHeader = [baggage toHTTPHeaderWithOriginalBaggage:originalBaggage];
3239
}
3340
}
3441

35-
// First we check if the current request is mutable, so we could easily add a new
36-
// header. Otherwise we try to change the current request for a new one with the extra
37-
// header.
38-
if ([sessionTask.currentRequest isKindOfClass:[NSMutableURLRequest class]]) {
39-
NSMutableURLRequest *currentRequest = (NSMutableURLRequest *)sessionTask.currentRequest;
40-
[SentryTracePropagation addHeaderFieldsToRequest:currentRequest
42+
if ([request isKindOfClass:[NSMutableURLRequest class]]) {
43+
NSMutableURLRequest *mutableRequest = (NSMutableURLRequest *)request;
44+
[SentryTracePropagation addHeaderFieldsToRequest:mutableRequest
4145
traceHeader:traceHeader
4246
baggageHeader:baggageHeader
4347
propagateTraceparent:propagateTraceparent];
4448
} else {
45-
// Even though NSURLSessionTask doesn't have 'setCurrentRequest', some subclasses
46-
// do. For those subclasses we replace the currentRequest with a mutable one with
47-
// the additional trace header. Since NSURLSessionTask is a public class and can be
48-
// overridden, we believe this is not considered a private api.
4949
SEL setCurrentRequestSelector = NSSelectorFromString(@"setCurrentRequest:");
5050
if ([sessionTask respondsToSelector:setCurrentRequestSelector]) {
51-
NSMutableURLRequest *newRequest = [sessionTask.currentRequest mutableCopy];
51+
NSMutableURLRequest *newRequest = [request mutableCopy];
5252
[SentryTracePropagation addHeaderFieldsToRequest:newRequest
5353
traceHeader:traceHeader
5454
baggageHeader:baggageHeader
@@ -61,15 +61,6 @@ + (void)addBaggageHeader:(SentryBaggage *)baggage
6161
}
6262
}
6363

64-
+ (BOOL)sessionTaskRequiresPropagation:(NSURLSessionTask *)sessionTask
65-
tracePropagationTargets:(NSArray *)tracePropagationTargets
66-
{
67-
return sessionTask.currentRequest != nil &&
68-
[SentryTracePropagation
69-
isTargetMatch:SENTRY_UNWRAP_NULLABLE(NSURL, sessionTask.currentRequest.URL)
70-
withTargets:tracePropagationTargets];
71-
}
72-
7364
+ (void)addHeaderFieldsToRequest:(NSMutableURLRequest *)request
7465
traceHeader:(SentryTraceHeader *)traceHeader
7566
baggageHeader:(NSString *)baggageHeader

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

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,23 @@ class SentryNetworkTrackerTests: XCTestCase {
340340
XCTAssertEqual(SentryNetworkTrackerTests.origin, span.origin)
341341
}
342342

343+
func testSpanData_VolatileCurrentRequest_UsesSnapshot() throws {
344+
var request = URLRequest(url: SentryNetworkTrackerTests.fullUrl)
345+
request.httpMethod = "GET"
346+
let task = VolatileRequestTaskMock(request: request)
347+
task.currentRequestAccessLimit = 1
348+
349+
let sut = fixture.getSut()
350+
let transaction = startTransaction()
351+
sut.urlSessionTaskResume(task)
352+
353+
let spans = Dynamic(transaction).children as [Span]?
354+
let span = try XCTUnwrap(spans?.first)
355+
356+
XCTAssertEqual(span.spanDescription, "GET \(SentryNetworkTrackerTests.testUrl)")
357+
XCTAssertEqual(span.data["http.request.method"] as? String, "GET")
358+
}
359+
343360
func testStatusForTaskRunning() {
344361
let sut = fixture.getSut()
345362
let task = createDataTask()
@@ -1486,4 +1503,46 @@ class SentryNetworkTrackerTests: XCTestCase {
14861503
request.httpMethod = method
14871504
return URLSessionStreamTaskMock(request: request)
14881505
}
1506+
1507+
// MARK: - Concurrent resume + setState race (issue #8012)
1508+
1509+
func testResumeConcurrentWithSetState_DoesNotCrash() {
1510+
let sut = fixture.getSut()
1511+
1512+
let queue = DispatchQueue(label: "resume-setState-race", qos: .userInteractive, attributes: [.concurrent, .initiallyInactive])
1513+
let iterations = 500
1514+
let expectation = XCTestExpectation(description: "Concurrent resume and setState")
1515+
expectation.expectedFulfillmentCount = iterations * 2
1516+
expectation.assertForOverFulfill = true
1517+
1518+
for _ in 0..<iterations {
1519+
let task = createDataTask()
1520+
_ = startTransaction()
1521+
1522+
queue.async {
1523+
sut.urlSessionTaskResume(task)
1524+
expectation.fulfill()
1525+
}
1526+
queue.async {
1527+
task.state = .completed
1528+
sut.urlSessionTask(task, setState: .completed)
1529+
expectation.fulfill()
1530+
}
1531+
}
1532+
1533+
queue.activate()
1534+
wait(for: [expectation], timeout: 10)
1535+
}
1536+
1537+
func testResumeAfterTaskCompleted_DoesNotCrash() {
1538+
let sut = fixture.getSut()
1539+
let transaction = startTransaction()
1540+
let task = createDataTask()
1541+
1542+
task.state = .completed
1543+
sut.urlSessionTaskResume(task)
1544+
1545+
let spans = Dynamic(transaction).children as [Span]?
1546+
XCTAssertEqual(spans?.count ?? 0, 0)
1547+
}
14891548
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,29 @@ final class SentryTracePropagationTests: XCTestCase {
147147
XCTAssertTrue(SentryTracePropagation.isTargetMatch(localhostURL, withTargets: targetsWithInvalidType))
148148
}
149149

150+
// MARK: - currentRequest snapshot safety (issue #8012)
151+
152+
func testAddBaggageHeader_TaskWithNoCurrentRequest_DoesNotCrash() throws {
153+
let emptyBaggage = Baggage()
154+
let traceHeader = TraceHeader(
155+
trace: SentryId(),
156+
spanId: SpanId(),
157+
sampled: .yes
158+
)
159+
160+
let task = URLSessionDataTaskMock()
161+
162+
SentryTracePropagation.addBaggageHeader(
163+
emptyBaggage,
164+
traceHeader: traceHeader,
165+
propagateTraceparent: true,
166+
tracePropagationTargets: [try XCTUnwrap(NSRegularExpression(pattern: ".*"))],
167+
toRequest: task
168+
)
169+
170+
XCTAssertNil(task.currentRequest?.value(forHTTPHeaderField: "sentry-trace"))
171+
}
172+
150173
private func createSessionTask(method: String = "GET") throws -> URLSessionDownloadTaskMock {
151174
let url = try XCTUnwrap(URL(string: "https://www.domain.com/api?query=value&query2=value2#fragment"))
152175
var request = URLRequest(url: url)

‎Tests/SentryTests/Integrations/Performance/Network/URLSessionTaskMock.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,4 +75,15 @@ static int64_t const DATA_BYTES_SENT = 652;
7575

7676
@end
7777

78+
/**
79+
* A mock that returns nil from currentRequest after a configurable number of accesses.
80+
* Simulates the real-world race where NSURLSessionTask.currentRequest becomes a dangling
81+
* pointer (or nil) when the task completes on another thread between repeated reads.
82+
*/
83+
@interface VolatileRequestTaskMock : URLSessionDataTaskMock
84+
85+
@property (nonatomic) NSUInteger currentRequestAccessLimit;
86+
87+
@end
88+
7889
NS_ASSUME_NONNULL_END

0 commit comments

Comments
 (0)