Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[subset] Raise the bar in new vs old table size #1930

Merged
merged 1 commit into from Aug 25, 2019

Conversation

ebraminio
Copy link
Collaborator

@ebraminio ebraminio commented Aug 25, 2019

https://crbug.com/oss-fuzz/16740

This is actually an interesting thing that {h,v}mtx allocates as
much as a font pretends to have glyphs but the solution is not
that obvious as regular fonts can have less than actually containing
metrics in their {h,v}mtx. This change raises the bar to consider this
hmtx 4 byte for every glyph case.

Initially we wanted to just find things allocating crazy amount of
memory but having the assert has led to interesting findings also
so let's don't remove the assert and see what we can find elsewhere.

This is actually interesting thing that {h,v}mtx allocated as
much as a font pretends have glyphs but the solution is not
that obvious. This change is enough to consider this hmtx case.
Initially we wanted to just find things making crazy amount of
memory but having the assert has led to interesting findings also
so let's don't remove the assert and see what we can find elsewhere.
@ebraminio ebraminio merged commit 269a120 into harfbuzz:master Aug 25, 2019
@ebraminio ebraminio deleted the hmtx branch August 25, 2019 16:07
@behdad
Copy link
Member

behdad commented Aug 26, 2019

OR JUST REMOVE THE ARBITRARY LIMITATION!!!

@ebraminio
Copy link
Collaborator Author

Done

@behdad
Copy link
Member

behdad commented Aug 26, 2019

Can you explain why glyf subset code was running out of room repeatedly? Ideally we like to detect such issues if we can formulate a better way. Thanks.

@ebraminio
Copy link
Collaborator Author

Can you explain why glyf subset code was running out of room repeatedly? Ideally we like to detect such issues if we can formulate a better way. Thanks.

You are right that I should've explained it more and I am sorry.

https://github.com/ebraminio/harfbuzz/blob/2023f0da44ba492740676b5fcd0fb19f4d883979/src/hb-ot-glyf-table.hh#L575 wasn't considering instruction size itself and wasn't failing when there was no space for that. However the logic here source_glyph.length - dest_start.length - instruction_length meant if source_glyph.length was equal to dest_start.length and instruction_length was anything but zero (by reading memory out of the glyph bound) it was becoming a negative value.

@behdad
Copy link
Member

behdad commented Aug 28, 2019

Yeah we should rewrite that bound checking in terms of hb_array_t...

@ebraminio
Copy link
Collaborator Author

Yeah we should rewrite that bound checking in terms of hb_array_t...

That's why I went for rewriting most of it after painfully understanding of what is going on it.

@khaledhosny khaledhosny added the subset hb-subset related bugs label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
subset hb-subset related bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants