Skip to content

Fix overflow panic in skipping_iterator_t with explicit saturating_add#397

Merged
behdad merged 1 commit into
harfbuzz:mainfrom
drott:addOverflowGsubGpos
Jun 26, 2026
Merged

Fix overflow panic in skipping_iterator_t with explicit saturating_add#397
behdad merged 1 commit into
harfbuzz:mainfrom
drott:addOverflowGsubGpos

Conversation

@drott

@drott drott commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

When shaping extremely long lines with fonts that have classic kern or
AAT kerx tables, the shaper instantiates a single skipping_iterator_t
once outside the loop and reuses it across the entire buffer. Because
self.glyph_data accumulates matching sequence indices without being reset,
it can eventually exceed u16::MAX (65,535) and cause an addition
overflow panic.

This fix upgrades self.glyph_data to a u32, and propagates the type
upgrade to match_func.

Assisted-by: Gemini

@drott drott force-pushed the addOverflowGsubGpos branch from 07216fc to 0220f2a Compare June 24, 2026 15:18
@drott drott requested a review from dfrg June 24, 2026 15:19
@drott

drott commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

Not 100% sure if this is the right approach, but does help against issues with encountering overflows with AAT font shaping.

@behdad

behdad commented Jun 24, 2026

Copy link
Copy Markdown
Member

If I understand the code correctly, this should simply be a uint and no saturation needed. In fact, the fact that it's 16bit is one of those beefs I have with this codebase baking a lot of gid16 assumptions around.

@drott

drott commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

I'd prefer to avoid machine-sized types like std::os::raw::c_uint and instead have a specific size, but do you mean u64 then or u32 as I did in this change?

@behdad

behdad commented Jun 25, 2026

Copy link
Copy Markdown
Member

u32 is fine, but I prefer to change themay_match and other signatures to pass down u32 as well, all the way. Should need no saturation.

@dfrg

dfrg commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

u32 is fine, but I prefer to change themay_match and other signatures to pass down u32 as well, all the way. Should need no saturation.

+1. Use of unnecessarily narrow types is endemic in this codebase and choosing promotion rather than truncation has been my strategy when I encounter these.

@drott drott force-pushed the addOverflowGsubGpos branch from 0220f2a to 10bb9c5 Compare June 26, 2026 08:52
When shaping extremely long lines with fonts that have classic `kern` or
AAT `kerx` tables, the shaper instantiates a single `skipping_iterator_t`
once outside the loop and reuses it across the entire buffer. Because
`self.glyph_data` accumulates matching sequence indices without being reset,
it can eventually exceed `u16::MAX` (65,535) and cause an addition
overflow panic.

This fix upgrades `self.glyph_data` to a `u32`, and propagates the type
upgrade to match_func.

Assisted-by: Gemini
@drott drott force-pushed the addOverflowGsubGpos branch from 10bb9c5 to c262b3f Compare June 26, 2026 08:59
@drott

drott commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Upgraded to u32 and propagating this to match_func PTAL.

@dfrg dfrg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thanks!

@behdad behdad merged commit 6e85d3f into harfbuzz:main Jun 26, 2026
3 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.

3 participants