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

Fix fuzzer bug with cmap4 #1101

Merged
merged 1 commit into from
Oct 3, 2024
Merged

Fix fuzzer bug with cmap4 #1101

merged 1 commit into from
Oct 3, 2024

Conversation

rsheeter
Copy link
Collaborator

@rsheeter rsheeter commented Aug 20, 2024

#954 (the same for cmap12) has raw bytes which are presumably the minimized test repro (?)

The fuzzer repro for this is 137K so we use hand-crafted data to demonstrate the Cmap4Iter used to be happy to reprocess codepoints.

IMHO the floor for the next range should be cur_range.end + 1 but this somehow breaks a klippa integrate test so I deferred that to #1166.

Fixes #1100

@drott
Copy link
Contributor

drott commented Aug 20, 2024

The fuzzer repro for this is 137K

Is most of the size all in cmap or in other parts? If it's only cmap, not sure if it helps here, but @cmyr had a good suggestion on how to test per table only in #969 (comment)

@rsheeter
Copy link
Collaborator Author

A significant portion is cmap, ttx -l reports cmap to be 73978 long. I think I might have to resort to mangling some bytes by hand when I have a free minute.

@rsheeter rsheeter force-pushed the fz3 branch 4 times, most recently from c291fa5 to 520c668 Compare October 1, 2024 18:20
// https://github.com/googlefonts/fontations/issues/1100
// cmap4 ranges are u16 so no need to stress about values past char::MAX
self.cur_range = next_range.start.max(self.cur_range.end)
..next_range.end.max(self.cur_range.end);
self.cur_start_code = self.cur_range.start as u16;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it matters in broken fonts, but I think self.cur_start_code should still be set to next_range.start even if we chop up the range because the remainder of the encoded data is likely dependent on the original value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see your point but also don't want to produce duplicate values. I'm a touch low on time so lets get the immediate fix landed and defer that part to a follow-on. Filed as #1175.

@rsheeter rsheeter merged commit 11af007 into main Oct 3, 2024
10 checks passed
@rsheeter rsheeter deleted the fz3 branch October 3, 2024 17:48
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.

[fuzzer] cmap4 timeout, merrily accepts overlapping ranges
3 participants