close 7442 - Added checked arithmetic to prevent integer overflow #7443
Conversation
|
|
| use zerotrie::dense::ZeroAsciiDenseSparse2dTrieOwned; | ||
|
|
||
| #[test] | ||
| fn test_dense_suffix_count_with_low_frequency_suffixes() { |
There was a problem hiding this comment.
Did you intend to include these tests in this PR or the other PR?
There was a problem hiding this comment.
yess these were to be in the previous PR, my bad
utils/zerotrie/src/dense.rs
Outdated
| .checked_mul(suffix_count) | ||
| .and_then(|v| v.checked_add(column_index))?; | ||
| let Some(offset) = self.dense.get(index) else { | ||
| unreachable!( |
There was a problem hiding this comment.
Please use debug_assert!(false as before, instead of unreachable!, so that we don't have a panic compiled into the code.
| @@ -0,0 +1,149 @@ | |||
| // This file is part of ICU4X. For terms of use, please see the file | |||
There was a problem hiding this comment.
Please put these tests in dense_test.rs and use check_data for the assertions.
| let trie = ZeroAsciiDenseSparse2dTrieOwned::try_from_btree_map_str(&data, b'/').unwrap(); | ||
| let trie = trie.as_borrowed(); | ||
|
|
||
| // With empty dense matrix, all lookups should still work via sparse path |
There was a problem hiding this comment.
Suggestion: assert the Debug output of the trie, which encodes the values that went into sparse and the values that went into dense.
|
Hey @sffc i can see there's 2 fail in the check (semver fails after 8m), looking at the logs, it is due to a pre-existing issue, they're in icu_calendar and icu_locale_core. |
|
@sffc made the required changes, PTAL |
3 participants
Closes #7442
Adds overflow protection to
ZeroAsciiDenseSparse2dTrieBorrowed::get()using checked arithmetic operations.Changes
File: utils/zerotrie/src/dense.rs
checked_mul()andchecked_add()checked_add()which perevents overflow when adding offset to row_value_offset