fix: resolve EXC_BAD_ACCESS in SentryTracer/SentryNetworkTracker span lifecycle#8058
fix: resolve EXC_BAD_ACCESS in SentryTracer/SentryNetworkTracker span lifecycle#8058itaybre wants to merge 8 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8058 +/- ##
=============================================
- Coverage 87.306% 87.203% -0.104%
=============================================
Files 554 559 +5
Lines 31986 32172 +186
Branches 13139 13146 +7
=============================================
+ Hits 27926 28055 +129
- Misses 4012 4069 +57
Partials 48 48
... and 21 files with indirect coverage changes Continue to review full report in Codecov by Harness.
|
… 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
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)
itaybre
force-pushed
the
fix/exc-bad-access-tracer-network-span-lifecycle
branch
from
ad1d81a to
2712c25
Compare
…acer/SentryNetworkTracker span lifecycle
📲 Install BuildsiOS
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 52f26a1 | 1227.29 ms | 1261.50 ms | 34.21 ms |
| 8c39e1a | 1223.40 ms | 1255.42 ms | 32.02 ms |
| 8d75f5a | 1223.90 ms | 1255.94 ms | 32.04 ms |
| 943b250 | 1214.69 ms | 1257.32 ms | 42.63 ms |
| 4c437ea | 1217.19 ms | 1250.79 ms | 33.60 ms |
| 8180609 | 1214.67 ms | 1243.36 ms | 28.69 ms |
| 2c9ee54 | 1226.87 ms | 1251.10 ms | 24.23 ms |
| 1887b2e | 1218.51 ms | 1243.06 ms | 24.55 ms |
| 1c5ecda | 1219.35 ms | 1253.76 ms | 34.41 ms |
| e983818 | 1214.41 ms | 1245.20 ms | 30.80 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 52f26a1 | 24.14 KiB | 1.19 MiB | 1.17 MiB |
| 8c39e1a | 24.14 KiB | 1.22 MiB | 1.20 MiB |
| 8d75f5a | 24.14 KiB | 1.17 MiB | 1.15 MiB |
| 943b250 | 24.14 KiB | 1.17 MiB | 1.15 MiB |
| 4c437ea | 24.14 KiB | 1.18 MiB | 1.15 MiB |
| 8180609 | 24.14 KiB | 1.16 MiB | 1.13 MiB |
| 2c9ee54 | 24.14 KiB | 1.17 MiB | 1.15 MiB |
| 1887b2e | 24.14 KiB | 1.15 MiB | 1.13 MiB |
| 1c5ecda | 24.14 KiB | 1.15 MiB | 1.12 MiB |
| e983818 | 24.14 KiB | 1.22 MiB | 1.20 MiB |
Previous results on branch: fix/exc-bad-access-tracer-network-span-lifecycle
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a6c6416 | 1220.39 ms | 1243.80 ms | 23.41 ms |
| 1119cfd | 1232.30 ms | 1269.40 ms | 37.10 ms |
| 6fdbc97 | 1212.43 ms | 1248.49 ms | 36.06 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a6c6416 | 24.14 KiB | 1.22 MiB | 1.20 MiB |
| 1119cfd | 24.14 KiB | 1.22 MiB | 1.20 MiB |
| 6fdbc97 | 24.14 KiB | 1.22 MiB | 1.20 MiB |
itaybre
requested review from
NinjaLikesCheez,
noahsmartin,
philipphofmann and
philprime
as code owners
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6435c68. Configure here.
…to ensure timestamp assignment
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
| // 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; |
There was a problem hiding this comment.
m: The SentryTracePropagation.addBaggageHeader only gets called from the SentryNetworkTracker.urlSessionTaskResume which already snapshots the sessionTask.currentRequest before the @synchronized(sessionTask) {. We could also pass the already snapshotted currentRequest as a parameter to the SentryTracePropagation.addBaggageHeader.
|
|
||
| // MARK: - Weak span safety in finishCallback (issue #8012) | ||
|
|
||
| func testFinishCallback_ConcurrentTracerFinish_DoesNotCrash() throws { |
There was a problem hiding this comment.
m: I ran these two tests repeatedly (1000 times) without the fix in this PR, and they didn't crash. Are you sure they reproduce the crash? Am I missing something?
There was a problem hiding this comment.
It failed for me when I ran, but seems to be non deterministic (or a later change broke it).
And it was more of an integration test. I will add a different test here
| // 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; |
There was a problem hiding this comment.
m: Snapshotting this could reduce the chance of returning a freed object, but if the root cause is multiple threads modifying this, this crash will happen again. Or am I missing something?
There was a problem hiding this comment.
We are snapshotting a copy, so the possibility of this is lower.
I believe this is one of the limitation of swizzling native frameworks, we don't know how apple may access / modify the tasks so we cant do much aside from protecting every time we access the object. |
3 participants
Description
Fix two crash paths in the
SentryTracer→SentryNetworkTrackercall chain that causeEXC_BAD_ACCESScrashes. This is the most frequent crash pattern in SDK-CRASHES-COCOA-5 (43/100 sampled events), affecting 7 customer projects across SDK versions 8.36.0–9.9.0 with ~140K events in the last 90 days.Root causes and fixes
1. TOCTOU race in
SentryTracer.canBeFinished—hasUnfinishedChildSpansToWaitForwas evaluated outside@synchronized(self), so two concurrent threads could both observe "no unfinished children" and race intofinishInternal, operating on deallocated state. Moved the check inside the synchronized block.2. Volatile
currentRequestre-reads —SentryTracePropagation.addBaggageHeader:andSentryNetworkTracker.urlSessionTaskResume:accessedsessionTask.currentRequestmultiple times without retaining. The property can return a freed object if the task completes on another thread between reads (CFURLRequestSetHTTPRequestBodycrash). Both methods now snapshot the request once into a local variable.3. Weak span dangling refs in TTD callback —
SentryTimeToDisplayTracker'sfinishCallbackaccessed weakinitialDisplaySpan/fullDisplaySpanproperties repeatedly without strongifying. The spans could be zeroed mid-callback if the tracer's children are released concurrently. Now strongified into locals at callback entry with nil guards.How tested
make format— cleanmake analyze— cleanmake build-ios— succeedsmake test-ios ONLY_TESTING=SentryTests/SentryTracerTests— 86 tests passmake test-ios ONLY_TESTING=SentryTests/SentryTracePropagationTests,SentryTests/SentryNetworkTrackerTests,SentryTests/SentryTimeToDisplayTrackerTest— 100 tests passCloses #8012