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.

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 memory leak when evicting or expiring an entry (v0.7.x) #65

Merged
merged 6 commits into from
Jan 11, 2022
47 changes: 47 additions & 0 deletions .github/workflows/Miri.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
name: Miri tests

on:
push:
paths-ignore:
- '.devcontainer/**'
- '.gitpod.yml'
- '.vscode/**'
- 'tests/**'
pull_request:
paths-ignore:
- '.devcontainer/**'
- '.gitpod.yml'
- '.vscode/**'
- 'tests/**'
schedule:
# Run against the last commit on the default branch on Friday at 9pm (UTC?)
- cron: '0 21 * * 5'

jobs:
test:
runs-on: ubuntu-latest

steps:
- name: Checkout Moka
uses: actions/checkout@v2

- name: Install Rust nightly toolchain with Miri
uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: nightly
override: true
components: miri

- uses: Swatinem/rust-cache@v1

- name: cargo clean
uses: actions-rs/cargo@v1
with:
command: clean

- name: Run Miri test (deque)
uses: actions-rs/cargo@v1
with:
command: miri
args: test deque
25 changes: 16 additions & 9 deletions src/common/deque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,17 @@ impl<T> Deque<T> {
self.len -= 1;
}

/// Unlinks the specified node from the current list, and then drop the node.
///
/// This method takes care not to create mutable references to `element`, to
/// maintain validity of aliasing pointers.
///
/// Panics:
pub(crate) unsafe fn unlink_and_drop(&mut self, node: NonNull<DeqNode<T>>) {
self.unlink(node);
std::mem::drop(Box::from_raw(node.as_ptr()));
}

#[allow(unused)]
pub(crate) fn reset_cursor(&mut self) {
self.cursor = None;
Expand Down Expand Up @@ -504,11 +515,7 @@ mod tests {
assert!(!deque.contains(node3_ref));
assert!(node3_ref.next.is_none());
assert!(node3_ref.next.is_none());

// This does not work as expected because NonNull implements Copy trait.
// See clippy(clippy::drop_copy) at
// https://rust-lang.github.io/rust-clippy/master/index.html#drop_copy
// std::mem::drop(node3_ptr);
std::mem::drop(unsafe { Box::from_raw(node3_ptr.as_ptr()) });

// peek_front() -> node2
let head_h = deque.peek_front().unwrap();
Expand Down Expand Up @@ -541,7 +548,7 @@ mod tests {
assert!(!deque.contains(node2_ref));
assert!(node2_ref.next.is_none());
assert!(node2_ref.next.is_none());
// std::mem::drop(node2_ptr);
std::mem::drop(unsafe { Box::from_raw(node2_ptr.as_ptr()) });

// peek_front() -> node1
let head_g = deque.peek_front().unwrap();
Expand All @@ -568,7 +575,7 @@ mod tests {
assert!(!deque.contains(node1_ref));
assert!(node1_ref.next.is_none());
assert!(node1_ref.next.is_none());
// std::mem::drop(node1_ptr);
std::mem::drop(unsafe { Box::from_raw(node1_ptr.as_ptr()) });

// peek_front() -> node1
let head_h = deque.peek_front();
Expand Down Expand Up @@ -629,7 +636,7 @@ mod tests {
// Try to unlink during iteration.
assert_eq!((&mut deque).next(), Some(&"a".into()));
// Next will be "c", but we unlink it.
unsafe { deque.unlink(node3_ptr) };
unsafe { deque.unlink_and_drop(node3_ptr) };
// Now, next should be "b".
assert_eq!((&mut deque).next(), Some(&"b".into()));
assert!((&mut deque).next().is_none());
Expand Down Expand Up @@ -691,7 +698,7 @@ mod tests {
// -------------------------------------------------------
// Iterate after an unlink.
// Unlink the second node "c". Now "a" -> "c".
unsafe { deque.unlink(node3_ptr) };
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();
Expand Down
6 changes: 4 additions & 2 deletions src/sync/deques.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ impl<K> Deques<K> {
let p = node.as_ref();
if &p.region == deq.region() {
if deq.contains(p) {
deq.unlink(node);
// https://github.com/moka-rs/moka/issues/64
deq.unlink_and_drop(node);
}
} else {
panic!(
Expand All @@ -181,7 +182,8 @@ impl<K> Deques<K> {
let p = node.as_ref();
if &p.region == deq.region() {
if deq.contains(p) {
deq.unlink(node);
// https://github.com/moka-rs/moka/issues/64
deq.unlink_and_drop(node);
}
} else {
panic!(
Expand Down
6 changes: 4 additions & 2 deletions src/unsync/deques.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@ impl<K> Deques<K> {
node: NonNull<DeqNode<KeyHashDate<K>>>,
) {
if deq.contains(node.as_ref()) {
deq.unlink(node);
// https://github.com/moka-rs/moka/issues/64
deq.unlink_and_drop(node);
} else {
panic!(
"unlink_node - node is not a member of {} deque. {:?}",
Expand All @@ -138,7 +139,8 @@ impl<K> Deques<K> {
let p = node.as_ref();
debug_assert_eq!(&p.region, &WriteOrder);
if deq.contains(p) {
deq.unlink(node);
// https://github.com/moka-rs/moka/issues/64
deq.unlink_and_drop(node);
} else {
panic!(
"unlink_node - node is not a member of write_order deque. {:?}",
Expand Down