Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions Sources/Sentry/SentryNetworkTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"];

Expand Down
35 changes: 19 additions & 16 deletions Sources/Sentry/SentryTimeToDisplayTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
34 changes: 17 additions & 17 deletions Sources/Sentry/SentryTracePropagation.m
Original file line number Diff line number Diff line change
Expand Up @@ -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
// override, 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
Expand Down
4 changes: 2 additions & 2 deletions Sources/Sentry/SentryTracer.m
Original file line number Diff line number Diff line change
Expand Up @@ -501,9 +501,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.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
59 changes: 59 additions & 0 deletions Tests/SentryTests/Transaction/SentryTracerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading