Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the caches mutating a deque node through a NonNull pointer derived from a shared reference #6

Merged
merged 1 commit into from
Jul 6, 2023
Merged

Fix the caches mutating a deque node through a NonNull pointer derived from a shared reference #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
Bumped the minimum supported Rust version (MSRV) to 1.61 (May 19, 2022).
([#5][gh-pull-0005])

### Fixed

- Fixed the caches mutating a deque node through a `NonNull` pointer derived from a
shared reference. ([#6][gh-pull-0006]).


## Version 0.10.0

Expand Down Expand Up @@ -40,6 +45,7 @@ lightweight.
<!-- Links -->
[moka-v0.9.6]: https://github.com/moka-rs/moka/tree/v0.9.6

[gh-pull-0006]: https://github.com/moka-rs/mini-moka/pull/6/
[gh-pull-0005]: https://github.com/moka-rs/mini-moka/pull/5/
[gh-pull-0002]: https://github.com/moka-rs/mini-moka/pull/2/
[gh-pull-0001]: https://github.com/moka-rs/mini-moka/pull/1/
96 changes: 68 additions & 28 deletions src/common/deque.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ impl<T> DeqNode<T> {
}
}

pub(crate) fn next_node(&self) -> Option<&DeqNode<T>> {
self.next.as_ref().map(|node| unsafe { node.as_ref() })
pub(crate) fn next_node_ptr(this: NonNull<Self>) -> Option<NonNull<DeqNode<T>>> {
unsafe { this.as_ref() }.next
}
}

Expand Down Expand Up @@ -126,11 +126,13 @@ impl<T> Deque<T> {
}

pub(crate) fn peek_front(&self) -> Option<&DeqNode<T>> {
// This method takes care not to create mutable references to whole nodes,
// to maintain validity of aliasing pointers into `element`.
self.head.as_ref().map(|node| unsafe { node.as_ref() })
}

pub(crate) fn peek_front_ptr(&self) -> Option<NonNull<DeqNode<T>>> {
self.head.as_ref().cloned()
}

/// Removes and returns the node at the front of the list.
pub(crate) fn pop_front(&mut self) -> Option<Box<DeqNode<T>>> {
// This method takes care not to create mutable references to whole nodes,
Expand Down Expand Up @@ -158,9 +160,7 @@ impl<T> Deque<T> {
}

#[cfg(test)]
fn peek_back(&self) -> Option<&DeqNode<T>> {
// This method takes care not to create mutable references to whole nodes,
// to maintain validity of aliasing pointers into `element`.
pub(crate) fn peek_back(&self) -> Option<&DeqNode<T>> {
self.tail.as_ref().map(|node| unsafe { node.as_ref() })
}

Expand Down Expand Up @@ -222,12 +222,20 @@ impl<T> Deque<T> {
}
}

pub(crate) fn move_front_to_back(&mut self) {
if let Some(node) = self.head {
unsafe { self.move_to_back(node) };
}
}

/// Unlinks the specified node from the current list.
///
/// This method takes care not to create mutable references to `element`, to
/// maintain validity of aliasing pointers.
///
/// Panics:
/// IMPORTANT: This method does not drop the node. If the node is no longer
/// needed, use `unlink_and_drop` instead, or drop it at the caller side.
/// Otherwise, the node will leak.
pub(crate) unsafe fn unlink(&mut self, mut node: NonNull<DeqNode<T>>) {
if self.is_at_cursor(node.as_ref()) {
self.advance_cursor();
Expand Down Expand Up @@ -265,7 +273,7 @@ impl<T> Deque<T> {
std::mem::drop(Box::from_raw(node.as_ptr()));
}

#[allow(unused)]
#[cfg(test)]
pub(crate) fn reset_cursor(&mut self) {
self.cursor = None;
}
Expand Down Expand Up @@ -683,35 +691,67 @@ mod tests {
// -------------------------------------------------------
// First iteration.
// peek_front() -> node1
let node1a = deque.peek_front().unwrap();
assert_eq!(node1a.element, "a".to_string());
let node2a = node1a.next_node().unwrap();
assert_eq!(node2a.element, "b".to_string());
let node3a = node2a.next_node().unwrap();
assert_eq!(node3a.element, "c".to_string());
assert!(node3a.next_node().is_none());
let node1a = deque.peek_front_ptr().unwrap();
assert_eq!(unsafe { node1a.as_ref() }.element, "a".to_string());
let node2a = DeqNode::next_node_ptr(node1a).unwrap();
assert_eq!(unsafe { node2a.as_ref() }.element, "b".to_string());
let node3a = DeqNode::next_node_ptr(node2a).unwrap();
assert_eq!(unsafe { node3a.as_ref() }.element, "c".to_string());
assert!(DeqNode::next_node_ptr(node3a).is_none());

// -------------------------------------------------------
// Iterate after a move_to_back.
// Move "b" to the back. So now "a" -> "c" -> "b".
unsafe { deque.move_to_back(node2_ptr) };
let node1a = deque.peek_front().unwrap();
assert_eq!(node1a.element, "a".to_string());
let node3a = node1a.next_node().unwrap();
assert_eq!(node3a.element, "c".to_string());
let node2a = node3a.next_node().unwrap();
assert_eq!(node2a.element, "b".to_string());
assert!(node2a.next_node().is_none());
let node1a = deque.peek_front_ptr().unwrap();
assert_eq!(unsafe { node1a.as_ref() }.element, "a".to_string());
let node3a = DeqNode::next_node_ptr(node1a).unwrap();
assert_eq!(unsafe { node3a.as_ref() }.element, "c".to_string());
let node2a = DeqNode::next_node_ptr(node3a).unwrap();
assert_eq!(unsafe { node2a.as_ref() }.element, "b".to_string());
assert!(DeqNode::next_node_ptr(node2a).is_none());

// -------------------------------------------------------
// Iterate after an unlink.
// Unlink the second node "c". Now "a" -> "c".
unsafe { deque.unlink_and_drop(node3_ptr) };
let node1a = deque.peek_front().unwrap();
assert_eq!(node1a.element, "a".to_string());
let node2a = node1a.next_node().unwrap();
assert_eq!(node2a.element, "b".to_string());
assert!(node2a.next_node().is_none());
let node1a = deque.peek_front_ptr().unwrap();
assert_eq!(unsafe { node1a.as_ref() }.element, "a".to_string());
let node2a = DeqNode::next_node_ptr(node1a).unwrap();
assert_eq!(unsafe { node2a.as_ref() }.element, "b".to_string());
assert!(DeqNode::next_node_ptr(node2a).is_none());
}

#[test]
fn peek_and_move_to_back() {
let mut deque: Deque<String> = Deque::new(MainProbation);

let node1 = DeqNode::new("a".into());
deque.push_back(Box::new(node1));
let node2 = DeqNode::new("b".into());
let _ = deque.push_back(Box::new(node2));
let node3 = DeqNode::new("c".into());
let _ = deque.push_back(Box::new(node3));
// "a" -> "b" -> "c"

let node1a = deque.peek_front_ptr().unwrap();
assert_eq!(unsafe { node1a.as_ref() }.element, "a".to_string());
unsafe { deque.move_to_back(node1a) };
// "b" -> "c" -> "a"

let node2a = deque.peek_front_ptr().unwrap();
assert_eq!(unsafe { node2a.as_ref() }.element, "b".to_string());

let node3a = DeqNode::next_node_ptr(node2a).unwrap();
assert_eq!(unsafe { node3a.as_ref() }.element, "c".to_string());
unsafe { deque.move_to_back(node3a) };
// "b" -> "a" -> "c"

deque.move_front_to_back();
// "a" -> "c" -> "b"

let node1b = deque.peek_front().unwrap();
assert_eq!(node1b.element, "a".to_string());
}

#[test]
Expand Down
23 changes: 9 additions & 14 deletions src/sync/base_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,25 +914,26 @@ where
let mut skipped_nodes = SmallVec::default();

// Get first potential victim at the LRU position.
let mut next_victim = deqs.probation.peek_front();
let mut next_victim = deqs.probation.peek_front_ptr();

// Aggregate potential victims.
while victims.policy_weight < candidate.policy_weight {
if candidate.freq < victims.freq {
break;
}
if let Some(victim) = next_victim.take() {
next_victim = victim.next_node();
next_victim = DeqNode::next_node_ptr(victim);
let vic_elem = &unsafe { victim.as_ref() }.element;

if let Some(vic_entry) = cache.get(victim.element.key()) {
if let Some(vic_entry) = cache.get(vic_elem.key()) {
victims.add_policy_weight(vic_entry.policy_weight());
victims.add_frequency(freq, victim.element.hash());
victim_nodes.push(NonNull::from(victim));
victims.add_frequency(freq, vic_elem.hash());
victim_nodes.push(victim);
retries = 0;
} else {
// Could not get the victim from the cache (hash map). Skip this node
// as its ValueEntry might have been invalidated.
skipped_nodes.push(NonNull::from(victim));
skipped_nodes.push(victim);

retries += 1;
if retries > MAX_CONSECUTIVE_RETRIES {
Expand Down Expand Up @@ -1112,10 +1113,7 @@ where
// invalidated ValueEntry (which should be still in the write op
// queue) has a pointer to this node, move the node to the back of
// the deque instead of popping (dropping) it.
if let Some(node) = deq.peek_front() {
let node = NonNull::from(node);
unsafe { deq.move_to_back(node) };
}
deq.move_front_to_back();
true
}
}
Expand Down Expand Up @@ -1165,10 +1163,7 @@ where
// invalidated ValueEntry (which should be still in the write op
// queue) has a pointer to this node, move the node to the back of
// the deque instead of popping (dropping) it.
if let Some(node) = deqs.write_order.peek_front() {
let node = NonNull::from(node);
unsafe { deqs.write_order.move_to_back(node) };
}
deqs.write_order.move_front_to_back();
}
}
}
Expand Down
13 changes: 7 additions & 6 deletions src/unsync/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,22 +745,23 @@ where
let mut victim_nodes = SmallVec::default();

// Get first potential victim at the LRU position.
let mut next_victim = deqs.probation.peek_front();
let mut next_victim = deqs.probation.peek_front_ptr();

// Aggregate potential victims.
while victims.weight < candidate.weight {
if candidate.freq < victims.freq {
break;
}
if let Some(victim) = next_victim.take() {
next_victim = victim.next_node();
next_victim = DeqNode::next_node_ptr(victim);
let vic_elem = &unsafe { victim.as_ref() }.element;

let vic_entry = cache
.get(&victim.element.key)
.get(&vic_elem.key)
.expect("Cannot get an victim entry");
victims.add_policy_weight(victim.element.key.as_ref(), &vic_entry.value, weigher);
victims.add_frequency(freq, victim.element.hash);
victim_nodes.push(NonNull::from(victim));
victims.add_policy_weight(vic_elem.key.as_ref(), &vic_entry.value, weigher);
victims.add_frequency(freq, vic_elem.hash);
victim_nodes.push(victim);
} else {
// No more potential victims.
break;
Expand Down