Merge pull request #283 from moka-rs/m09-backport-gh259

Backport #259 to v0.9.8

CHANGELOG.md

@@ -5,6 +5,12 @@
 Bumped the minimum supported Rust version (MSRV) to 1.60 (Apr 7, 2022).
 ([#282][gh-pull-0282])
 
+### Fixed
+
+- Fixed the caches mutating a deque node through a `NonNull` pointer derived from a
+  shared reference. ([#283][gh-pull-0283]) (Backported [#259](gh-pull-0259) from
+  v0.11.0)
+
 ### Changed
 
 - Upgraded `quanta` crate to v0.11.0. ([#282][gh-pull-0282])
@@ -545,6 +551,8 @@ The minimum supported Rust version (MSRV) is now 1.51.0 (2021-03-25).
 [ghsa-qc84-gqf4-9926]: https://github.com/advisories/GHSA-qc84-gqf4-9926
 [gh-rust-issue-62958]: https://github.com/rust-lang/rust/issues/62958
 
+[RUSTSEC-2020-0168]: https://rustsec.org/advisories/RUSTSEC-2020-0168.html
+
 [gh-06chaynes]: https://github.com/06chaynes
 [gh-aspect]: https://github.com/aspect
 [gh-barkanido]: https://github.com/barkanido
@@ -557,6 +565,7 @@ The minimum supported Rust version (MSRV) is now 1.51.0 (2021-03-25).
 [gh-Swatinem]: https://github.com/Swatinem
 [gh-tinou98]: https://github.com/tinou98
 
+[gh-issue-0243]: https://github.com/moka-rs/moka/issues/243/
 [gh-issue-0212]: https://github.com/moka-rs/moka/issues/212/
 [gh-issue-0207]: https://github.com/moka-rs/moka/issues/207/
 [gh-issue-0162]: https://github.com/moka-rs/moka/issues/162/
@@ -571,7 +580,9 @@ The minimum supported Rust version (MSRV) is now 1.51.0 (2021-03-25).
 [gh-issue-0034]: https://github.com/moka-rs/moka/issues/34/
 [gh-issue-0031]: https://github.com/moka-rs/moka/issues/31/
 
+[gh-pull-0283]: https://github.com/moka-rs/moka/pull/283/
 [gh-pull-0282]: https://github.com/moka-rs/moka/pull/282/
+[gh-pull-0259]: https://github.com/moka-rs/moka/pull/259/
 [gh-pull-0216]: https://github.com/moka-rs/moka/pull/216/
 [gh-pull-0195]: https://github.com/moka-rs/moka/pull/195/
 [gh-pull-0190]: https://github.com/moka-rs/moka/pull/190/

README.md

@@ -86,7 +86,10 @@ routers. Here are some highlights:
 
 ## Change Log
 
-- [CHANGELOG.md](https://github.com/moka-rs/moka/blob/master/CHANGELOG.md)
+- For v0.9.x releases:
+  [CHANGELOG.md (`maint-09` branch)](https://github.com/moka-rs/moka/blob/maint-09/CHANGELOG.md)
+- For the latest release:
+  [CHANGELOG.md (`master` branch)](https://github.com/moka-rs/moka/blob/master/CHANGELOG.md)
 
 
 ## Table of Contents

src/common/deque.rs

@@ -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
     }
 }
 
@@ -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,
@@ -159,8 +161,6 @@ 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`.
         self.tail.as_ref().map(|node| unsafe { node.as_ref() })
     }
 
@@ -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();
@@ -265,7 +273,7 @@ impl<T> Deque<T> {
         std::mem::drop(Box::from_raw(node.as_ptr()));
     }
 
-    #[allow(unused)]
+    #[cfg(any(test, feature = "sync", feature = "future"))]
     pub(crate) fn reset_cursor(&mut self) {
         self.cursor = None;
     }
@@ -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]

src/dash/base_cache.rs

@@ -921,25 +921,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 {
@@ -1119,10 +1120,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
         }
     }
@@ -1172,10 +1170,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();
             }
         }
     }

src/sync_base/base_cache.rs

@@ -1447,26 +1447,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();
-                let vic_elem = &victim.element;
+                next_victim = DeqNode::next_node_ptr(victim);
+                let vic_elem = &unsafe { victim.as_ref() }.element;
 
                 if let Some(vic_entry) = cache.get(vic_elem.hash(), |k| k == vic_elem.key()) {
                     victims.add_policy_weight(vic_entry.policy_weight());
                     victims.add_frequency(freq, vic_elem.hash());
-                    victim_nodes.push(NonNull::from(victim));
+                    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 {
@@ -1678,10 +1678,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
         }
     }
@@ -1744,10 +1741,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();
             }
         }
     }

src/unsync/cache.rs

@@ -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;