diff --git a/src/linked_hash_map.rs b/src/linked_hash_map.rs index 2a442d6..978c74c 100644 --- a/src/linked_hash_map.rs +++ b/src/linked_hash_map.rs @@ -482,17 +482,22 @@ where let mut cur = values.as_ref().links.value.next; while cur != values { let next = cur.as_ref().links.value.next; + // Compute the hash before invoking the callback. The callback + // only receives `&K`, but interior mutability could change the + // key's hash or equality, and this node is stored in the table + // under its *original* hash. + let hash = hash_key(&self.hash_builder, (*cur.as_ptr()).key_ref()); let filter = { let (k, v) = (*cur.as_ptr()).entry_mut(); !f(k, v) }; if filter { - let k = (*cur.as_ptr()).key_ref(); - let hash = hash_key(&self.hash_builder, k); - self.table - .find_entry(hash, |o| (*o).as_ref().key_ref().eq(k)) - .unwrap() - .remove(); + // Remove the table entry pointing at *this* node, matching + // by pointer identity rather than key equality: the callback + // may have mutated the key to compare equal to a different + // entry, which would otherwise leave the table referencing a + // freed node. + self.table.find_entry(hash, |o| *o == cur).unwrap().remove(); drop_filtered_values.drop_later(cur); } cur = next; diff --git a/tests/linked_hash_map.rs b/tests/linked_hash_map.rs index 911b5b6..5dbc535 100644 --- a/tests/linked_hash_map.rs +++ b/tests/linked_hash_map.rs @@ -1,5 +1,6 @@ use std::{ - cell::Cell, + cell::{Cell, RefCell}, + hash::{Hash, Hasher}, panic::{catch_unwind, AssertUnwindSafe}, rc::Rc, }; @@ -868,3 +869,58 @@ fn test_clear_panic_safe() { // stale, moved-out node. drop(map); } + +// Regression test for https://github.com/djc/hashlink/issues/42 +// +// A key that changes its hash/equality through interior mutability inside the +// `retain_with_order` callback must not make the method remove a different table entry than the +// node it frees, which would leave the table referencing a freed node. +#[test] +fn test_retain_with_order_key_mutation_sound() { + #[derive(Debug)] + struct Key(RefCell); + + impl Key { + fn new(value: &str) -> Self { + Self(RefCell::new(value.to_owned())) + } + + fn set(&self, value: &str) { + *self.0.borrow_mut() = value.to_owned(); + } + } + + impl PartialEq for Key { + fn eq(&self, other: &Self) -> bool { + self.0.borrow().as_str() == other.0.borrow().as_str() + } + } + + impl Eq for Key {} + + impl Hash for Key { + fn hash(&self, state: &mut H) { + self.0.borrow().hash(state); + } + } + + let mut map = LinkedHashMap::new(); + map.insert(Key::new("a"), 1); + map.insert(Key::new("b"), 2); + + // The callback mutates the first key to compare equal to the second, then + // asks to drop it. + let mut first = true; + map.retain_with_order(|key, _| { + if first { + first = false; + key.set("b"); + false + } else { + true + } + }); + + // Looking up a stale key must not dereference a freed node. + let _ = map.contains_key(&Key::new("a")); +}