Skip to content
This repository was archived by the owner on Jun 11, 2026. It is now read-only.

Reject unsafe Core Foundation object hooks#46

Merged
steipete merged 1 commit into
masterfrom
fix/nsurl-object-hook
Jun 11, 2026
Merged

Reject unsafe Core Foundation object hooks#46
steipete merged 1 commit into
masterfrom
fix/nsurl-object-hook

Conversation

@steipete

Copy link
Copy Markdown
Owner

Summary

  • reject Core Foundation-backed objects before object-hook dynamic subclassing
  • apply object safety validation consistently to Interpose(object) and direct object.hook(...)
  • preserve repeated hooks on InterposeKit's own generated subclass
  • document the limitation and add an NSURL regression test

Root cause

Swift ARC releases Objective-C objects through swift_unknownObjectRelease. After object_setClass changes an NSURL instance to an InterposeKit-generated subclass, that release traps on current Apple runtimes. Core Foundation-backed Foundation objects therefore cannot safely use InterposeKit's per-object isa-swizzling path.

Verification

  • exact NSURL repro on macOS 26.5: both APIs now throw and the URL remains usable
  • swift test
  • swift test -c release
  • macOS Xcode tests with UTF16 and UTF8 plus code coverage
  • iOS 26.5 simulator Xcode tests
  • watchOS simulator build
  • bash Tests/verify_release_linking.sh
  • bash Tests/verify_class_load_watcher.sh
  • IKT_FORCE_DYLD_IMAGE_CALLBACK=1 bash Tests/verify_class_load_watcher.sh
  • $autoreview: clean

Closes #23

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 11, 2026, 6:58 AM ET / 10:58 UTC.

Summary
The PR adds a Core Foundation-backed object guard before per-object dynamic subclassing, applies it to both object-hook entry points, adds a new error case, updates docs/changelog, and adds an NSURL regression test.

Reproducibility: yes. there is a high-confidence source-level reproduction path: the linked report calls NSURL.host through object.hook, and current master reaches object_setClass without a Core Foundation guard. I did not execute the macOS runtime repro in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Files changed: 7 files, 64 additions, 11 deletions. The patch spans runtime validation, error reporting, docs, changelog, and regression coverage.
  • Object-hook entry points guarded: 2 paths. Both Interpose(object) and direct object.hook/ObjectHook construction now share the safety validation.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] The guard intentionally converts Core Foundation-backed object hooks from attempted dynamic subclassing into a thrown error; that is safer for the known crash, but callers that attempted these hooks must now handle the new failure path.

Maintainer options:

  1. Accept fail-fast safety behavior (recommended)
    Maintainers can merge the PR as a bug fix if rejecting unsafe Core Foundation-backed hooks is the intended compatibility tradeoff for avoiding the NSURL crash.
  2. Reclassify the release note
    If maintainers consider the new thrown error a public compatibility break, move or expand the release note before merge so users know to handle it during upgrades.

Next step before merge

  • Owner-authored safety PR should stay open for maintainer merge judgment and normal required checks; no automated repair is indicated.

Security
Cleared: The diff changes Swift runtime validation, docs, changelog, and tests only; I found no concrete security or supply-chain concern.

Review details

Best possible solution:

Merge this safety guard if maintainers accept the fail-fast compatibility change, preserving the README/changelog caveat and NSURL regression coverage.

Do we have a high-confidence way to reproduce the issue?

Yes, there is a high-confidence source-level reproduction path: the linked report calls NSURL.host through object.hook, and current master reaches object_setClass without a Core Foundation guard. I did not execute the macOS runtime repro in this read-only review.

Is this the best way to solve the issue?

Yes, the proposed approach is narrow: it rejects unsafe objects before dynamic subclassing and applies the same validation to both object-hook APIs. The main maintainer decision is whether the new fail-fast behavior is an acceptable compatibility tradeoff.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 20945a4b67cb.

Label changes

Label changes:

  • add P2: This is a focused crash-prevention bug fix for object hooking with limited API surface and a regression test.
  • add merge-risk: 🚨 compatibility: The PR adds a new thrown error for Core Foundation-backed object hooks, which can change upgrade behavior for callers that previously attempted those hooks.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The real-behavior-proof gate is not applicable because this is an owner-authored PR, though the PR body lists macOS, iOS, watchOS, SwiftPM, and script verification.

Label justifications:

  • P2: This is a focused crash-prevention bug fix for object hooking with limited API surface and a regression test.
  • merge-risk: 🚨 compatibility: The PR adds a new thrown error for Core Foundation-backed object hooks, which can change upgrade behavior for callers that previously attempted those hooks.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The real-behavior-proof gate is not applicable because this is an owner-authored PR, though the PR body lists macOS, iOS, watchOS, SwiftPM, and script verification.
Evidence reviewed

What I checked:

  • Owner-authored item: The supplied GitHub context identifies the PR author as steipete with author association OWNER, which makes this a keep-open item for explicit maintainer handling.
  • No target AGENTS.md: No AGENTS.md exists inside the InterposeKit git root; the parent workspace AGENTS.md was outside the target repository and was not applied as target policy. (20945a4b67cb)
  • Current unsafe object path: Current master initializes object interposing with KVO/posing checks only, before later object-hook preparation reaches the dynamic subclass path. (Sources/InterposeKit/InterposeKit.swift:71, 20945a4b67cb)
  • Current dynamic subclass mutation: The existing object-hook implementation uses object_setClass when creating the dynamic subclass, matching the root cause described for Core Foundation-backed objects. (Sources/InterposeKit/InterposeSubclass.swift:63, 20945a4b67cb)
  • PR safety guard: The proposed branch adds validateObjectForHooking with a Core Foundation-backed object check and throws coreFoundationObjectDetected before object hooks proceed. (Sources/InterposeKit/InterposeKit.swift:73, 7413a066a3d2)
  • Direct hook path covered: The proposed ObjectHook initializer calls the same validation before storing the object or building the replacement block. (Sources/InterposeKit/ObjectHook.swift:21, 7413a066a3d2)

Likely related people:

  • steipete: Git history shows Peter Steinberger introduced individual object hooks, extracted the InterposeSubclass path, recently maintained KVO/object-hook behavior, and authored the proposed fix branch. (role: introduced behavior and recent area contributor; confidence: high; commits: 9e387e1e8936, 4bcf98e02596, 510ab22f0fa8; files: Sources/InterposeKit/InterposeKit.swift, Sources/InterposeKit/ObjectHook.swift, Sources/InterposeKit/InterposeSubclass.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jun 11, 2026
@steipete steipete merged commit a7e1b57 into master Jun 11, 2026
7 checks passed
@steipete steipete deleted the fix/nsurl-object-hook branch June 11, 2026 11:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash when hooking NSURL

1 participant