Validate range in NSMutableAttributedString.add(link:for:color:) - #1446
Open
nortfiz wants to merge 1 commit into
Open
Validate range in NSMutableAttributedString.add(link:for:color:)#1446nortfiz wants to merge 1 commit into
nortfiz wants to merge 1 commit into
Conversation
The range argument was handed to addAttribute with no bounds check. A range with non-zero length running past the end of the string raises NSRangeException, and since message layout runs on messagesViewQueue there is no handler above it, so the process is terminated. Reject NSNotFound and negative values before anything else, since trimRange only clamps from above and location + length would trap on Int overflow. Then clamp with trimRange and skip empty ranges.
Author
|
Full write-up with the disassembly and the range table: https://nortfiz.github.io/telegram-macos-crash/ |
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.
Opening one specific chat kills the app immediately, every time. I collected 16 identical crash reports in 22 minutes. The client restores the last chat on launch, so it crashes again on every relaunch and there's no way out from the UI.
add(link:for:color:)inpackages/TGUIKit/Sources/Extensions.swiftpasses itsrangestraight toaddAttributewith no validation:A range with non-zero length running past the end of the string raises
NSRangeException. Message layout runs onmessagesViewQueue, nothing catches it there, andNSApplicationUncaughtExceptionHandlertakes the process down.Crash is on Telegram 12.9 (282526), macOS 26.4, arm64:
The App Store build is stripped, so I identified
<frame A>by disassembly. The report's arm64 UUID matches the shipping binary, so the offsets are usable directly.<frame A>is reached by a directblfrom<frame B>with no Foundation frame between them, which rules out anything invoked through a Foundation block. Its body puts the receiver inx20(swiftself), loads arg 1 as an existential and bridges it with_bridgeAnythingToObjectiveC, moves args 2 and 3 intox4/x5with no arithmetic, and loads a second attribute key immediately after the recorded return address. That isadd(link:for:color:).Behaviour of
addAttributeagainst an 11-character string:{0,5}{3,0}{11,0}{100,0}{NSNotFound,0}{100,3}{8,50}{-4,6}{NSNotFound,5}NSMutableRLEArray objectAtIndex:effectiveRange:: Out of boundsTwo things follow. Empty ranges are harmless at any location, so the existing
if range.location != NSNotFoundguards around this helper aren't the protection that's missing:range(of:)returns{NSNotFound, 0}, which never throws. And the offending range must have non-zero length.The helper has 61 call sites. 20 of them pass a range with no bounds check at all, several on the message layout path (
ChatRowItem,ChatServiceItem,ChatContactRowItem).The fix clamps inside the helper with
trimRangefrom the same file. TheNSNotFoundand negative checks have to come first:trimRangeonly clamps from above, and computinglocation + lengthon{NSNotFound, n}traps onIntoverflow before any range logic runs. Valid ranges pass through unchanged, and the worst case becomes a link that loses its styling.I could not narrow down which of the 20 call sites produces the bad range. The shipping build is stripped and the crash report doesn't reach past the helper. If it'd be useful I can add an
assertionFailureon the rejected path so debug builds surface the caller instead of silently skipping.Related: #919, same queue and same Foundation frame, open since January 2023.