Skip to content

Fix buffer overread setting line-break text in LgLineBreaker - #1057

Merged
jasonleenaylor merged 1 commit into
mainfrom
claude/verify-scenario-long-prose-ci-956666
Aug 12, 2026
Merged

Fix buffer overread setting line-break text in LgLineBreaker#1057
jasonleenaylor merged 1 commit into
mainfrom
claude/verify-scenario-long-prose-ci-956666

Conversation

@jasonleenaylor

@jasonleenaylor jasonleenaylor commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Quick Summary

  • Fixes the intermittent VerifyScenario("long-prose") / RunBenchmark("long-prose") CI failure — a COMException: Access violation from VwRootBox.Layout that hit unrelated PRs roughly weekly (five identical failures Jul 16–Aug 10; example run).
  • LgLineBreaker::put_LineBreakText now copies its text with the length-taking UnicodeString constructor instead of assigning the bare pointer, which scans an unterminated buffer for a null terminator.
  • Adds a native regression test covering the copy length and word-break lookup.

CI-ready checklist

  • Commit messages follow .github/commit-guidelines.md (subject ≤ 72 chars, no trailing punctuation; if body present, blank line then ≤ 80-char lines).
  • No whitespace warnings locally:
    git fetch origin
    git log --check --pretty=format:"---% h% s" origin/<base>..
    git diff --check --cached
  • Builds/tests pass locally (or I've run the CI-style build via Bash script or MSBuild).
  • If this is core-developer AI-assisted work, I followed Docs/workflows/ai-pr-workflow.md and ran pr-preflight or the equivalent branch-readiness review before requesting review.
  • For any Src/** folders touched, corresponding AGENTS.md files are updated or explicitly confirmed still accurate.

Notes for reviewers (optional)

Why it failed. UniscribeEngine::FindBreakPoint hands put_LineBreakText a text buffer that carries no null terminator. Storing that text by assigning the bare pointer made ICU scan past the end of the buffer looking for a terminator; the read only faults when the memory after the buffer happens to be unmapped, which is what made the crash rare and heap-layout dependent. It surfaced only in the long-prose scenario because paragraphs under 1000 characters go through a stack buffer (where the overread never faults), and that scenario has by far the largest heap-allocated paragraph text. The overread shipped with the ICU 70 upgrade (6592f1e, Feb 2022), so real projects with paragraphs over ~1000 characters have been exposed to it since.

Verification. Made the crash deterministic with full PageHeap: the scan faulted at exactly text[cch] inside icuuc70!UnicodeString::doAppend on every long-prose layout before the fix, and not at all after. All 30 render baseline tests pass unchanged (the fix does not alter break positions), and the new native TestLgLineBreaker test passes.

Related history. The STA attribute added by #919 for these same AVs remains correct and should stay: Views COM objects are apartment-threaded, so the fixtures need [Apartment(ApartmentState.STA)] regardless — but the AVs it chased were most likely this overread.

🤖 Generated with Claude Code


This change is Reviewable

* Copy the text with the length-taking UnicodeString constructor instead
  of assigning the bare pointer, which scans an unterminated buffer for
  a null terminator.
* Adds a native regression test covering the copy length and
  word-break lookup.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   11m 0s ⏱️ -31s
5 761 tests ±0  5 680 ✅ ±0  81 💤 ±0  0 ❌ ±0 
5 770 runs  ±0  5 689 ✅ ±0  81 💤 ±0  0 ❌ ±0 

Results for commit e23a552. ± Comparison against base commit b857373.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 38.05%. Comparing base (3c9a28a) to head (e23a552).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1057      +/-   ##
==========================================
+ Coverage   38.02%   38.05%   +0.02%     
==========================================
  Files        1499     1499              
  Lines      349910   350117     +207     
  Branches    40226    40233       +7     
==========================================
+ Hits       133055   133231     +176     
- Misses     187577   187603      +26     
- Partials    29278    29283       +5     
Files with missing lines Coverage Δ
Src/views/lib/LgLineBreaker.cpp 44.69% <100.00%> (+6.43%) ⬆️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thejambi thejambi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@thejambi reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on jasonleenaylor).

@jasonleenaylor
jasonleenaylor merged commit 1674067 into main Aug 12, 2026
7 checks passed
@jasonleenaylor
jasonleenaylor deleted the claude/verify-scenario-long-prose-ci-956666 branch August 12, 2026 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants