Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add protection against out-of-bounds read in ttconv#20629
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add protection against out-of-bounds read in ttconv#20629
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch!
aitikgupta left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Awesome! 🎉
Can we introduce a test (likely the code sample in#20628 (comment)) here?
(that raises another bug, but is there a way we can triggeronly this out-of-bounds error?)
Or.. testing ttconv snippets are out of scope of Matplotlib?
You should try to use thelinking keywords when you know this fixes the issue. Since I've merged the other PR, please add a test here. |
Sorry, about that. I've updated the other open PRs.
Thanks! I'll rebase the PR. It would be super difficult to test the initial issue, i.e. a test that would fail on an out-of-bounds read. The table search exits in most cases by chance. At some point, random bytes interpreted as a table name make the algorithm think it is already beyond the sought for entry. To test this manually, I ran I can, however, add a test, like the example, to make sure that the output is valid (but this test would succeed in ~99% of the cases even without this PR). |
Yes, but it would at least check that the change from the other PR wouldn't break. We'll have to live with the possibility of a flaky error if this one resurfaces, but that's unlikely anyway. I was able to replicate this issue in valgrind, so at least that's known to be fixed. |
This commit adds to counter to track how many tables are read from the tabledirectory and stops the search for further tables early if the total number oftables has been reached.
The new test case checks that Type 42 fonts without the prep table (here stix),are properly embedded. The test succeeds if the embedded font contains valuesfor searchRange, entrySelector, rangeShift; and no erroneous out-of-bounds readoccurs while searching for the missing prep table.
1073013 todeefcbaCompare…629-on-v3.4.xBackport PR#20629 on branch v3.4.x (Add protection against out-of-bounds read in ttconv)
Uh oh!
There was an error while loading.Please reload this page.
This PR addresses#20628.
PR Summary
The member variable
font->numTablesindicates the number of tables stored in the directory table. The size of the memory allocated for the directory tableis derived from this variable. This PR adds a safe-guard inttfont_sfnts()to preventptrto move beyond the end of the table and thus prevent read-access beyond the memory allocated forfont->offset_table.See#20628
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).