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

[sanitize] change max ops to track number of bytes processed. #2923

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

garretrieger
Copy link
Collaborator

@garretrieger garretrieger commented Mar 31, 2021

Counting bytes as the operations is likely to be a better proxy for how
much work processing the table will cost vs. the current approach of
counting the number of sub-objects.

This should allow checks for max features, max scripts, etc. to be removed.

I tested this change against the full collection of fonts at https://github.com/google/fonts
and a max ops factor of 3 was sufficient to successfully sanitize all of them.

behdad added a commit that referenced this pull request Mar 31, 2021
The code in question was introduced in 70eb2ff.
Rewrite it to not call sanitizer check_range() as we want to use
check_range() for byte accounting.

Part of #2923
@garretrieger garretrieger force-pushed the sanitize branch 2 times, most recently from a6b5288 to ee1e5df Compare April 1, 2021 00:44
@garretrieger garretrieger marked this pull request as ready for review April 1, 2021 00:45
Counting bytes as the operations is likely to be a better proxy for how
much work processing the table will cost vs. the current approach of
counting the number of sub-objects.

This should allow checks for max features, max scripts, etc. to be removed.

I tested this change against the full collection of fonts at https://github.com/google/fonts
and a max ops factor of 3 was sufficient to successfully sanitize all of them.
@behdad behdad merged commit 8ef4257 into harfbuzz:master Apr 1, 2021
@behdad
Copy link
Member

behdad commented Apr 1, 2021

Thanks!

We should consider reducing timeouts now that we don't expect ops to take long.

@garretrieger garretrieger deleted the sanitize branch April 5, 2021 20:36
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.

None yet

2 participants