fix: guard against NSNotFound selection-range overflow crashes (#319)#385
fix: guard against NSNotFound selection-range overflow crashes (#319)#385postoso wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4eacabeba3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var range = CFRange() | ||
| let ok = AXValueGetValue(unsafeBitCast(axValue, to: AXValue.self), .cfRange, &range) | ||
| return ok ? range : nil | ||
| return ok ? Self.sanitizedSelectedTextRange(range) : nil |
There was a problem hiding this comment.
When the Accessibility insertion path sees an AX element that returns {location: NSNotFound, length: 0} but still has an existing kAXValueAttribute, this sanitizer now makes insertTextAtCursorUsingSelectedRange return false; tryAllTextInsertionMethods then immediately falls through to setTextViaValue, which replaces the entire field value with the dictated text. Before this change, that same sentinel range was clamped to the end of the existing NSString, so existing contents were preserved. For macOS 26 apps exposing this sentinel during direct Accessibility insertion, dictation can overwrite the whole field instead of inserting.
Useful? React with 👍 / 👎.
|
If you can provide a screencapture of the crash, that would be really helpful, like a video clip or something. Because I'm not able to reproduce it, so I don't know if this fix is gonna cause more issues, if that makes sense. Other than that, I think it's a beautiful PR, would love to merge this, if you can prove me that it's a fix for a reproducible issue xD |
…-dev#319) macOS 26 AX APIs can report a selected-text range of {location: NSNotFound (Int.max), length: 0}. Feeding that raw location into caret/bounds arithmetic (before.location + expectedLength; range.location + range.length) overflows and traps (EXC_BREAKPOINT) ~5s after a successful clipboard insertion. - TypingService: sanitize getSelectedTextRange (reject NSNotFound/negative); extract overflow-safe caretMovedExpectedDistance helper (addingReportingOverflow). - TextSelectionService: NSNotFound-aware guard + overflow-safe boundedSelectionRange. - Add unit tests for the sanitizer, caret-distance, and bounds helpers.
…field overwrite Review follow-up to the altic-dev#319 NSNotFound selection-range fix. The shared getSelectedTextRange getter was made to reject the macOS 26 sentinel ({location: NSNotFound, length: 0}) by returning nil. That over-broad sanitization regressed the AX-direct insertion path: insertTextAtCursorUsingSelectedRange already clamped the sentinel location to the field length (inserting at the end, preserving contents), but a nil return made it bail to approach 1 (setTextViaValue), which REPLACES the entire field with the dictated text -> data loss on apps that expose the sentinel during direct AX insertion. - getSelectedTextRange: return the raw range again. Both callers handle the sentinel safely on their own (insertion clamps to end; the deferred verification path runs it through the overflow-safe caretMovedExpectedDistance). - Remove the now-unneeded sanitizedSelectedTextRange helper + its tests. - Factor the insertion clamp into a pure, testable clampedInsertionRange helper; add tests proving the sentinel clamps to {textLength, 0} (insert at end) rather than bailing. - Keep the genuine crash fix (overflow-safe caretMovedExpectedDistance) and TextSelectionService.boundedSelectionRange (correct there: extraction path has no graceful clamp, so rejecting the sentinel is the right behavior). The crash stays fixed: with the raw sentinel range, the verification path's caretMovedExpectedDistance(before: {Int.max, 0}, ...) overflows on before.location + expectedLength via addingReportingOverflow and returns false instead of trapping (altic-dev#319).
postoso
force-pushed
the
fix/319-typingservice-overflow
branch
from
be3e13f to
cb1eed7
Compare
|
Hey @altic-dev, fair ask. This PR fixes #319, which @domci reported with a full crash signature ( Here's what happens. macOS AX sometimes hands back a selected-text range of The 10 unit tests feed that exact @domci, you've got the setup that reproduces it. Any chance you could drop the crash log or a quick clip on #319? That'd give @altic-dev the end-to-end proof. Rebased onto 1.6.0; builds clean, swiftlint --strict clean, and all 10 unit tests pass against current main. |
hi @postoso i sure would love to. but im on Version 1.6.0 (12) now and the bug seems gone. I cant reproduce anymore. |
|
Thanks for checking, @domci, glad it's not hitting you on 1.6.0(12) anymore. To be straight about what this is now: defense-in-depth, not a fix for a live repro. The NSNotFound / Int.max range still feeds the caret math on the clipboard-fallback path, and the 10 unit tests prove that arithmetic traps before this change and stays safe after, so the guard holds whether or not the crash currently reproduces, and the happy path is untouched. @altic-dev, your call on whether that's worth keeping. Glad to leave it in as a latent-overflow guard, or close it if you'd rather not carry a fix for something that's not currently reproducible. Either works for me. |
3 participants
What
Fixes a reproducible crash (EXC_BREAKPOINT / Swift integer-overflow trap) ~5 s after a successful clipboard-mode text insertion ("one good dictation per launch, then a crash").
Root cause
macOS 26 AX APIs can report a selected-text range of
{location: NSNotFound (Int.max), length: 0}. Feeding that raw location into caret/bounds arithmetic (before.location + expectedLength;range.location + range.length) overflows and traps.Fix (defense-in-depth)
TypingService.getSelectedTextRangesanitizes the range (rejectsNSNotFound/negative).addingReportingOverflow/.magnitude), preserving the original tolerance semantics.TextSelectionServiceguard now catchesNSNotFound(not justkCFNotFound == -1), and its bounds validation is overflow-safe viaboundedSelectionRange.Verification
swiftlint --strictclean;xcodebuild buildsucceeds; the 10 unit tests pass.Closes #319