Avoid integer overflows with correct signing (tracking HB) and assertions#146
Avoid integer overflows with correct signing (tracking HB) and assertions#146alerque merged 2 commits intoharfbuzz:mainfrom
Conversation
61ee39e to
424d79f
Compare
|
After some more fiddling I ruled out most of the other potential instances of subtractions from It might be better to fix these with an |
|
See #142 (comment) |
424d79f to
b7898a9
Compare
b7898a9 to
86f235c
Compare
86f235c to
b7c7169
Compare
src/hb/ot_layout_gsubgpos.rs
Outdated
| } | ||
|
|
||
| ctx.buffer.move_to(end); | ||
| ctx.buffer.move_to(end as usize); |
There was a problem hiding this comment.
end is always guaranteed to be positive here?
There was a problem hiding this comment.
Good question. At first blush it looks like on this point it is a bug-for-bug port of HB. The upstream function takes an unsigned integer argument:
bool
hb_buffer_t::move_to (unsigned int i)But it is being passed a signed integer:
int end;
...
(void) buffer->move_to (end);I don't know if/how C resolves this. @behdad?
At some point it looks like there is a guarantee that it will be at least as large as...
if (end < int (match_positions[idx]))
{
end = match_positions[idx];There was a problem hiding this comment.
Yes, end is also always positive because there's at least one glyph in the match sequence... You can assert as you cast it.
Would you be able to send a PR to HB to add similar asserts?
There was a problem hiding this comment.
I added an assertion for this case too (via Rust's TryInto trait since we aren't enforcing any context specific bounds, just making sure the cast can't loose information) and also opened a matching PR on HB.
37e79ca to
f33bd0c
Compare
f33bd0c to
d5b704b
Compare
LaurenzV
left a comment
There was a problem hiding this comment.
I didn't compare it with the HB code, but seems fine. 😄
See discussion in issue #142,
currently known to be incomplete and untested.The lack of test cases that cover the original code made a POC hard to work on.Closes #142.