Don't pre-allocate the Type1 /Subrs array from the declared count#32062
Don't pre-allocate the Type1 /Subrs array from the declared count#32062vsaraikin wants to merge 1 commit into
Conversation
_parse_subrs allocated [None] * count from the count declared in the font before reading the body, so a malformed font declaring a huge count could force a large allocation before being rejected. Accumulate entries into a dict and build the list after the body parses, matching _parse_charstrings.
|
Thank you for opening your first PR into Matplotlib! If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks. We also ask that you please finish addressing any review comments on this PR and wait for it to be merged (or closed) before opening a new one, as it can be a valuable learning experience to go through the review process. You can also join us on discourse chat for real-time discussion. For details on testing, writing docs, and our review process, please see the developer guide. We strive to be a welcoming and open project. Please follow our Code of Conduct. |
| # force a large allocation before it is rejected; _parse_charstrings | ||
| # already avoids this by accumulating into a dict. |
There was a problem hiding this comment.
| # force a large allocation before it is rejected; _parse_charstrings | |
| # already avoids this by accumulating into a dict. | |
| # force a large allocation before it is rejected |
This is not relevant here. Also (without being expert in this part of the codebase), _parse_charstrings actually returns a dict, so accumulting in the dict is natural there. Whereas here it an explicit measure to prevent pre-allocation.
| path.write_bytes(pfa) | ||
|
|
||
| tracemalloc.start() | ||
| with pytest.raises(StopIteration): |
There was a problem hiding this comment.
Am I correct that this StopIteration is raised from one of the next() calls in the for-loop?
Should we catch that and raise a more appropriate error such as
try:
for _ in range(count):
...
except StopIteration:
raise RuntimeError("Malformed Type1 font file: Incomplete /Subrs")
Fixes #31962.
When parsing a Type1 font,
_parse_subrsread the declared/Subrscount and immediately didarray = [None] * count, before reading the body. The count comes straight from the font file, so a malformed font can declare a huge count in a handful of bytes and force a large allocation before anything is validated._parse_charstringsalready avoids this by accumulating into a dict, so this just brings_parse_subrsin line.The change accumulates the parsed entries into a dict and only allocates the
count-sized result list once the body has actually been read.The regression test builds a tiny font that declares
/Subrs 5000000 arraywith an empty body. On master that allocates ~40 MB ([None] * 5000000) before the parse fails; with the change the parse still fails the same way but peak allocation stays small. The test uses tracemalloc and asserts the peak stays under 5 MB.