Skip to content

Fix UB: replace unsafe String::from_utf8_unchecked#7781

Merged
Manishearth merged 1 commit intounicode-org:mainfrom
sayrer:fix-transliterator-utf8-safety
Mar 16, 2026
Merged

Fix UB: replace unsafe String::from_utf8_unchecked#7781
Manishearth merged 1 commit intounicode-org:mainfrom
sayrer:fix-transliterator-utf8-safety

Conversation

@sayrer
Copy link
Contributor

@sayrer sayrer commented Mar 16, 2026

with checked conversion

A panic in a CustomTransliterator can unwind through Insertable's Drop impl without fully restoring UTF-8 validity, causing into_string() to produce undefined behavior via from_utf8_unchecked. Use checked from_utf8().expect() so this becomes a clean panic instead.

This one seemed correct to me (process: hammer on Claude until it gets a good patch). Maybe there is a better way, but I think this one is ok.

Changelog

icu_experimental/transliterator: Fix UB caused in partially-written unwind states by using checked UTF-8 conversion instead of from_utf8_unchecked

@sayrer sayrer requested a review from a team as a code owner March 16, 2026 20:38
…rsion

A panic in a CustomTransliterator can unwind through Insertable's Drop
impl without fully restoring UTF-8 validity, causing into_string() to
produce undefined behavior via from_utf8_unchecked. Use checked
from_utf8().expect() so this becomes a clean panic instead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sayrer sayrer force-pushed the fix-transliterator-utf8-safety branch from a704cdd to e2cac80 Compare March 16, 2026 20:45
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Good call.

In general for ICU4X I would prefer us to be much more careful about unchecked indexing, it's really not worth it in most cases.

@Manishearth Manishearth merged commit 00bcbe0 into unicode-org:main Mar 16, 2026
34 of 35 checks passed
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.

2 participants