From 9ded74380681f0496994bf83b0c32299845fc36e Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Tue, 11 Jan 2022 08:13:17 +0800 Subject: [PATCH 1/6] Fix memory leak (#64) when unlinking a DeqNode for evicting/expiring an entry --- src/common/deque.rs | 12 +++++------- src/sync/deques.rs | 4 ++++ src/unsync/deques.rs | 4 ++++ 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/common/deque.rs b/src/common/deque.rs index f55f0391..9c658d5f 100644 --- a/src/common/deque.rs +++ b/src/common/deque.rs @@ -504,11 +504,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(); @@ -541,7 +537,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(); @@ -568,7 +564,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(); @@ -630,6 +626,7 @@ mod tests { assert_eq!((&mut deque).next(), Some(&"a".into())); // Next will be "c", but we unlink it. unsafe { deque.unlink(node3_ptr) }; + std::mem::drop(unsafe { Box::from_raw(node3_ptr.as_ptr()) }); // Now, next should be "b". assert_eq!((&mut deque).next(), Some(&"b".into())); assert!((&mut deque).next().is_none()); @@ -692,6 +689,7 @@ mod tests { // Iterate after an unlink. // Unlink the second node "c". Now "a" -> "c". unsafe { deque.unlink(node3_ptr) }; + std::mem::drop(unsafe { Box::from_raw(node3_ptr.as_ptr()) }); let node1a = deque.peek_front().unwrap(); assert_eq!(node1a.element, "a".to_string()); let node2a = node1a.next_node().unwrap(); diff --git a/src/sync/deques.rs b/src/sync/deques.rs index 99e0bd13..52fe260c 100644 --- a/src/sync/deques.rs +++ b/src/sync/deques.rs @@ -167,6 +167,8 @@ impl Deques { if &p.region == deq.region() { if deq.contains(p) { deq.unlink(node); + // https://github.com/moka-rs/moka/issues/64 + drop(Box::from_raw(node.as_ptr())); } } else { panic!( @@ -182,6 +184,8 @@ impl Deques { if &p.region == deq.region() { if deq.contains(p) { deq.unlink(node); + // https://github.com/moka-rs/moka/issues/64 + drop(Box::from_raw(node.as_ptr())); } } else { panic!( diff --git a/src/unsync/deques.rs b/src/unsync/deques.rs index 56d39aee..112652df 100644 --- a/src/unsync/deques.rs +++ b/src/unsync/deques.rs @@ -123,6 +123,8 @@ impl Deques { ) { if deq.contains(node.as_ref()) { deq.unlink(node); + // https://github.com/moka-rs/moka/issues/64 + drop(Box::from_raw(node.as_ptr())); } else { panic!( "unlink_node - node is not a member of {} deque. {:?}", @@ -139,6 +141,8 @@ impl Deques { debug_assert_eq!(&p.region, &WriteOrder); if deq.contains(p) { deq.unlink(node); + // https://github.com/moka-rs/moka/issues/64 + drop(Box::from_raw(node.as_ptr())); } else { panic!( "unlink_node - node is not a member of write_order deque. {:?}", From 22c152a060058b8010c9213734242564c2414255 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Tue, 11 Jan 2022 17:17:30 +0800 Subject: [PATCH 2/6] Fix memory leak (#64) when unlinking a DeqNode for evicting/expiring an entry Add `unlink_and_drop` method to `common::Deque`. --- src/common/deque.rs | 14 ++++++++++++-- src/sync/deques.rs | 6 ++---- src/unsync/deques.rs | 6 ++---- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/common/deque.rs b/src/common/deque.rs index 9c658d5f..e3997ca2 100644 --- a/src/common/deque.rs +++ b/src/common/deque.rs @@ -246,6 +246,17 @@ impl Deque { 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>) { + self.unlink(node); + std::mem::drop(Box::from_raw(node.as_ptr())); + } + #[allow(unused)] pub(crate) fn reset_cursor(&mut self) { self.cursor = None; @@ -625,8 +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) }; - std::mem::drop(unsafe { Box::from_raw(node3_ptr.as_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()); diff --git a/src/sync/deques.rs b/src/sync/deques.rs index 52fe260c..68f400d7 100644 --- a/src/sync/deques.rs +++ b/src/sync/deques.rs @@ -166,9 +166,8 @@ impl Deques { 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 - drop(Box::from_raw(node.as_ptr())); + deq.unlink_and_drop(node); } } else { panic!( @@ -183,9 +182,8 @@ impl Deques { 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 - drop(Box::from_raw(node.as_ptr())); + deq.unlink_and_drop(node); } } else { panic!( diff --git a/src/unsync/deques.rs b/src/unsync/deques.rs index 112652df..844a74b4 100644 --- a/src/unsync/deques.rs +++ b/src/unsync/deques.rs @@ -122,9 +122,8 @@ impl Deques { node: NonNull>>, ) { if deq.contains(node.as_ref()) { - deq.unlink(node); // https://github.com/moka-rs/moka/issues/64 - drop(Box::from_raw(node.as_ptr())); + deq.unlink_and_drop(node); } else { panic!( "unlink_node - node is not a member of {} deque. {:?}", @@ -140,9 +139,8 @@ impl Deques { 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 - drop(Box::from_raw(node.as_ptr())); + deq.unlink_and_drop(node); } else { panic!( "unlink_node - node is not a member of write_order deque. {:?}", From df22a939d4ffa8f9758dda428055d3f902eb41d7 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Tue, 11 Jan 2022 17:23:15 +0800 Subject: [PATCH 3/6] Fix memory leak (#64) when unlinking a DeqNode for evicting/expiring an entry Add GitHub CI to run Miri test on the `deque` module. --- .github/workflows/Miri.yml | 46 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 .github/workflows/Miri.yml diff --git a/.github/workflows/Miri.yml b/.github/workflows/Miri.yml new file mode 100644 index 00000000..d83a56a6 --- /dev/null +++ b/.github/workflows/Miri.yml @@ -0,0 +1,46 @@ +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: ${{ matrix.rust }} + 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 test deque From ffe41f004397ee1374380d93df67e235d608b9b5 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Tue, 11 Jan 2022 17:25:35 +0800 Subject: [PATCH 4/6] Fix memory leak (#64) when unlinking a DeqNode for evicting/expiring an entry Add GitHub CI to run Miri test on the `deque` module. --- .github/workflows/Miri.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/Miri.yml b/.github/workflows/Miri.yml index d83a56a6..a4b2fd3f 100644 --- a/.github/workflows/Miri.yml +++ b/.github/workflows/Miri.yml @@ -29,7 +29,7 @@ jobs: uses: actions-rs/toolchain@v1 with: profile: minimal - toolchain: ${{ matrix.rust }} + toolchain: nightly override: true components: miri From e308e859625e349b1f101f8680f9b29db1273246 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Tue, 11 Jan 2022 17:31:46 +0800 Subject: [PATCH 5/6] Fix memory leak (#64) when unlinking a DeqNode for evicting/expiring an entry Add GitHub CI to run Miri test on the `deque` module. --- .github/workflows/Miri.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/Miri.yml b/.github/workflows/Miri.yml index a4b2fd3f..03fa7ba2 100644 --- a/.github/workflows/Miri.yml +++ b/.github/workflows/Miri.yml @@ -43,4 +43,5 @@ jobs: - name: Run Miri test (deque) uses: actions-rs/cargo@v1 with: - command: miri test deque + command: miri + args: test deque From 067187d5aae304520817d08eb12d54d3573898ff Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Tue, 11 Jan 2022 17:55:47 +0800 Subject: [PATCH 6/6] Fix memory leak when unlinking a DeqNode for evicting/expiring an entry Tweak a test case. --- src/common/deque.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/common/deque.rs b/src/common/deque.rs index e3997ca2..1bf9596c 100644 --- a/src/common/deque.rs +++ b/src/common/deque.rs @@ -698,8 +698,7 @@ mod tests { // ------------------------------------------------------- // Iterate after an unlink. // Unlink the second node "c". Now "a" -> "c". - unsafe { deque.unlink(node3_ptr) }; - std::mem::drop(unsafe { Box::from_raw(node3_ptr.as_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();