fix(text): Harden UnicodeText malformed-text surrogate, indexing, and ICU bidi fallback#22697
Draft
agneszitte wants to merge 1 commit intomasterfrom
Draft
fix(text): Harden UnicodeText malformed-text surrogate, indexing, and ICU bidi fallback#22697agneszitte wants to merge 1 commit intomasterfrom
agneszitte wants to merge 1 commit intomasterfrom
Conversation
github-actions
bot
added
area/skia ✏️
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens Skia UnicodeText shaping/drawing to avoid crashes on malformed Unicode input and out-of-range highlighter/run scenarios, and adds coverage to ensure those cases don’t throw during layout/rendering.
Changes:
- Sanitizes malformed UTF-16 (unpaired surrogates) and adds an ICU BiDi setup fallback path.
- Adds multiple draw-time bounds guards around highlighter slices, run breaks, word boundaries, and line metric lookups.
- Adds unit tests and Skia runtime tests covering malformed text and overlong highlighter ranges.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Uno.UI/UI/Xaml/Documents/UnicodeText.skia.cs | Adds surrogate sanitization, BiDi fallback, and defensive bounds checks in Draw and word-boundary generation. |
| src/Uno.UI.Tests/Windows_UI_XAML_Controls/TextBlockTests/Given_TextBlock.cs | Adds unit tests for malformed surrogate text and out-of-range highlighter ranges. |
| src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_TextBlock.cs | Adds Skia-only rendering tests to ensure no crashes with unpaired surrogates (with/without highlighters). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Uno.UI.Tests/Windows_UI_XAML_Controls/TextBlockTests/Given_TextBlock.cs
Outdated
Show resolved
Hide resolved
agneszitte
force-pushed
the
dev/agzi/fix-unicodetext-skia-index-guards
branch
from
c31a471 to
772a80b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agneszitte
changed the title
Contributor
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22697/wasm-skia-net9/index.html |
Contributor
|
The build 197277 found UI Test snapshots differences: Details
|
Contributor
|
|
Contributor
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22697/wasm-skia-net9/index.html |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agneszitte
force-pushed
the
dev/agzi/fix-unicodetext-skia-index-guards
branch
from
3460940 to
5e06d71
Compare
agneszitte
changed the title
agneszitte
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agneszitte
force-pushed
the
dev/agzi/fix-unicodetext-skia-index-guards
branch
2 times, most recently
from
6f8b634 to
4e78893
Compare
- Add defensive draw-time bounds checks for highlighter/run-break/word-boundary and line metric indexing. - Sanitize malformed UTF-16 surrogate units to U+FFFD before shaping. - Retry ICU ubidi_setPara without embedding levels when explicit levels fail, with bidi handle cleanup. - Keep/update unit and Skia runtime regression coverage for malformed surrogate and highlighter-range scenarios.
agneszitte
force-pushed
the
dev/agzi/fix-unicodetext-skia-index-guards
branch
from
4e78893 to
131ed62
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1368
to
+1371
| var c = chars[index]; | ||
| if (char.IsHighSurrogate(c)) | ||
| { | ||
| if (index + 1 < chars.Length && char.IsLowSurrogate(chars[index + 1])) |
There was a problem hiding this comment.
The sanitization logic for valid surrogate pairs is incorrect. When a valid high-low surrogate pair is detected (line 1368), the code increments by 1 (line 1370) and continues. On the next loop iteration, index will point to the low surrogate, which will match the condition at line 1377 and be incorrectly replaced with U+FFFD.
The fix is to increment by 2 instead of 1 on line 1370, so the loop skips both characters in the valid pair: index += 2;
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
3 participants
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.
Related to https://github.com/unoplatform/kahua-private/issues/426
Summary
This PR hardens the Skia
UnicodeTextcrash path for malformed-text scenarios, focusing on draw-time indexing, surrogate handling, and ICU/bidi setup. The goal is fail-safe rendering (no runtime exceptions) for the reported repro family.Why
The repro hit
ArgumentOutOfRangeExceptioninUnicodeText.Drawwith malformed surrogate/highlighter combinations. While fixing that path, related edge cases in bidi setup and draw-time index access were hardened to prevent equivalent crash outcomes.What changed
U+FFFDbefore shaping while preserving inline boundary behavior.ubidi_setParafails with embedding levels, retry without embedding levels.ubidi_setParafails in ICU interop.Local Validation
dotnet build src/Uno.UI/Uno.UI.Tests.csproj -c Debug -f net9.0dotnet build src/Uno.UI.RuntimeTests/Uno.UI.RuntimeTests.Skia.csproj -c Debugdotnet test src/Uno.UI.Tests/Uno.UI.Tests.csproj -c Debug -f net9.0 --filter "FullyQualifiedName~When_Text_Has_Unpaired_Surrogate|FullyQualifiedName~When_Highlighter_Range_Exceeds_Text"UnicodeTextcrash signatures.