Skip to content

Commit 265291f

Browse files
fix(node): avoid rolling-hash overflow panic (#195)
The rolling hash used for chunk-boundary detection could overflow and panic on some inputs; this makes that computation overflow-safe. Includes a focused regression test (tests/rolling_hash_overflow.rs). Isolated fix off current main; no unrelated changes.
1 parent 48cdd90 commit 265291f

3 files changed

Lines changed: 101 additions & 12 deletions

File tree

src/node.rs

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -696,11 +696,12 @@ impl<const N: usize> NodeChunk for ProllyNode<N> {
696696
base: u64,
697697
modulus: u64,
698698
) -> u64 {
699-
let mut hash = 0;
699+
let mut hash: u64 = 0;
700700
for (key, value) in keys.iter().zip(values) {
701-
hash = (hash * base
702-
+ Self::hash_item(key, base, modulus)
703-
+ Self::hash_item(value, base, modulus))
701+
hash = hash
702+
.wrapping_mul(base)
703+
.wrapping_add(Self::hash_item(key, base, modulus))
704+
.wrapping_add(Self::hash_item(value, base, modulus))
704705
% modulus;
705706
}
706707
hash
@@ -723,23 +724,28 @@ impl<const N: usize> NodeChunk for ProllyNode<N> {
723724

724725
let base_exp_window_size = Self::mod_exp(base, window_size, modulus);
725726

726-
let hash = (old_hash * base + new_key_hash + new_value_hash) % modulus;
727-
let hash = (hash + modulus - (old_key_hash * base_exp_window_size) % modulus) % modulus;
727+
let hash = old_hash
728+
.wrapping_mul(base)
729+
.wrapping_add(new_key_hash)
730+
.wrapping_add(new_value_hash)
731+
% modulus;
732+
let hash = (hash + modulus - (old_key_hash.wrapping_mul(base_exp_window_size)) % modulus)
733+
% modulus;
728734

729-
(hash + modulus - (old_value_hash * base_exp_window_size) % modulus) % modulus
735+
(hash + modulus - (old_value_hash.wrapping_mul(base_exp_window_size)) % modulus) % modulus
730736
}
731737

732738
fn mod_exp(base: u64, exp: u64, modulus: u64) -> u64 {
733-
let mut result = 1;
739+
let mut result: u64 = 1;
734740
let mut base = base % modulus;
735741
let mut exp = exp;
736742

737743
while exp > 0 {
738744
if exp % 2 == 1 {
739-
result = (result * base) % modulus;
745+
result = (result.wrapping_mul(base)) % modulus;
740746
}
741747
exp >>= 1;
742-
base = (base * base) % modulus;
748+
base = (base.wrapping_mul(base)) % modulus;
743749
}
744750

745751
result

src/streaming_chunker.rs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,13 +138,23 @@ impl Splitter for RollingHashSplitter {
138138
if self.window.len() < self.min_chunk_size {
139139
// Building the initial window. This mirrors
140140
// `initialize_rolling_hash`: hash = hash * base + kh + vh.
141-
self.hash = (self.hash.wrapping_mul(self.base) + kh + vh) % self.modulus;
141+
self.hash = self
142+
.hash
143+
.wrapping_mul(self.base)
144+
.wrapping_add(kh)
145+
.wrapping_add(vh)
146+
% self.modulus;
142147
self.window.push_back((kh, vh));
143148
} else {
144149
// Slide: drop the front of the window, add the new item.
145150
// Matches `update_rolling_hash`.
146151
let (old_kh, old_vh) = self.window.pop_front().unwrap();
147-
let mut h = (self.hash.wrapping_mul(self.base) + kh + vh) % self.modulus;
152+
let mut h = self
153+
.hash
154+
.wrapping_mul(self.base)
155+
.wrapping_add(kh)
156+
.wrapping_add(vh)
157+
% self.modulus;
148158
h = (h + self.modulus - (old_kh.wrapping_mul(self.base_exp_min)) % self.modulus)
149159
% self.modulus;
150160
h = (h + self.modulus - (old_vh.wrapping_mul(self.base_exp_min)) % self.modulus)

tests/rolling_hash_overflow.rs

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
/*
2+
Licensed under the Apache License, Version 2.0 (the "License");
3+
you may not use this file except in compliance with the License.
4+
You may obtain a copy of the License at
5+
6+
http://www.apache.org/licenses/LICENSE-2.0
7+
8+
Unless required by applicable law or agreed to in writing, software
9+
distributed under the License is distributed on an "AS IS" BASIS,
10+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
See the License for the specific language governing permissions and
12+
limitations under the License.
13+
*/
14+
15+
use prollytree::config::TreeConfig;
16+
use prollytree::node::{Node, ProllyNode};
17+
use prollytree::storage::InMemoryNodeStorage;
18+
use prollytree::tree::{ProllyTree, Tree};
19+
20+
fn large_modulus_config() -> TreeConfig<32> {
21+
TreeConfig::<32> {
22+
base: 4_000_000_007,
23+
modulus: 5_000_000_000,
24+
min_chunk_size: 2,
25+
max_chunk_size: 8,
26+
pattern: 0b11,
27+
..TreeConfig::default()
28+
}
29+
}
30+
31+
#[test]
32+
fn build_canonical_large_modulus_does_not_overflow_and_preserves_keys() {
33+
let config = large_modulus_config();
34+
let pairs: Vec<_> = (0..64u64)
35+
.map(|i| (i.to_be_bytes().to_vec(), format!("value-{i}").into_bytes()))
36+
.collect();
37+
let mut storage = InMemoryNodeStorage::<32>::default();
38+
39+
let root = ProllyNode::<32>::build_canonical_from_pairs(pairs.clone(), &config, &mut storage);
40+
41+
for (key, value) in pairs {
42+
let leaf = root
43+
.find(&key, &storage)
44+
.expect("key should be retrievable");
45+
let index = leaf
46+
.keys
47+
.iter()
48+
.position(|stored_key| stored_key == &key)
49+
.expect("leaf should contain key");
50+
assert_eq!(leaf.values[index], value);
51+
}
52+
}
53+
54+
#[test]
55+
fn prolly_tree_large_modulus_does_not_overflow_and_preserves_keys() {
56+
let config = large_modulus_config();
57+
let mut tree = ProllyTree::new(InMemoryNodeStorage::<32>::default(), config);
58+
59+
for i in 0..64u64 {
60+
tree.insert(i.to_be_bytes().to_vec(), format!("value-{i}").into_bytes());
61+
}
62+
63+
for i in 0..64u64 {
64+
let key = i.to_be_bytes().to_vec();
65+
let leaf = tree.find(&key).expect("key should be retrievable");
66+
let index = leaf
67+
.keys
68+
.iter()
69+
.position(|stored_key| stored_key == &key)
70+
.expect("leaf should contain key");
71+
assert_eq!(leaf.values[index], format!("value-{i}").into_bytes());
72+
}
73+
}

0 commit comments

Comments
 (0)