Skip to content

Conversation

@zeonzip
Copy link
Contributor

@zeonzip zeonzip commented Jul 24, 2025

Adds buffer limit from #48

This PR adds the buffer limit to 10000 during multiplesubst in the GSUB table as asked for in #48.

I'll add a test once I know I applied the correct fix.
If this wasn't the expected fix, please tell me and I will gladly update it.

Copy link
Contributor

@wezm wezm left a comment

Choose a reason for hiding this comment

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

Nice. I expected this to require more changes, but it seems good.

I'll add a test once I know I applied the correct fix.

Note that there is a font in the unicode text-rendering-tests that can be used for testing.

@mhosken
Copy link

mhosken commented Jul 25, 2025

As per the bug report, I strongly suggest making this a multiple of the input buffer size. There are scripts without spaces and there are fonts with contextual spaces both which can result in buffers of a paragraph of text being processed. Harfbuzz uses a factor of 64 for its max (up to a hard limit of 1 billion glyphs). This is reasonable since it is only about spotting runaway growth.

@zeonzip
Copy link
Contributor Author

zeonzip commented Jul 25, 2025

Nice. I expected this to require more changes, but it seems good.

I'll add a test once I know I applied the correct fix.

Note that there is a font in the unicode text-rendering-tests that can be used for testing.

Just mentioning we could do some more flexible fix for this like harfbuzz where they more count how many times a substitution has added more glyphs than removed and sets a cap on that instead making a glyph limit less understandable since this fix the glyph limit will only apply when substitution is used leading users to not understand why their font without substitutions works with very long text but not the ones with.

@zeonzip
Copy link
Contributor Author

zeonzip commented Jul 25, 2025

As per the bug report, I strongly suggest making this a multiple of the input buffer size. There are scripts without spaces and there are fonts with contextual spaces both which can result in buffers of a paragraph of text being processed. Harfbuzz uses a factor of 64 for its max (up to a hard limit of 1 billion glyphs). This is reasonable since it is only about spotting runaway growth.

Sure, I'll add a proportional limit tomorrow when I am back

@zeonzip
Copy link
Contributor Author

zeonzip commented Aug 14, 2025

Any update on this? Should we implement something like HarfBuzz's solution for more QOL of the solution or just finish this first dab?

@wezm
Copy link
Contributor

wezm commented Aug 14, 2025

Oh sorry if I was unclear. I think we should do the multiple of input buffer approach as per my comment here #48 (comment)

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