rust-farmhash icon indicating copy to clipboard operation
rust-farmhash copied to clipboard

Fix overflow-induced panic

Open ryanpbrewster opened this issue 2 years ago • 0 comments

At master, invoking

hash32_with_seed("trial-0-key-27".as_bytes(), 1);

will trigger a panic (in debug mode) due to integer overflow:

thread 'test_hash32_no_overflow' panicked at 'attempt to add with overflow', src/farmhashmk_shared.rs:17:17
stack backtrace:
   0: rust_begin_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:48:5
   3: farmhash::farmhashmk_shared::mk_hask32_len_13_to_24
             at ./src/farmhashmk_shared.rs:17:17
   4: farmhash::farmhashmk::mk_hash32_with_seed
             at ./src/farmhashmk.rs:15:20
   5: farmhash::hash32_with_seed
             at ./src/lib.rs:35:5
   6: test_hash32::test_hash32_no_overflow
             at ./tests/test_hash32.rs:108:13
   7: test_hash32::test_hash32_no_overflow::{{closure}}
             at ./tests/test_hash32.rs:107:1
   8: core::ops::function::FnOnce::call_once
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ops/function.rs:248:5
   9: core::ops::function::FnOnce::call_once
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Most of the other arithmetic operations are explicitly using wrapping_xyz variants to avoid this.

While investigating this, I went ahead and did a brief audit of the other 32-bit hash functions for non-wrapping adds, then reverse-engineered inputs that would trigger panics on those. I added test cases as I went, verified that they panic at master, then adjusted the code and verified that they now pass.

ryanpbrewster avatar Dec 28 '22 00:12 ryanpbrewster