fix: prevent out-of-bounds access on truncated UTF-8 strings and crafted index maps - #4617
Open
stweil wants to merge 3 commits into
Open
fix: prevent out-of-bounds access on truncated UTF-8 strings and crafted index maps#4617stweil wants to merge 3 commits into
stweil wants to merge 3 commits into
Conversation
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Pull request overview
Hardens index-map deserialization/accessors and UTF-8 scanning against malformed input.
Changes:
- Adds bounds validation for bidirectional index maps.
- Safely handles truncated UTF-8 sequences in training utilities.
- Adds regression tests for malformed inputs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/ccutil/indexmapbidi.cpp |
Validates deserialized indices and accessor inputs. |
src/ccutil/indexmapbidi.h |
Adds guarded index accessors. |
src/training/unicharset/normstrngs.cpp |
Bounds UTF-8 whitespace scans. |
src/training/pango/pango_font_info.cpp |
Bounds Pango UTF-8 iteration. |
unittest/indexmapbidi_test.cc |
Tests malformed maps and indices. |
unittest/normstrngs_test.cc |
Tests truncated UTF-8 prefixes. |
unittest/pango_font_info_test.cc |
Tests truncated UTF-8 handling in Pango. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
stweil
force-pushed
the
fix-oob-utf8-index-maps
branch
from
August 26, 2026 08:28
ebf125c to
e1e8d67
Compare
IndexMapBiDi::DeSerialize used the compact and sparse indices read from the file directly as subscripts of sparse_map_, so a crafted or corrupt .traineddata with an out-of-range value performed a heap out-of-bounds write during deserialization. DeSerialize now validates every index before use and rejects such data, including remaining_pairs with an odd element count, which previously read one element past the end of the vector. It also rejects files in which a sparse slot is claimed by more than one compact representative or remaining pair: such a file can encode a master cycle (for example sparse_map_ = [1, 0]) that would make MasterCompactIndex loop forever. Copilot's review of this PR suggested the duplicate-claim check and the empty base-map test case. The public index accessors had the same unchecked-subscript pattern: IndexMapBiDi::SparseToCompact and IndexMap::CompactToSparse read outside their vectors for out-of-range indices, Merge subscripted both maps with unchecked compact indices, MapFeatures read sparse_map_ with unchecked feature indices, and IsCompactDeleted could chase an out-of-range master index. They now return the not-mapped / not-merged / missed-feature result for out-of-range input instead of invoking undefined behavior. The Merge(-1, index) merge-away sentinel used by IntFeatureMap is still accepted. IndexMap::SparseToCompact no longer reads compact_map_ when the map is empty. Key changes: - indexmapbidi.cpp: validate compact_map_ entries and remaining_pairs in DeSerialize before subscripting sparse_map_, including duplicate sparse slot claims that could encode a master cycle; guard Merge and MapFeatures; reject an empty compact_map_ in IndexMap::SparseToCompact. - indexmapbidi.h: bounds-check IndexMap::CompactToSparse, IndexMapBiDi::SparseToCompact and IsCompactDeleted. - unittest: add DeSerializeRejectsBadIndices (crafted blobs with out-of-range, negative, odd-count, duplicate-claim and cyclic indices) and AccessorsRejectBadIndices, which also covers the empty-map guards in both SparseToCompact implementations, plus a many-to-one serialize/deserialize round trip as a positive control. On unpatched code the new tests die on ASan heap-buffer-overflow write in DeSerialize and reads in SparseToCompact. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Signed-off-by: Stefan Weil <sw@weilnetz.de>
SpanUTF8Whitespace and SpanUTF8NotWhitespace iterated with UNICHAR::const_iterator, which derives the character width from the leading byte alone. A multibyte sequence truncated at the end of the string (for example "ab\xE8") made *it read past the NUL terminator and ++it run past the end of the string: an out-of-bounds read that only stopped at a crash. Both functions now step the bytes manually: a truncated trailing sequence ends the whitespace span and is counted as non-whitespace bytes (never more than the bytes actually present), and an illegal leading byte keeps the previous iterator semantics (one whitespace byte, and a boundary for the non-whitespace span). Valid UTF-8 input is handled exactly as before. Key changes: - normstrngs.cpp: rewrite both span scans with explicit width checks against the remaining bytes. - unittest: add SpanUTF8TruncatedPrefix covering 2/3/4-byte truncated prefixes after leading spaces and illegal leading bytes. On unpatched code the test dies on an ASan global-buffer-overflow read in UNICHAR::utf8_step via const_iterator::operator*. Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Signed-off-by: Stefan Weil <sw@weilnetz.de>
CoversUTF8Text, DropUncoveredChars and GetSpacingProperties iterated with UNICHAR::const_iterator over byte ranges that may end in a truncated multibyte sequence. The iterator's *it and is_legal read the full width from the leading byte, so a truncated trailing prefix (for example "ab\xE8") read past the end of the string and ++it jumped past the end iterator: an out-of-bounds read that only stopped at a crash. The loops now step the bytes manually: an illegal leading byte keeps the iterator's space fallback, and a sequence truncated at the end of the string is skipped one byte at a time (dropped and counted by DropUncoveredChars, skipped by the other two) instead of read past the end. Valid UTF-8 input is handled exactly as before. Key changes: - pango_font_info.cpp: rewrite the three character loops with explicit width checks against the remaining bytes. - unittest: add HandlesTruncatedUtf8. On unpatched code the test dies on an ASan stack-buffer-overflow read in UNICHAR::utf8_step via is_legal() in DropUncoveredChars. Assisted-by: OpenCode / qwen3.8-27b-thinking (Alibaba Cloud) Signed-off-by: Stefan Weil <sw@weilnetz.de>
stweil
force-pushed
the
fix-oob-utf8-index-maps
branch
from
August 26, 2026 08:51
e1e8d67 to
b738944
Compare
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.
I created this PR and the included commits with OpenCode / qwen3.8-27b.
Background
Follow-up to #4609 (
fix: prevent out-of-bounds read in UNICHAR::UTF8ToUTF32)and #4614 (
fix(ccutil): guard IndexMapBiDi sparse indices). While reviewingthose PRs I found the same bug classes in a few more places; this PR fixes
the remaining instances:
1.
IndexMapBiDi::DeSerialize(src/ccutil/indexmapbidi.cpp)The compact and sparse indices read from the file were used directly as
subscripts of
sparse_map_. A crafted or corrupt.traineddatawithout-of-range values performs a heap out-of-bounds write during
deserialization — same class as #3584, but on the load path, reachable from
untrusted file input.
DeSerializenow validates every index before use andrejects such data, including
remaining_pairswith an odd element count,which previously read one element past the end of the vector.
2. Unchecked
IndexMapBiDiaccessors (src/ccutil/indexmapbidi.{h,cpp})IndexMapBiDi::SparseToCompact,IndexMap::CompactToSparse,Merge,MapFeaturesandIsCompactDeletedsubscripted their vectors without boundschecks (OOB reads, reachable from training data via
TrainingSampleSetandIntFeatureMap). They now return the not-mapped / not-merged /missed-feature result for out-of-range input. The
Merge(-1, x)merge-awaysentinel used by
IntFeatureMap::DeleteMapFeatureis still accepted, andIndexMap::SparseToCompactno longer readscompact_map_when the map isempty.
3. UTF-8 span scans (src/training/unicharset/normstrngs.cpp)
SpanUTF8Whitespace/SpanUTF8NotWhitespaceiterated withUNICHAR::const_iterator, which derives the character width from the leadingbyte alone. A multibyte sequence truncated at the end of the string (e.g.
"ab\xE8") made*itread past the NUL terminator and++itrun past theend of the string — an OOB read (issue #4495's class) that only stopped at a
crash. Both scans now step the bytes manually with explicit width checks.
4.
PangoFontInfocharacter loops (src/training/pango/pango_font_info.cpp)CoversUTF8Text,DropUncoveredCharsandGetSpacingPropertieshad thesame iterator pattern and the same OOB read / infinite loop on a truncated
trailing prefix. They now step the bytes manually.
For all of these, behavior on valid input is unchanged (including the
iterator's space fallback for illegal bytes); only previously-undefined
cases get defined behavior.
Test plan
ASan build (Debug,
-fsanitize=address, legacy + training + pango tests).The new regression tests crash on unpatched code and pass on the patched
code:
IndexMapBiDiTest.DeSerializeRejectsBadIndicesIndexMapBiDi::DeSerializeIndexMapBiDiTest.AccessorsRejectBadIndicesIndexMapBiDi::SparseToCompactNormstrngsTest.SpanUTF8TruncatedPrefixUNICHAR::utf8_stepviaconst_iterator::operator*PangoFontInfoTest.HandlesTruncatedUtf8UNICHAR::utf8_stepviais_legal()inDropUncoveredCharsDeSerializeRejectsBadIndicesfeeds crafted blobs (out-of-range, negativeand odd-count indices) and includes a many-to-one
serialize/deserialize round trip as a positive control.
Regression runs (all pass under ASan):
indexmapbidi_test,normstrngs_test,pango_font_info_test,intfeaturemap_test,stringrenderer_test,mastertrainer_test,ligature_table_test,unicharset_test,dawg_test.Out of scope
The same truncated-prefix pattern still appears in
LigatureTable::RemoveLigatures/RemoveCustomLigatures(src/training/pango/ligature_table.cpp:115,135) and in several
StringRendererloops (src/training/pango/stringrenderer.cpp:289,673,693,711,846);happy to address those in a follow-up.