al8n / caches-rs

This is a Rust implementation for popular caches (support no_std).
104 stars 14 forks source link

WTinyLFUCache panics with misaligned pointer dereference in debug builds #10

Closed wfraser closed 1 year ago

wfraser commented 1 year ago

Rust now checks for misaligned pointer access in debug builds, which it seems WTinyLFUCache is always doing.

% rustc -Vv
rustc 1.72.1 (d5c2e9c34 2023-09-13)
binary: rustc
commit-hash: d5c2e9c342b358556da91d61ed4133f6f50fc0c3
commit-date: 2023-09-13
host: aarch64-apple-darwin
release: 1.72.1
LLVM version: 16.0.5

(edit: note my host is aarch64, but this issue was also observed on an x86_64 Linux machine)

use caches::{
    Cache,
    WTinyLFUCache,
};

fn main() {
    let mut cache = WTinyLFUCache::<String, i64>::with_sizes(6, 4, 10, 5).unwrap();
    cache.get(&"blah".to_owned());
}
thread 'main' panicked at 'misaligned pointer dereference: address must be a multiple of 0x8 but is 0x600002110252', /Users/wfraser/.cargo/registry/src/index.crates.io-6f17d22bba15001f/caches-0.2.4/src/lfu/tinylfu/bloom.rs:109:22
stack backtrace:
   0: rust_begin_unwind
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/std/src/panicking.rs:593:5
   1: core::panicking::panic_nounwind_fmt
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/panicking.rs:96:14
   2: core::panicking::panic_misaligned_pointer_dereference
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/panicking.rs:175:5
   3: caches::lfu::tinylfu::bloom::Bloom::is_set
             at /Users/wfraser/.cargo/registry/src/index.crates.io-6f17d22bba15001f/caches-0.2.4/src/lfu/tinylfu/bloom.rs:109:22
   4: caches::lfu::tinylfu::bloom::Bloom::contains
             at /Users/wfraser/.cargo/registry/src/index.crates.io-6f17d22bba15001f/caches-0.2.4/src/lfu/tinylfu/bloom.rs:130:17
   5: caches::lfu::tinylfu::bloom::Bloom::contains_or_add
             at /Users/wfraser/.cargo/registry/src/index.crates.io-6f17d22bba15001f/caches-0.2.4/src/lfu/tinylfu/bloom.rs:141:12
   6: caches::lfu::tinylfu::TinyLFU<K,KH>::increment
             at /Users/wfraser/.cargo/registry/src/index.crates.io-6f17d22bba15001f/caches-0.2.4/src/lfu/tinylfu.rs:220:13
   7: <caches::lfu::wtinylfu::WTinyLFUCache<K,V,KH,FH,RH,WH> as caches::cache_api::Cache<K,V>>::get
             at /Users/wfraser/.cargo/registry/src/index.crates.io-6f17d22bba15001f/caches-0.2.4/src/lfu/wtinylfu.rs:550:9
   8: super_simple::main
             at ./src/main.rs:8:5
   9: core::ops::function::FnOnce::call_once
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread caused non-unwinding panic. aborting.
wfraser commented 1 year ago

Probably the simplest fix is to just:

diff --git a/rust/vendor/caches-0.2.3/src/lfu/tinylfu/bloom.rs b/rust/vendor/caches-0.2.3/src/lfu/tinylfu/bloom.rs
index c9485f6d2f3a1..ca73e069ed8e4 100644
--- a/rust/vendor/caches-0.2.3/src/lfu/tinylfu/bloom.rs
+++ b/rust/vendor/caches-0.2.3/src/lfu/tinylfu/bloom.rs
@@ -96,7 +96,9 @@ impl Bloom {
         let offset = ((idx % 64) >> 3) as u64;
         let ptr = (raw + offset) as *mut u64;
         unsafe {
-            *ptr |= MASK[idx % 8] as u64;
+            ptr.write_unaligned(ptr.read_unaligned() | MASK[idx % 8] as u64);
         }
     }

@@ -106,7 +108,7 @@ impl Bloom {
         let offset = ((idx % 64) >> 3) as u64;
         let ptr = (raw + offset) as *mut u64;
         unsafe {
-            let r = (*ptr >> (idx % 8)) & 1;
+            let r = (ptr.read_unaligned() >> (idx % 8)) & 1;
             r == 1
         }
     }

also note, the tests in bloom.rs are currently broken, but I commented out the last 3 and it passes with this change.

al8n commented 1 year ago

Thanks for reporting! Fixed in 0.2.5.

wfraser commented 1 year ago

Thanks! Amazing quick fix!

Just one small question: the 0.2.5 release seems to have also added a dependency on getrandom (which also pulls in more deps), but I don't see this being used anywhere in the code. Is this something that can be removed?

al8n commented 1 year ago

Thanks! Amazing quick fix!

Just one small question: the 0.2.5 release seems to have also added a dependency on getrandom (which also pulls in more deps), but I don't see this being used anywhere in the code. Is this something that can be removed?

Ah, It is a feature gate for supporting the target wasm32 family (rand requires). It should only be compiled for wasm family targets.