fix(editor): resolve the ranges input services send against the document before using them - #2351
Merged
Merged
Conversation
…ent before using them
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2339.
The problem
TextView'sNSTextInputClientconformance treated a caller-suppliedNSRangeas trusted. The callers are macOS system services, and they are documented to send ranges that fall outside the document: the doc comment sitting directly above the method that crashes is Apple's own text, and it says an implementation "should be prepared for aRange to be out of bounds", should return the intersection, and should return nil when the location is completely outside. Ghostty ships a comment on the same method saying "a lot of macOS system behaviors request bogus ranges", naming Look Up and QuickLook, so an IME is not even needed to reach this.The write half is worse than the read half, and it is not an Objective-C exception at all:
attributedSubstring(forProposedRange:actualRange:)andfirstRect(forCharacterRange:actualRange:)passed the raw range torangeOfComposedCharacterSequences(for:)andattributedSubstring(from:). Measured on macOS 27:NSInvalidArgumentException("The index N is invalid") andNSRangeException. Whether that kills the app depends on which dispatch path the callback arrived on. Measured: an exception undersendEvent, aTimer, or aCFRunLoopSource0callout is swallowed; one inside a main-queue block is fatal; andNSApplicationCrashOnExceptions=YES, which any crash-reporting SDK sets, makes it always fatal. Even swallowed it unwinds AppKit's input-manager frames mid-operation and skips Swift deinits, so the input session is left inconsistent.insertText(_:replacementRange:)andsetMarkedText(_:selectedRange:replacementRange:)forwarded the range intoreplaceCharacters(in:with:), which registers undo before it validates anything. Building the inverse mutation slices the storage, and TextStory answers an out-of-range slice withfatalError("Range invalid for string"). Measured:insertText("X", replacementRange: NSRange(location: 4, length: 99))on a five-character document exits with signal 5. No exception handler can reach that one and no dispatch path swallows it.unmarkText()and the marked-text branch ofinsertTextfeed stored ranges back in, andMarkedTextManagerhas no hook for edits made through any other path, so its ranges outlive the text they were computed against.The fix
The clamp sits at the
NSTextInputClientboundary, which is where the untrusted values enter, andreplaceCharactersstays strict for first-party callers such as Vim and the inline suggestions. Every peer engine that implements this protocol withoutNSTextViewdoes the same: STTextView, Chromium, WebKit, Zed and Ghostty all resolve the range against the document before touching storage.NSRange.resolved(inDocumentOfLength:)composes the package's existingclamped(toLength:)and returns nil for the cases a clamp cannot express:NSNotFound, a negative location or length, and a location plus length that overflows. That last one also protectsclamped(toLength:)itself, whosemaxtraps on those inputs.attributedSubstring(forProposedRange:actualRange:)presetsactualRangeto{NSNotFound, 0}, returns nil when the range names nothing in the document, and otherwise writes the resolved range and returns that substring.firstRect(forCharacterRange:actualRange:)resolves outside theactualRange != nilbranch. The raising call used to sit inside it, so the old code only crashed when the caller wanted the adjusted range back. A zero-length range still resolves, because that is how an input method asks where to put its candidate window.MarkedTextManagerresolves its own stored ranges before they are used. Resolving only what gets replaced would stop the crash and leave the bookkeeping stale, which corrupts the composition instead: measured, marking "n" into "Hello", replacing the document with "Hi" from elsewhere, then typing the composition gave "Hini" and then "Hininih" rather than "Hinih".firstRectfalls back to the end-of-document rect, converted through the window, for a range it cannot resolve. Returning a bareNSZeroRectwould have put the candidate window, the press-and-hold accent popover and the dictation indicator in the corner of the display, because that value is read as screen coordinates.attributedSubstringreturns nil only when the location itself is outside the document, which is the case the protocol documents. A caret position inside it answers with an empty string, so a client reading the caret's context can still tell "no characters selected" from "no text here".Verification
swift test --package-path LocalPackages/CodeEditTextView: 165 tests in 16 suites pass. Against the unfixed code the new suite dies withNSInvalidArgumentException: The index 3 is invalid, which is the reported failure.NSNotFound,Int.maxlength, empty document, grapheme rounding, caret positions), the caret rect, the NULLactualRangepath, both write methods, the multi-cursor collapse, and a composition that survives an edit made outside it.xcodebuild -scheme TablePro build: passes.TextView+NSTextInput.swiftare present at HEAD as well.No UI automation: reaching this needs a real input service to send a stale range, which no deterministic XCUITest can arrange.
Found while investigating, not fixed here
All four were reproduced with compiled probes. None is required for this fix to be correct.
unmarkText()deletes the composition instead of accepting it, contradicting the docstring three lines above it. Measured side by side againstNSTextView: on "alpha " with "ceshi" marked, AppKit keeps "alpha ceshi" and drops only the marking, while this view leaves "alpha ". Shift-click mid-composition, atTextView+Mouse.swift:52, throws away what the user typed.MarkedTextManager.updateForNewSelectionsreturns the inverse of what its own comment says, so collapsing to one cursor mid-composition leaves both marked ranges live and the commit writes two edits.CEUndoManager.registerMutationnever consultsisDisabled, so every IME keystroke becomes its own undo step and Cmd+Z walks back through romaji that was never committed.MarkedTextManagerstill has no edit-tracking hook of the kindselectionManager.didReplaceCharactersgives selections. This change resolves its ranges every time they are used, which is enough for the composition to stay correct, but the ranges still go stale in between.