diff --git a/CHANGELOG.md b/CHANGELOG.md index fe2cb93edd..286b052eaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ - Only expose `experimental.dataCollection` APIs in SDK V10 (#8435) +### Fixes + +- Prevent crash on SDK start when the app binary contains classes that reference `@available`-gated APIs. `SentrySubClassFinder` now detects `UIViewController` subclasses without realizing unrelated classes, so it no longer triggers the Swift runtime crash for classes like SwiftUI gesture coordinators or `RoomPlan`/`ActivityKit` wrappers (#8457) + ## 9.22.0 ### Features diff --git a/HANDOFF-subclassfinder-fix.md b/HANDOFF-subclassfinder-fix.md new file mode 100644 index 0000000000..183ae211dc --- /dev/null +++ b/HANDOFF-subclassfinder-fix.md @@ -0,0 +1,263 @@ +# Handoff: Fix `SentrySubClassFinder` availability crash (GH #8152) + +> Working note for picking this up across sessions. Tracks the live PR, decisions, +> and the one deferred optimization. Not part of the shipped SDK. + +## Status + +- **PR:** [#8457](https://github.com/getsentry/sentry-cocoa/pull/8457), branch + `fix/subclassfinder-availability-crash`. Open as a **draft** (intentionally — not + yet marked ready-to-merge; double-checking first). +- **Reviews:** three rounds (philipphofmann, NinjaLikesCheez, itaybre). All raised + comments addressed in code + replies; see "Review feedback" below. +- **`origin/main` merged in** (merge commit, includes `#8494` "reduce SDK-start + overhead"). Conflicts in `SentrySubClassFinder.swift` (kept our `[AnyClass]` flow + + main's empty-check early return; dropped main's `free(classes)`, which was for the + old C-array API) and its test (kept our `classes` seam + main's + `_NoViewController_DoesNotDispatchToMainQueue` test adapted to that seam). +- **Tests:** `SentrySubClassFinderTests` green on the local sim (10 tests) after the + merge. Includes the differential enumeration test, the section null-entry test, the + real-wrapper regression test, and the filter-drop test noted under Open blockers. + +## Open blockers (must resolve before ready-to-merge) + +- **Finding 2 — raw `__objc_classlist` entries are unremapped (OPEN, P1).** Full + writeup + follow-up in `REVIEW-PR-8457.md` §"Finding 2". `classes(inSection:size:)` + returns the raw compiler-emitted pointers without objc4's `remapClass`, and + `SentrySubClassFinder` carries them across the queue hop straight to the swizzler. + For a class objc4 remaps, the raw pointer differs from the live class object, which + can bypass `SentrySwizzle`'s class-identity dedup (`NSMutableSet` in + `SentrySwizzle.m`). + - **An earlier local attempt did NOT fix it** (comment + a filter-drop test only). The + review disproved the comment's core claim: objc4 only forbids a future class from + being _completed by a Swift class_, NOT from having an Objective-C view-controller + superclass. A reserved ObjC `objc_getFutureClass` VC subclass passes + `SentrySubClassFinder`'s `class_getSuperclass` filter yet has raw ≠ live identity — + demonstrated by a local probe (`SentryFutureViewController : NSViewController`, + `rawIsViewController=yes rawEqualsLive=no`). + - **Local changes currently in the tree (uncommitted):** softened comment in + `SentryDefaultObjCRuntimeWrapper.swift` (now flags this as a known open issue, no + longer claims safety) + `testActOnSubclassesOfViewController_WhenClassDoesNotReach` + `ViewController_IsNotSwizzled` (documents the filter drop for the + weak-missing-superclass shape only; explicitly NOT a Finding-2 fix). + - **Constraint on any real fix:** must NOT reintroduce the GH-8152 realization crash. + The review's "store names, resolve on main via `NSClassFromString`" suggestion + realizes the class and can pick a same-named class from another image — the exact + behavior this PR removed. A viable fix likely applies a `remapClass`-equivalent to + each entry (skipping nil results) transiently while keeping the non-realizing path. + - **Test gap:** name-set equivalence and the current filter test are insufficient; a + real regression test needs an ObjC bundle + `objc_getFutureClass` to exercise + remapping and pointer identity. + - Deferred by user decision (2026-07-21): store findings now, pick up the fix later. + +## TL;DR + +- **Issue:** [#8152](https://github.com/getsentry/sentry-cocoa/issues/8152) (related: + [#3798](https://github.com/getsentry/sentry-cocoa/issues/3798), Swift bug + [swiftlang/swift#72657](https://github.com/swiftlang/swift/issues/72657)). +- **Crash:** SDK crashes on start (UIViewController performance tracing) because + `SentrySubClassFinder` called `NSClassFromString` on every class in the app image. + That **realizes** the class; realizing a Swift class whose metadata references an + `@available`-gated newer-framework type forces Swift metadata completion and + crashes with `EXC_BAD_ACCESS` in `swift_getSingletonMetadata` on OS versions below + the framework's availability. Reproduces only on **real iOS 17.x (and older) + devices**, not simulators. +- **Real-world crashers are NOT view controllers:** SwiftUI gesture + `Coordinator: NSObject` (conforms under `UIGestureRecognizerRepresentable`, iOS + 18+), `RoomPlan`/`ActivityKit` wrappers. +- **Fix (pure Swift):** enumerate classes by reading the image's `__objc_classlist` + Mach-O section (gives class **pointers**, NOT realized) instead of + `objc_copyClassNamesForImage` + `NSClassFromString`. Then use the existing + `class_getSuperclass`-based `isClass(_:subClassOf:)` walk (already safe, never + realizes). The confirmed VC class pointers are carried to the main thread and handed + straight to the swizzle block — `NSClassFromString` is not used at all anymore (it + realizes, and could resolve a same-named class from a different image). + +## Root-cause chain + +`SentrySubClassFinder.actOnSubclassesOfViewController(inImage:)` (background queue): + +1. old: `objcRuntimeWrapper.copyClassNamesForImage` → class name C-strings (safe). +2. old: for each name → `NSClassFromString(name)` ← **THE CRASH** (realizes) → + `isClass(subclassOf: UIViewController)` walk → keep name if VC. +3. old: main thread → `NSClassFromString(name)` again → hand `Class` to swizzle block. + +Only step 2's `NSClassFromString` was the problem. `class_getSuperclass` (used by +`isClass`) was always safe. The new code drops `NSClassFromString` entirely (both +steps 2 and 3) — it walks unrealized class pointers and passes them straight to the +swizzle. + +## The fix (what changed) + +Same structure, but get class **pointers** without realizing: + +- Added `classes(forImage:) -> [AnyClass]` to `SentryObjCRuntimeWrapper` protocol. +- Default impl (`SentryDefaultObjCRuntimeWrapper`): `import MachO`; find the image + via `_dyld_image_count`/`_dyld_get_image_name`; read + `getsectiondata(header, "__DATA_CONST", "__objc_classlist", &size)` (fallback + `"__DATA"`); rebind the section to `AnyClass?` (the honest type of its + `Class _Nullable` entries — no `unsafeBitCast`) and `compactMap` out nulls. These + are dyld-bound but **not realized**. +- Finder: iterate `classes(forImage:)`, run the unchanged `isClass` superclass walk, + read names via `class_getName` only to apply `swizzleClassNameExcludes` (neither + `class_getSuperclass` nor `class_getName` sends a message, so `+initialize` never + runs on the background thread), then collect the class **pointers** and hand them + directly to the main-thread swizzle block. Holding/iterating `AnyClass` does not + message the class either; the swizzle is the first message, and it runs on main. + +### Files changed (PR #8457, vs origin/main) + +- `Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift` — swap + enumeration; net simpler. +- `Sources/Swift/Core/Integrations/Performance/SentryUIViewControllerSwizzling.swift` + — comment note that `objc_getClassList` realizes every class. +- `Sources/Swift/Helper/SentryObjCRuntimeWrapper.swift` — add `classes(forImage:)` + to protocol (+ threading/arch doc). +- `Sources/Swift/Helper/SentryDefaultObjCRuntimeWrapper.swift` — implement it + (`import MachO`), arch guard, `MH_MAGIC_64` guard, off-main-thread doc. Section + parsing extracted to a testable `static func classes(inSection:size:)` that reads + entries as `AnyClass?` and `compactMap`s out nulls (no `unsafeBitCast`). +- `CHANGELOG.md` — `## Unreleased` → `### Fixes` entry (#8457). +- `Tests/.../SentrySubClassFinderTests.swift` — new `classes` seam; differential test + (`testClassesForImage_whenReadingEveryLoadedImage_shouldMatchCopyClassNamesForImage`), + section null-entry test (`testClassesInSection_whenSectionContainsNullEntry_shouldSkipIt`), + real-wrapper regression test + (`testRealRuntimeWrapper_whenReadingBundleImage_findsBundleViewControllers`) + gated + `AvailabilityGatedNonViewController` fixture, and the merged-in + `_NoViewController_DoesNotDispatchToMainQueue` test. +- `Tests/SentryTests/Helper/SentryTestObjCRuntimeWrapper.{h,m}` — added `classes` + override block (parallels existing `classesNames`). + +Note: `Sources/Swift/Options.swift` and `Sources/SentryObjC/Public/SentryObjCOptions.h` +are NOT changed for docs — an earlier draft added a `swizzleClassNameExcludes` +"fallback" paragraph that was removed per review; the option's doc is unchanged from +`main`. + +## Review feedback (all addressed) + +- **arch guard** (NinjaLikesCheez, `m`): gate the `#if` to 64-bit archs since we bind + to `mach_header_64`. Done: `(arch(arm64) || arch(x86_64))`. Used `x86_64` (not + `arm64e`) because Swift's `arch(arm64)` already matches `arm64e`, and `x86_64` is + needed for the Intel simulator. Commit `62b002c8d`. +- **magic check** (itaybre `l` → NinjaLikesCheez upgraded to `h`, tied to the FAT + thread): verify `header.pointee.magic == MH_MAGIC_64` before rebinding. Done. Also + answers FAT: a FAT container has `FAT_MAGIC`, fails the check, so we skip rather + than misread. No FAT parsing added — `_dyld_get_image_header` only returns thin, + in-memory, native-arch slices, so a FAT header can't reach this code. Mirrors the + repo's only precedent, `firstCmdAfterHeader` in `SentryCrashDynamicLinker.c`. +- **arm64e ptrauth** (itaybre, `m`): **RESOLVED — device-validated safe.** Built the + iOS-Swift sample `ARCHS=arm64e ONLY_ACTIVE_ARCH=NO` and ran on a physical iPhone 12 + (A14, iOS 26.5); app + `iOS-Swift.debug.dylib` + `Sentry.framework` all arm64e. The + SDK read 44 classrefs from the arm64e `__objc_classlist`, walked superclasses, and + swizzled 31 UIViewController subclasses with zero `EXC_BAD_ACCESS`. dyld applies + chained fixups **in place** at load time, so the in-memory section already holds + runtime-correct pointers that `class_getSuperclass`/`class_getName` accept directly — + no ptrauth strip needed. (Static/on-disk parsers would still need to decode fixups.) +- **`unsafeBitcast` to `mach_header_64` to save the rebind** (NinjaLikesCheez, `l`): + kept `withMemoryRebound` for clarity. +- **dyld lock (new, self-found):** verified against Apple dyld source + (`DyldAPIs.cpp`): `_dyld_image_count()` does NOT lock, but + `_dyld_get_image_header`/`_dyld_get_image_name` acquire the dyld loader **read + lock** (`withLoadersReadLock`). So `classes(forImage:)` must run off the main + thread. Added doc comments on both the impl and the protocol. No behavior change: + the sole caller already runs it on a background queue via `dispatchAsync`. + +## Why it's safe (verified, not assumed) + +Verified against **objc4 source** + empirical tests on Apple silicon (arm64): + +1. `class_getSuperclass` = `cls->getSuperclass()` — pure field read, **never + realizes**, handles arm64e ptrauth internally. The only realizing/crashing call + was `NSClassFromString`. +2. `objc_copyClassNamesForImage` returns `demangledName(false)`; `class_getName` + returns the same demangled form. Neither realizes. So new names == old names by + construction. +3. `class_getName` and the superclass walk do **not** trigger `+initialize` — + `+initialize` fires only on the first _message send_, and neither function messages + the class. Holding/iterating an `AnyClass` in a Swift array doesn't message it + either (metatype pointer, no retain-message) — unlike ObjC `NSArray addObject:`. + Guarded by `testGettingSubclasses_DoesNotCallInitializer` (registers a class with a + custom `+initialize`, asserts the finder never calls it). This is why passing the + class pointer to the main thread (instead of a name) is safe: the swizzle block is + the first message and it runs on main. +4. **Classes with a Swift-generic superclass** (e.g. a `UIHostingController` + subclass) are **not in `__objc_classlist`** — and `objc_copyClassNamesForImage` + doesn't return them either (same section). So switching enumeration loses **zero** + coverage; those VCs were never image-enumerated (handled by the root-VC swizzle + path). Key finding that made a more complex approach unnecessary. +5. arm64e: no hand-rolled pointer stripping (we call `class_getSuperclass`, which + authenticates correctly). App images are almost always plain arm64 anyway. + +### Differential/oracle evidence + +- `testClassesForImage_whenReadingEveryLoadedImage_shouldMatchCopyClassNamesForImage` + iterates every loaded image and asserts the new (`__objc_classlist` + `class_getName`) + name set equals the old (`objc_copyClassNamesForImage`) set. Never calls + `NSClassFromString`, so it's safe on every OS. Permanent CI guard for the one thing + that changed. +- Local throwaway oracle (earlier session): across ~163 images / ~9,610 classes, new + == old, 0 mismatches; superclass walk over all classes: no crash, max depth 7, 0 + cycles. + +## Tests (9 total, all green) + +- Behavioral tests inject `[AnyClass]` via the `classes` seam (find VCs, honor + excludes, ignore non-VC, wrong image, no `+initialize`, and — from the merged + `#8494` — no main-queue dispatch when there's nothing to swizzle). +- `testClassesForImage_whenReadingEveryLoadedImage_shouldMatchCopyClassNamesForImage` + — differential enumeration equivalence (above). +- `testClassesInSection_whenSectionContainsNullEntry_shouldSkipIt` — hands the + `classes(inSection:size:)` helper a crafted buffer with a null slot and asserts the + null is skipped. +- `testRealRuntimeWrapper_whenReadingBundleImage_findsBundleViewControllers` — drives + the **real** `SentryDefaultObjCRuntimeWrapper` through the finder against this + bundle; asserts the bundle's VCs are found and non-VCs (incl. the gated + `AvailabilityGatedNonViewController`) are not. The actual `EXC_BAD_ACCESS` is + device/OS-version-specific and can't be reproduced on CI sims, so this guards the + enumeration + selection behavior, not the crash itself. +- Requires `@_spi(Private) @testable import Sentry` (the default wrapper is + `@_spi(Private)`). + +## Deferred / future work (NOT blocking this PR) + +- **Reuse `SentryBinaryImageCache` instead of scanning all dyld images.** + `classes(forImage:)` currently loops `_dyld_image_count()` to find the header. + `SentryBinaryImageInfo.address` IS the in-memory `mach_header` pointer + (`SentryCrashDynamicLinker.c:414` `buffer->address = (uintptr_t)header`), so we + could pass that straight to `getsectiondata` and skip the loop. **Caveats to + resolve first:** (1) the cache is populated **async on a background thread** in prod + (`dispatch_async` in `sentrycrashbic_startCache`), so it may be empty/incomplete + when the finder runs at startup — needs a fallback to the dyld scan; (2) it's + indexed **by address, not name** — no by-name lookup exists, would need a linear + scan of `getAllBinaryImages()` or a new accessor. Decide if the win is worth the + coupling. (User may tackle this later.) +- **On-device arm64e run: DONE** (see arm64e item above — iPhone 12, no crash). +- **Perf** of the section read + superclass walk vs the old path on a real device. + +## Rejected over-engineered approach (do NOT resurrect unless needed) + +First attempt added a C file doing raw `vm_read`/`sentrycrashmem_copySafely` reads of +`class_t.superclass`, `ptrauth_strip`, name-demangle matching, plus a `_SentryPrivate.h` +import. It hit the `scripts/check-sentrycrash-imports.sh` ratchet and was unnecessary +once we confirmed `class_getSuperclass` is realization-free. The SentryCrash module has +battle-tested non-realizing class introspection if ever needed, but note its name-read +path assumes a _realized_ class and its `isMemoryReadable` uses a non-thread-safe shared +static buffer. + +## Environment / invocation pitfalls + +- Test target only compiles under the **`Test` build configuration** (defines + `SENTRY_TEST`). Use `make test-ios` (sets `-configuration Test`), not a bare + XcodeBuildMCP `test_sim` with `Debug`. +- This machine's sim: override `IOS_DEVICE_NAME="iPhone 17 Pro" IOS_SIMULATOR_OS=26.4`. +- `SWIFT_TREAT_WARNINGS_AS_ERRORS=YES` / `GCC_TREAT_WARNINGS_AS_ERRORS=YES`. +- `make build-ios`/`make test-ios` honor `FOR_AGENTS=true`; full logs in + `raw-*-output.log` (gitignored). + +## Before marking ready + +- `make format` + `make analyze` clean; `SentrySubClassFinderTests` green. +- `classes(forImage:)` is `@_spi(Private)` → not in `sdk_api.json` (confirmed absent); + no `make generate-public-api` needed. +- Confirm Danger passes (changelog references **#8457**, the PR number). +- Then flip draft → ready and add `ready-to-merge` for full CI. diff --git a/REVIEW-PR-8457.md b/REVIEW-PR-8457.md new file mode 100644 index 0000000000..6e4dae8b47 --- /dev/null +++ b/REVIEW-PR-8457.md @@ -0,0 +1,353 @@ +# PR 8457 Local Review Handoff + +## Review Status + +- PR: `https://github.com/getsentry/sentry-cocoa/pull/8457` +- Title: `fix: prevent SubClassFinder availability crash` +- Reviewed head: `6e8284f8718f1fc665bafbb10ea545c8f08c7ec3` +- Base during review: `main` at `6d9248043b5919d9b675d9e743188924b44b6bf6` +- Review date: July 21, 2026 +- Verdict: **Do not merge yet** +- GitHub activity: no comments, reviews, or mutations were posted +- Local changes from review: this handoff file only + +## PR Intent + +- Fix GH-8152, where `SentrySubClassFinder` realizes unrelated Swift/Objective-C classes through `NSClassFromString`. +- Class realization can crash older OS versions when Swift metadata references an availability-gated framework type. +- Replace `objc_copyClassNamesForImage` plus `NSClassFromString` discovery with direct reads of the image's `__objc_classlist` Mach-O section. +- Walk superclasses with `class_getSuperclass` without realizing classes. +- Pass discovered `AnyClass` pointers to the main-thread swizzling block. + +## Finding 1: Concurrent Image Unload Can Crash + +### Severity + +- **P0 / merge blocker** +- Violates the SDK requirement to never crash the host application. + +### Affected Code + +- `Sources/Swift/Helper/SentryDefaultObjCRuntimeWrapper.swift:28` +- `Sources/Swift/Helper/SentryDefaultObjCRuntimeWrapper.swift:32` +- `Sources/Swift/Helper/SentryDefaultObjCRuntimeWrapper.swift:36` +- `Sources/Swift/Helper/SentryDefaultObjCRuntimeWrapper.swift:56` +- `Sources/Swift/Helper/SentryDefaultObjCRuntimeWrapper.swift:65` + +### Problem + +- `classes(forImage:)` iterates `_dyld_image_count`, matches an image name, obtains a Mach-O header, obtains a section pointer, and copies class pointers. +- The image is not pinned and no loader/runtime synchronization protects the header or section memory. +- An unload can occur after any of these operations: + - `_dyld_get_image_name(index)` + - `_dyld_get_image_header(index)` + - `getsectiondata(...)` + - before or during `UnsafeBufferPointer(...).compactMap` +- The installed Apple SDK header explicitly documents this iteration as not thread-safe: + +```c +/* + * The following functions allow you to iterate through all loaded images. + * This is not a thread safe operation. Another thread can add or remove + * an image during the iteration. + */ +``` + +- Local header inspected: + - `/Applications/Xcode-16.4.0.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS18.5.sdk/usr/include/mach-o/dyld.h:51` +- The PR comments currently claim `_dyld_get_image_header` and `_dyld_get_image_name` acquire a loader read lock and that stale indices safely return `nil`. Those claims are not supported by the public API contract. +- The old `objc_copyClassNamesForImage` implementation is materially safer. Modern objc4 uses the runtime lock plus Mach-O UUID validation to avoid concurrent-unload ABA problems before reading an image's class list. + +### Deterministic Reproduction + +- A temporary Objective-C `MH_BUNDLE` containing a class was built locally. +- The probe performed these steps: + 1. `dlopen` the bundle. + 2. Find and retain its raw Mach-O header pointer using `_dyld_get_image_header`. + 3. `dlclose` the bundle so it is unmapped. + 4. Call `getsectiondata` using the stale header. +- Result: + +```text +before=45 loaded=348 index=45 header=0x1002dc000 +after=347 afterIndex=-1 afterHeader=0x0 +probe_status=139 +``` + +- Exit status `139` is `SIGSEGV`. +- This demonstrates that an Objective-C image can unload and that dereferencing the saved header or section is unsafe. +- The production race window also extends through the entire class-pointer copy, not only the `getsectiondata` call. + +### Required Resolution + +- Do not dereference an image header or section unless image lifetime is guaranteed for the complete operation. +- Use a mechanism that safely coordinates with image loading/unloading and validates that the header still represents the requested image. +- Possible directions requiring careful validation: + - Use the SDK's binary-image cache/callback infrastructure with appropriate synchronization and image removal handling. + - Pin the target image for the operation and independently verify the header-to-path association before dereferencing it. + - Use an Objective-C runtime API that already owns the necessary runtime lock and remapping behavior, if a supported API is available. +- Merely reading `_dyld_image_count` once does not solve this issue. +- Add an unload-race test using an Objective-C bundle or an equivalent deterministic harness. + +## Finding 2: Raw Class List Entries Are Unremapped + +### Severity + +- **P1 / merge blocker** +- The implementation passes class references to swizzling that are not guaranteed to be the live runtime class objects. + +### Affected Code + +- `Sources/Swift/Helper/SentryDefaultObjCRuntimeWrapper.swift:74` +- `Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift:50` +- `Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift:67` +- `Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift:85` +- `Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift:93` + +### Problem + +- objc4 defines the entries in the image class list as `classref_t`: + +```cpp +// classref_t is unremapped class_t* +typedef struct classref * classref_t; +``` + +- Apple runtime code calls `remapClass(classlist[i])` before returning or operating on an image class. +- Remapping can: + - Replace a compiler-emitted class object with a previously allocated future class object. + - Return `nil` for a class ignored because of a missing weak-linked superclass. +- `classes(inSection:size:)` instead rebinds entries directly to `AnyClass?` and returns them unchanged. +- The direct pointers are then retained across a queue hop and passed to the Sentry swizzler. + +### Local Reproduction + +- A future class was allocated with `objc_getFutureClass("SentryFutureVictim")`. +- A bundle defining that class was then loaded. +- The raw class-list pointer was compared with the live runtime lookup. +- Result: + +```text +future=0x1013451a0 live=0x1013451a0 raw=0x100b480c0 +liveName=SentryFutureVictim rawName=SentryFutureVictim +liveSuper=0x1ef149ad8 rawSuper=0x1ef149ad8 +runtimeCount=1 first=SentryFutureVictim rawEqualsLive=no +probe_status=10 +``` + +- The raw pointer and live class pointer differ even though their names and superclasses match. +- This is exactly the case handled by objc4's `remapClass` map. + +### Why Existing Tests Miss It + +- `testClassesForImage_whenReadingEveryLoadedImage_shouldMatchCopyClassNamesForImage` compares sets of class **names**. +- A remapped class and its raw compiler class have the same name. +- The test therefore passes even when pointer identity and runtime validity differ. +- `compactMap` only removes literal null entries; it cannot apply objc4's private remapping table. + +### Consequences + +- Sentry may swizzle a replaced or ignored class object instead of the live class. +- `SentrySwizzle` deduplication records class object identity in `NSMutableSet`; raw and live remapped classes have different identities. +- A later swizzle through the live pointer may bypass once-per-class deduplication and apply a second swizzle. +- Runtime operations on ignored or otherwise remapped raw metadata are outside the supported runtime contract. + +### Recommended Resolution + +- Do not pass raw class-list pointers to the swizzler or retain them across the background-to-main queue hop. +- A safer supported-API design: + 1. Pin/synchronize the image while parsing its class list. + 2. Use raw pointers only transiently for the non-realizing superclass walk. + 3. Store the selected class names, not the raw pointers. + 4. Resolve only the selected `UIViewController` subclasses on the main thread. + 5. Verify `class_getImageName` still matches the requested image before swizzling to avoid same-name cross-image resolution. +- Resolving a selected view-controller class on main does not add a new realization requirement: the swizzling implementation immediately messages and realizes the selected class anyway. +- Add a future-class/remapping regression test; name-set equivalence is insufficient. + +### Follow-Up Review of Local Attempt + +- Follow-up reviewed on July 21, 2026. +- Local changes inspected: + - Added explanatory comments at `Sources/Swift/Helper/SentryDefaultObjCRuntimeWrapper.swift:56` justifying the intentional use of unremapped pointers. + - Added `testActOnSubclassesOfViewController_WhenClassDoesNotReachViewController_IsNotSwizzled` at `Tests/SentryTests/Integrations/Performance/SentrySubClassFinderTests.swift:157`. +- Conclusion: **the local attempt does not address Finding 2**. +- No production behavior changed. Raw `__objc_classlist` entries are still returned unchanged, retained across the queue hop, and passed to the swizzler. + +#### Disproven Assumption + +- The new comment claims a resolved future class is, in practice, never a `UIViewController` subclass. +- objc4 only prevents a future class from being completed by a Swift class. It does not restrict the Objective-C superclass hierarchy of a future class. +- A deterministic local probe reserved `SentryFutureViewController` through `objc_getFutureClass`, then loaded an Objective-C bundle defining it as an `NSViewController` subclass. +- `NSViewController` exercises the same Objective-C runtime remapping behavior as an Objective-C `UIViewController` subclass. +- Result: + +```text +future=0x10228ffc0 live=0x10228ffc0 raw=0x1026900b8 rawEqualsLive=no +rawName=SentryFutureViewController liveName=SentryFutureViewController +rawSuper=NSViewController liveSuper=NSViewController +rawIsViewController=yes liveIsViewController=yes +``` + +- The raw compiler-emitted pointer: + - Differs from the live runtime pointer. + - Has a superclass chain reaching the view-controller base class. + - Passes the same superclass filter used by `SentrySubClassFinder`. +- The current implementation would therefore forward the unremapped raw pointer to the swizzler. + +#### Why the Added Test Is Insufficient + +- `FakeViewController.self` is a normal, live runtime class pointer. It is not a raw pointer that objc4 remaps to another class or to `nil`. +- The test only reconfirms that an ordinary non-view-controller class is rejected by the superclass walk. +- That behavior was already covered by `testActOnSubclassesOfViewController_IgnoreFakeViewController`. +- The test cannot detect: + - A raw pointer differing from the live runtime class pointer. + - A future class whose raw and live pointers share the same name and superclass. + - Incorrect class-identity deduplication in `SentrySwizzle`. + - A raw pointer that objc4 would remap to `nil`. +- The updated focused suite passed `10` tests with `0` failures on iOS 18.6, but none of those tests exercises Objective-C class remapping. + +#### Finding Status + +- **Finding 2 remains open and remains a merge blocker.** +- Documentation asserting that raw pointers are safe is not a replacement for obtaining the live runtime class identity or redesigning the handoff so raw pointers never reach the swizzler. + +## Finding 3: Required SPI Protocol Method Breaks Old Conformers + +### Severity + +- **P2 / compatibility risk** +- Potential unrecognized-selector host crash for an injected conformer built against an older SDK. + +### Affected Code + +- `Sources/Swift/Helper/SentryObjCRuntimeWrapper.swift:21` +- `Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift:50` +- `Sources/Swift/SentryDependencyContainer.swift:161` + +### Problem + +- `classesForImage:` is added as a required method to the public Objective-C SPI protocol `SentryObjCRuntimeWrapper`. +- `SentryDependencyContainer.objcRuntimeWrapper` is publicly settable through the private SPI surface. +- The new method is called unconditionally. +- An Objective-C conformer compiled against the previous SDK can still be supplied at runtime but does not implement `classesForImage:`. +- Calling the new selector on that object causes an unrecognized-selector exception. + +### Scope Check + +- A read-only GitHub code search across the `getsentry` organization found no external source conformers beyond `sentry-cocoa` tests. +- This lowers the known exposure but does not eliminate private, customer, or downstream SPI users. + +### Recommended Resolution + +- Avoid adding a required method to the existing SPI protocol. +- Options: + - Introduce a separate internal capability protocol used only by the default implementation. + - Make the Objective-C requirement optional and implement an explicit safe fallback. + - Remove this operation from the injectable wrapper if injection is not needed. +- The fallback must not restore the original unsafe realization of every class. + +### Verification Verdict (2026-07-21) + +- **Verdict: TRUE — the mechanism is confirmed, and the P2 severity is correct.** +- Every link in the chain was verified first-hand against the working tree, the diff vs base (`6d9248043`), the generated public header, and the test conformer. + +#### Evidence + +- **Required method, called unconditionally.** The method is added with no `optional` keyword, and the file has no `optional` anywhere: + - `Sources/Swift/Helper/SentryObjCRuntimeWrapper.swift:22` (`@objc(classesForImage:)` + `func classes(forImage:)`) + - `Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift:50` calls it directly; there is no `respondsToSelector` / optional guard anywhere in the Swift layer. +- **Reachable by external Objective-C conformers.** The protocol is `@objc @_spi(Private) public` and is emitted into the shipped `Sentry.framework/Headers/Sentry-Swift.h` as a plain `@protocol SentryObjCRuntimeWrapper` with **no SPI guard**. + - `develop-docs/SWIFT.md:47` confirms `@_spi(Private)` restricts _Swift_ importers only; it has no effect for Objective-C. + - `Sources/Swift/SentryDependencyContainer.swift:161` exposes the setter as `@objc public var objcRuntimeWrapper` (readwrite in the header). +- **Old conformers really are missing the method (smoking gun).** The repo's own Objective-C conformer had to _add_ `classesForImage:` under the same `#if`: + - `Tests/SentryTests/Helper/SentryTestObjCRuntimeWrapper.m:68` + - Objective-C only _warns_ (never errors) on incomplete protocol conformance, so a conformer built before this PR compiles and links, then hits `doesNotRecognizeSelector:` when the new SDK invokes the selector. +- **Default-on trigger path.** The swizzling path that reaches this call is enabled by default: + - `Sources/Swift/Options.swift:257` (`enableAutoPerformanceTracing = true`), `Sources/Swift/Options.swift:280` (`enableUIViewControllerTracing = true`), `Sources/Swift/Options.swift:446` (`enableSwizzling = true`). + +#### Severity Note + +- P2 is correct — not higher. The finding's own scope check holds: the only realistic external consumers are hybrid SDKs, and none conform to this protocol. The method is also platform/arch-gated (`#if (os(iOS) || os(tvOS) || os(visionOS)) && (arch(arm64) || arch(x86_64))`), so watchOS/macOS/32-bit slices are unaffected regardless. Real latent ABI hazard, small blast radius. + +#### Wording Refinement + +- The risk is **Objective-C** conformers only. A _Swift_ conformer would fail to **compile** against the new required method, so it can never reach runtime. Read the finding's "compiled against the previous SDK" as Objective-C-only. + +#### Deferred Remediation (NOT YET IMPLEMENTED) + +- Smallest safe fix, ready to pick up later: + 1. Mark the protocol method `@objc optional func classes(forImage:)` in `Sources/Swift/Helper/SentryObjCRuntimeWrapper.swift:22` (protocol is already `@objc`). + 2. In `Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift:50`, call via optional chaining (`self.objcRuntimeWrapper.classes?(forImage: cImageName)`) and, when `nil`, log a warning and **skip** swizzling for that image. The fallback must **not** realize all classes — that is the exact GH-8152 crash this PR fixes. + 3. Add a regression test in `Tests/SentryTests/Integrations/Performance/SentrySubClassFinderTests.swift` that injects a conformer omitting `classesForImage:` and asserts `actOnSubclassesOfViewController` completes without raising and performs no swizzling. + 4. Run `make generate-public-api` and commit any diff to satisfy the `api-stability` CI check. No `SentryObjC`/`SentryObjCCompat` wrapper change is needed (SPI, not the public wrapper SDK surface). +- `SentryDefaultObjCRuntimeWrapper` and the test conformer already implement the method, so the normal SDK path is unchanged; only a foreign incomplete conformer takes the skip branch. + +## Validation Completed + +### Tests + +- Command: + +```bash +IOS_SIMULATOR_OS=18.6 IOS_DEVICE_NAME='iPhone 16 Pro' \ + make test-ios FOR_AGENTS=true \ + ONLY_TESTING=SentryTests/SentrySubClassFinderTests +``` + +- Result: `9` tests executed, `0` failures. + +- Command: + +```bash +IOS_SIMULATOR_OS=15.5 IOS_DEVICE_NAME='iPhone 13 Pro' \ + make test-ios FOR_AGENTS=true \ + ONLY_TESTING=SentryTests/SentrySubClassFinderTests +``` + +- Result: `9` tests executed, `0` failures. +- The iOS 15.5 run is important because the fix targets older runtime/availability behavior. + +### CI State Observed + +- Fast unit tests and fast framework slice checks were passing. +- Full CI jobs were gated by the missing `ready-to-merge` label because the PR was still a draft. +- Danger, lint, analysis, and security checks observed during review were passing. + +### Cross-Platform Build Attempt + +- A local tvOS build was attempted with Xcode 26.6 and the installed tvOS 26.5 simulator. +- It failed during Swift package binary-artifact resolution before compilation: + +```text +downloaded archive of binary target ... does not contain a binary artifact +xcodebuild: error: Could not resolve package dependencies +``` + +- This failure appears unrelated to the PR and does not provide tvOS compilation evidence. +- Raw diagnostics are in `raw-build-output.log` from the review session. + +### arm64e Evidence + +- The PR author tested an arm64e build on a physical iPhone 12 running iOS 26.5. +- The sample read and swizzled classes without pointer-authentication failures. +- This is useful evidence for PAC handling. +- It does not cover concurrent unload or Objective-C class remapping. + +## Suggested Implementation Sequence + +1. Design a loader-safe image lifetime strategy. +2. Stop carrying raw class references to main. +3. Resolve selected names on main and validate image identity. +4. Preserve compatibility for existing runtime-wrapper conformers. +5. Add unload and future-class regression tests. +6. Re-run iOS 15.5 and current iOS tests. +7. Build tvOS, visionOS, Catalyst, and watchOS slices. +8. Re-run format and analysis before merge. + +## Review Conclusion + +- The PR fixes the normal availability-crash path in the tested configurations. +- The current implementation replaces a runtime-managed, unload-aware enumeration path with direct parsing that lacks image lifetime protection and class remapping. +- Both gaps are runtime correctness issues, not theoretical style concerns. +- The unload crash was reproduced locally and the remapped-pointer mismatch was demonstrated locally. +- Address Findings 1 and 2 before marking the PR ready to merge. diff --git a/Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift b/Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift index 499f7dd358..7df0667ab3 100644 --- a/Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift +++ b/Sources/Swift/Core/Integrations/Performance/SentrySubClassFinder.swift @@ -40,64 +40,63 @@ class SentrySubClassFinder: NSObject { return } - var count: UInt32 = 0 - guard let classes = self.objcRuntimeWrapper.copyClassNamesForImage(cImageName, &count) else { - return - } + // Get the image's classes from its objc class list. This gives us the classes without + // realizing them. We must not use NSClassFromString to find the subclasses, because it + // realizes the class, and realizing a class whose Swift metadata references an + // `@available`-gated newer-framework type crashes on older OS versions + // (https://github.com/getsentry/sentry-cocoa/issues/8152, + // https://github.com/swiftlang/swift/issues/72657). Walking the superclass chain with + // `class_getSuperclass` below doesn't realize any class, so it can't trigger that crash. + let classes = self.objcRuntimeWrapper.classes(forImage: cImageName) + + SentrySDKLog.debug("Found \(classes.count) number of classes in image: \(imageName).") + + // We inspect the classes on this background thread but only with `class_getSuperclass` + // (in `isClass`) and `class_getName`. Neither sends an Objective-C message to the class, + // so neither triggers its `+initialize`, which only runs on the first message send. This + // matters because UIViewControllers assume they run on the main thread, so we must not + // trigger their `+initialize` here. Storing the unrealized class pointers doesn't message + // them either; the swizzling block on the main thread sends the first message, where + // that is safe. We must not round-trip through NSClassFromString on the main thread + // either: it realizes the class, and realizing a class whose Swift metadata references + // an `@available`-gated newer-framework type crashes on older OS versions + // (https://github.com/getsentry/sentry-cocoa/issues/8152, + // https://github.com/swiftlang/swift/issues/72657) — the same reason we read the class + // list from the image instead (see above). It could also resolve a same-named class + // from a different image. + var classesToSwizzle: [AnyClass] = [] + for cls in classes { + guard self.isClass(cls, subClassOf: viewControllerClass) else { + continue + } - SentrySDKLog.debug("Found \(count) number of classes in image: \(imageName).") - - // Storing the actual classes in an NSArray would call initializer of the class, which we - // must avoid as we are on a background thread here and dealing with UIViewControllers, - // which assume they are running on the main thread. Therefore, we store the class name - // instead so we can search for the subclasses on a background thread. We can't use - // NSObject:isSubclassOfClass as not all classes in the runtime in classes inherit from - // NSObject and a call to isSubclassOfClass would call the initializer of the class, which - // we can't allow because of the problem with UIViewControllers mentioned above. - // - // Turns out the approach to search all the view controllers inside the app binary image is - // fast and we don't need to include this restriction that will cause confusion. - // In a project with 1000 classes (a big project), it took only ~3ms to check all classes. - var classesToSwizzle: [String] = [] - - for i in 0.., _ outCount: UnsafeMutablePointer?) -> UnsafeMutablePointer>? { return objc_copyClassNamesForImage(image, outCount) } - + @_spi(Private) public func classGetImageName(_ cls: AnyClass) -> UnsafePointer? { return class_getImageName(cls) } + + // Call this off the main thread; the sole caller runs it on a background queue. (These `_dyld_*` + // calls take the dyld loader read lock that image load/unload contends, and the superclass walk + // over every class takes a few ms — neither belongs on the main thread.) + // + // KNOWN OPEN ISSUE (Finding 1 in REVIEW-PR-8457.md; see HANDOFF-subclassfinder-fix.md): iterating + // the dyld image list is NOT thread-safe. `` states: "Another thread can add or + // remove an image during the iteration." If the target image is unloaded between the name match + // and the `getsectiondata` dereference below, the header/section pointer dangles and + // dereferencing it can crash. This is a P0 that must be resolved before merge (fix direction: + // coordinate the read with the loader, e.g. via SentryBinaryImageCache). Not reachable in the + // default config (only the never-unloadable main executable is enumerated); reachable when the + // enumerated image is dynamically unloadable. + // + // Only supported on iOS, tvOS, and visionOS. It reads `mach_header_64` via `getsectiondata`, so + // we gate it to 64-bit architectures: every slice these platforms ship (`arm64`/`arm64e` + // devices, `arm64`/`x86_64` simulators), excluding the 32-bit watchOS device slices + // (`arm64_32`, `armv7k`) where the 64-bit header layout doesn't apply. +#if (os(iOS) || os(tvOS) || os(visionOS)) && (arch(arm64) || arch(x86_64)) + @_spi(Private) + public func classes(forImage image: UnsafePointer) -> [AnyClass] { + // We read `_dyld_image_count` once instead of per iteration (unlike the CxaThrowSwapper): we + // search for one already-loaded image, so images added while iterating aren't our target. An + // index that becomes out of range after an unload returns nil below, but an in-range index + // whose image was unmapped yields a dangling header — see the KNOWN OPEN ISSUE above. + for index in 0..<_dyld_image_count() { + guard let cName = _dyld_get_image_name(index), strcmp(cName, image) == 0 else { + continue + } + guard let header = _dyld_get_image_header(index) else { + SentrySDKLog.warning("No header for image: \(String(cString: image)). Skipping class list.") + return [] + } + + // Only treat the header as a `mach_header_64` if it actually is one. dyld hands us thin, + // native-arch, in-memory slices, so this holds in practice; the check is defensive. It + // also rejects a FAT header (`FAT_MAGIC`), which would otherwise be misread, so we skip + // such an image instead of going off into the weeds. `magic` is the first field of both + // `mach_header` and `mach_header_64`, so we can read it before rebinding. + guard header.pointee.magic == MH_MAGIC_64 else { + SentrySDKLog.warning("Header for image: \(String(cString: image)) isn't a mach_header_64. Skipping class list.") + return [] + } + + // Read the class pointers from the image's `__objc_classlist` section (`__DATA_CONST` on + // modern binaries, `__DATA` on older ones). dyld binds these pointers at load time, but + // the classes aren't realized, so callers can inspect them without running class + // initialization, unlike `NSClassFromString`. + // + // These are the raw, compiler-emitted pointers; we do NOT run objc4's `remapClass` over + // them, so for a class objc4 remaps (a resolved future class from `objc_getFutureClass`, + // or a weak-linked class with a missing superclass that it maps to nil) the entry here can + // differ from the live runtime class or be a disavowed struct. + // + // KNOWN OPEN ISSUE (Finding 2 in REVIEW-PR-8457.md; see HANDOFF-subclassfinder-fix.md): + // an Objective-C future class can have a view-controller superclass, pass + // `SentrySubClassFinder`'s `class_getSuperclass` filter, and reach the swizzler as a raw + // pointer that differs from the live class — bypassing `SentrySwizzle`'s class-identity + // dedup. objc4 only forbids a future class from being *completed by a Swift class*, not + // from having an ObjC view-controller superclass, so this is not merely theoretical. + // This must be resolved before merge, and the fix must not reintroduce the GH-8152 + // realization crash: re-resolving by name via `NSClassFromString` realizes the class and + // can pick a same-named class from another image, which is exactly what this change removed. + var size: UInt = 0 + let section = header.withMemoryRebound(to: mach_header_64.self, capacity: 1) { header in + getsectiondata(header, "__DATA_CONST", "__objc_classlist", &size) + ?? getsectiondata(header, "__DATA", "__objc_classlist", &size) + } + guard let section else { + SentrySDKLog.debug("No __objc_classlist section for image: \(String(cString: image)).") + return [] + } + + return Self.classes(inSection: section, size: size) + } + + SentrySDKLog.debug("Image not found in loaded images: \(String(cString: image)).") + return [] + } + + // Internal so tests can exercise the null-entry edge case, which a real dyld-loaded + // `__objc_classlist` section can't be made to contain. + static func classes(inSection section: UnsafeMutablePointer, size: UInt) -> [AnyClass] { + // The section holds ObjC `Class _Nullable` pointers, so `AnyClass?` is their honest + // Swift type: reading through it needs no unsafeBitCast, and a null entry (malformed + // section) becomes `nil` and is skipped instead of producing an invalid `AnyClass`. + let count = Int(size) / MemoryLayout.stride + return section.withMemoryRebound(to: AnyClass?.self, capacity: count) { classes in + UnsafeBufferPointer(start: classes, count: count).compactMap { $0 } + } + } +#endif } // swiftlint:enable missing_docs diff --git a/Sources/Swift/Helper/SentryObjCRuntimeWrapper.swift b/Sources/Swift/Helper/SentryObjCRuntimeWrapper.swift index 89b8e19606..f5279ffe21 100644 --- a/Sources/Swift/Helper/SentryObjCRuntimeWrapper.swift +++ b/Sources/Swift/Helper/SentryObjCRuntimeWrapper.swift @@ -7,5 +7,20 @@ public protocol SentryObjCRuntimeWrapper { func copyClassNamesForImage(_ image: UnsafePointer, _ outCount: UnsafeMutablePointer?) -> UnsafeMutablePointer>? @objc(class_getImageName:) func classGetImageName(_ cls: AnyClass) -> UnsafePointer? + /// IMPORTANT: Call this off the main thread. `_dyld_get_image_header` and `_dyld_get_image_name` + /// acquire the dyld loader read lock that every image load/unload contends, so calling it on the + /// main thread risks blocking it. + /// + /// Returns the classes defined in the given image by reading its `__objc_classlist` section. + /// The classes are not realized, so callers can inspect them (e.g. walk their superclass chain) + /// without triggering class initialization. + /// + /// Only supported on iOS, tvOS, and visionOS. It relies on `getsectiondata` with `mach_header_64`, + /// so it's gated to 64-bit architectures and excludes the 32-bit watchOS device slices + /// (`arm64_32`, `armv7k`). +#if (os(iOS) || os(tvOS) || os(visionOS)) && (arch(arm64) || arch(x86_64)) + @objc(classesForImage:) + func classes(forImage image: UnsafePointer) -> [AnyClass] +#endif } // swiftlint:enable missing_docs diff --git a/Tests/SentryTests/Helper/SentryTestObjCRuntimeWrapper.h b/Tests/SentryTests/Helper/SentryTestObjCRuntimeWrapper.h index 3777771334..80f45eb78c 100644 --- a/Tests/SentryTests/Helper/SentryTestObjCRuntimeWrapper.h +++ b/Tests/SentryTests/Helper/SentryTestObjCRuntimeWrapper.h @@ -15,6 +15,12 @@ @property (nullable, nonatomic, copy) NSArray *_Nullable (^classesNames) (NSArray *_Nullable); +/** + * Overrides the classes returned for an image. Receives the classes the real wrapper found and + * returns the classes to use. When @c nil, the real implementation is used unchanged. + */ +@property (nullable, nonatomic, copy) NSArray *_Nonnull (^classes)(NSArray *_Nonnull); + @property (nullable, nonatomic) const char *imageName; @end diff --git a/Tests/SentryTests/Helper/SentryTestObjCRuntimeWrapper.m b/Tests/SentryTests/Helper/SentryTestObjCRuntimeWrapper.m index 72e24d1857..a6916b1706 100644 --- a/Tests/SentryTests/Helper/SentryTestObjCRuntimeWrapper.m +++ b/Tests/SentryTests/Helper/SentryTestObjCRuntimeWrapper.m @@ -64,4 +64,17 @@ - (const char *)class_getImageName:(Class)cls return self.imageName; } +#if TARGET_OS_IOS || TARGET_OS_TV || TARGET_OS_VISION +- (NSArray *)classesForImage:(const char *)image +{ + NSArray *result = [self.objcRuntimeWrapper classesForImage:image]; + + if (self.classes != nil) { + result = self.classes(result); + } + + return result; +} +#endif + @end diff --git a/Tests/SentryTests/Integrations/Performance/SentrySubClassFinderTests.swift b/Tests/SentryTests/Integrations/Performance/SentrySubClassFinderTests.swift index d9e1a7af19..596689e5ba 100644 --- a/Tests/SentryTests/Integrations/Performance/SentrySubClassFinderTests.swift +++ b/Tests/SentryTests/Integrations/Performance/SentrySubClassFinderTests.swift @@ -1,5 +1,6 @@ @_spi(Private) import SentryTestUtils -@testable import Sentry +@_spi(Private) @testable import Sentry +import MachO import ObjectiveC import XCTest @@ -9,18 +10,18 @@ class SentrySubClassFinderTests: XCTestCase { private class Fixture { lazy var runtimeWrapper: SentryTestObjCRuntimeWrapper = { let result = SentryTestObjCRuntimeWrapper() - result.classesNames = { _ in - return self.testClassesNames + result.classes = { _ in + return self.testClasses } return result }() let imageName: String let dispatchQueue = TestSentryDispatchQueueWrapper() - let testClassesNames = [NSStringFromClass(FirstViewController.self), - NSStringFromClass(SecondViewController.self), - NSStringFromClass(ViewControllerNumberThree.self), - NSStringFromClass(VCAnyNaming.self), - NSStringFromClass(FakeViewController.self)] + let testClasses: [AnyClass] = [FirstViewController.self, + SecondViewController.self, + ViewControllerNumberThree.self, + VCAnyNaming.self, + FakeViewController.self] init() { if let name = class_getImageName(FirstViewController.self) { imageName = String(cString: name, encoding: .utf8) ?? "" @@ -28,35 +29,35 @@ class SentrySubClassFinderTests: XCTestCase { imageName = "" } } - + func getSut(swizzleClassNameExcludes: Set = []) -> SentrySubClassFinder { return SentrySubClassFinder(dispatchQueue: dispatchQueue, objcRuntimeWrapper: runtimeWrapper, swizzleClassNameExcludes: swizzleClassNameExcludes) } } - + private var fixture: Fixture! - + override func setUp() { super.setUp() fixture = Fixture() } - + func testActOnSubclassesOfViewController() { assertActOnSubclassesOfViewController(expected: [FirstViewController.self, SecondViewController.self, ViewControllerNumberThree.self, VCAnyNaming.self]) } - + func testActOnSubclassesOfViewController_WithSwizzleClassNameExcludes() { assertActOnSubclassesOfViewController(expected: [SecondViewController.self, ViewControllerNumberThree.self], swizzleClassNameExcludes: ["FirstViewController", "VCAnyNaming"]) } - + func testActOnSubclassesOfViewController_NoViewController() { - fixture.runtimeWrapper.classesNames = { _ in [] } + fixture.runtimeWrapper.classes = { _ in [] } assertActOnSubclassesOfViewController(expected: []) } func testActOnSubclassesOfViewController_NoViewController_DoesNotDispatchToMainQueue() { // Arrange - fixture.runtimeWrapper.classesNames = { _ in [] } + fixture.runtimeWrapper.classes = { _ in [] } let sut = fixture.getSut() // Act @@ -67,22 +68,23 @@ class SentrySubClassFinderTests: XCTestCase { // Assert XCTAssertEqual(fixture.dispatchQueue.blockOnMainInvocations.count, 0) } - + func testActOnSubclassesOfViewController_IgnoreFakeViewController() { - fixture.runtimeWrapper.classesNames = { _ in [NSStringFromClass(FakeViewController.self)] } + fixture.runtimeWrapper.classes = { _ in [FakeViewController.self] } assertActOnSubclassesOfViewController(expected: []) } - + func testActOnSubclassesOfViewController_WrongImage_NoViewController() { - fixture.runtimeWrapper.classesNames = nil + fixture.runtimeWrapper.classes = nil assertActOnSubclassesOfViewController(expected: [], imageName: "OtherImage") } - + func testGettingSubclasses_DoesNotCallInitializer() throws { let trackedClassName = try XCTUnwrap( SentryInitializeForGettingSubclassesCalled.registerDynamicClass()) - fixture.runtimeWrapper.classesNames = { _ in - return self.fixture.testClassesNames + [trackedClassName] + let trackedClass: AnyClass = try XCTUnwrap(NSClassFromString(trackedClassName)) + fixture.runtimeWrapper.classes = { _ in + return self.fixture.testClasses + [trackedClass] } assertActOnSubclassesOfViewController( @@ -95,7 +97,131 @@ class SentrySubClassFinderTests: XCTestCase { XCTAssertFalse(SentryInitializeForGettingSubclassesCalled.wasCalled()) } + + /// Differential test proving the one thing the fix changed — how classes are enumerated — is + /// equivalent to the old way, across every image loaded in the test process (thousands of real + /// classes from every linked framework), on whatever platform and architecture CI runs. + /// + /// The fix replaced `objc_copyClassNamesForImage` (which returns names) with reading the + /// `__objc_classlist` section (which returns class pointers). The subsequent subclass check + /// (`class_getSuperclass`) is unchanged. So the only new behavior to guard is that the class set + /// from `__objc_classlist` matches the class set from `objc_copyClassNamesForImage`. + /// + /// This test deliberately does NOT call `NSClassFromString`: that realizes classes, which is the + /// exact operation that crashes for availability-gated classes on older OS versions (GH-8152). + /// Comparing the two enumerations by name never realizes anything, so the test is safe on every + /// OS — including ones where the old path would have crashed. + func testClassesForImage_whenReadingEveryLoadedImage_shouldMatchCopyClassNamesForImage() throws { + // -- Arrange -- + let sut = SentryDefaultObjCRuntimeWrapper() + var comparedImages = 0 + + for index in 0..<_dyld_image_count() { + let imageName = try XCTUnwrap(_dyld_get_image_name(index).map { String(cString: $0) }) + + // -- Act -- + // New enumeration: class pointers from __objc_classlist, named via class_getName. + let fromClassList = Set(sut.classes(forImage: imageName).map { NSStringFromClass($0) }) + + // Old enumeration: names from objc_copyClassNamesForImage. + var fromCopyNames: Set = [] + imageName.withCString { cImageName in + var count: UInt32 = 0 + guard let names = objc_copyClassNamesForImage(cImageName, &count) else { return } + defer { free(UnsafeMutableRawPointer(mutating: names)) } + for i in 0.. = []) { assertActOnSubclassesOfViewController(expected: expected, imageName: fixture.imageName, swizzleClassNameExcludes: swizzleClassNameExcludes) } @@ -134,4 +260,10 @@ class SecondViewController: UIViewController {} class ViewControllerNumberThree: UIViewController {} class VCAnyNaming: UIViewController {} class FakeViewController {} + +/// An `@available`-gated `NSObject` subclass, standing in for the real-world classes that crashed +/// GH-8152 (SwiftUI gesture coordinators, `RoomPlan`/`ActivityKit` wrappers). It is compiled into +/// the test image's `__objc_classlist` so the enumeration must encounter it without realizing it. +@available(iOS 17.0, tvOS 17.0, *) +class AvailabilityGatedNonViewController: NSObject {} #endif