From 08047998c3e2d136a14b76a218073eef25f788d0 Mon Sep 17 00:00:00 2001 From: John-John Tedro Date: Mon, 25 Apr 2022 02:18:01 +0200 Subject: [PATCH 1/4] Add test for memory leak in PinSlab --- tests/pin_slab_memory_leak.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 tests/pin_slab_memory_leak.rs diff --git a/tests/pin_slab_memory_leak.rs b/tests/pin_slab_memory_leak.rs new file mode 100644 index 0000000..7ec6a38 --- /dev/null +++ b/tests/pin_slab_memory_leak.rs @@ -0,0 +1,17 @@ +use unicycle::pin_slab::PinSlab; + +struct Foo(u32); + +struct Bar(Vec); + +#[global_allocator] +static ALLOCATOR: checkers::Allocator = checkers::Allocator::system(); + +#[checkers::test] +fn test_pin_slab_memory_leak() { + let mut copy = PinSlab::new(); + copy.insert(Foo(42)); + + let mut non_copy = PinSlab::new(); + non_copy.insert(Bar(vec![42])); +} From dbf9d6d250ea4a8b3b79dc7d3b8ff48fb51118df Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 25 Apr 2022 11:45:11 -0700 Subject: [PATCH 2/4] Add unit test for running destructors in PinVec --- src/pin_vec.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/pin_vec.rs b/src/pin_vec.rs index 6e3c407..1cd2eef 100644 --- a/src/pin_vec.rs +++ b/src/pin_vec.rs @@ -106,6 +106,8 @@ const fn calculate_key(key: usize) -> (usize, usize, usize) { mod tests { use crate::pin_vec::calculate_key; + use super::PinVec; + #[test] fn key_test() { // NB: range of the first slot. @@ -121,4 +123,24 @@ mod tests { ); } } + + #[test] + fn run_destructors() { + let mut destructor_ran = false; + + struct RunDestructor<'a>(&'a mut bool); + impl<'a> Drop for RunDestructor<'a> { + fn drop(&mut self) { + *self.0 = true; + } + } + + { + // Make sure PinVec runs the destructors + let mut v = PinVec::new(); + v.push(RunDestructor(&mut destructor_ran)); + } + + assert!(destructor_ran); + } } From f98893385fd707800d77d0b2d3fd36ca11522533 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 25 Apr 2022 11:59:00 -0700 Subject: [PATCH 3/4] Run destructors when clearing PinVec Also adds a Drop impl for PinVec, and removes the Drop impl for PinSlab since PinSlab's Drop is redundant now. Fixes #20 --- src/pin_slab.rs | 6 ------ src/pin_vec.rs | 13 +++++++++++++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/pin_slab.rs b/src/pin_slab.rs index bb4e32f..87826c5 100644 --- a/src/pin_slab.rs +++ b/src/pin_slab.rs @@ -281,12 +281,6 @@ impl Default for PinSlab { } } -impl Drop for PinSlab { - fn drop(&mut self) { - self.clear(); - } -} - #[cfg(test)] mod tests { use super::PinSlab; diff --git a/src/pin_vec.rs b/src/pin_vec.rs index 1cd2eef..a3453e1 100644 --- a/src/pin_vec.rs +++ b/src/pin_vec.rs @@ -1,5 +1,6 @@ use std::mem::{self, MaybeUninit}; use std::ops::{Index, IndexMut}; +use std::ptr::drop_in_place; pub struct PinVec { // Slots of memory. Once one has been allocated it is never moved. @@ -22,6 +23,12 @@ impl PinVec { } pub fn clear(&mut self) { + for i in 0..self.len { + // Safety: we know the pointer is initialized because its index is in bounds, and it + // can be dropped because we are emptying the container, which means these contents + // will not be accessed again. + unsafe { drop_in_place(self.get_mut(i).unwrap()) } + } self.slots.clear(); self.len = 0; } @@ -66,6 +73,12 @@ impl PinVec { } } +impl Drop for PinVec { + fn drop(&mut self) { + self.clear(); + } +} + impl Index for PinVec { type Output = T; From bfb53227b1a9cbad9cbcdec62edc2807d029dc94 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 26 Apr 2022 14:47:56 -0700 Subject: [PATCH 4/4] Drop by slice, not one by one --- src/pin_vec.rs | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/pin_vec.rs b/src/pin_vec.rs index a3453e1..8d42418 100644 --- a/src/pin_vec.rs +++ b/src/pin_vec.rs @@ -23,13 +23,25 @@ impl PinVec { } pub fn clear(&mut self) { - for i in 0..self.len { - // Safety: we know the pointer is initialized because its index is in bounds, and it - // can be dropped because we are emptying the container, which means these contents - // will not be accessed again. - unsafe { drop_in_place(self.get_mut(i).unwrap()) } + if mem::needs_drop::() { + let (last_slot, offset, _) = calculate_key(self.len()); + for (i, mut slot) in self.slots.drain(..).enumerate() { + let slice: &mut [MaybeUninit] = if i < last_slot { + &mut *slot + } else { + &mut slot[0..offset] + }; + // Safety: we initialized slice to only point to the already-initialized elements. + // It's safe to drop_in_place because we are draining the Vec. + unsafe { + let slice: &mut [T] = mem::transmute(slice); + drop_in_place(slice); + } + } + } else { + self.slots.clear(); } - self.slots.clear(); + debug_assert_eq!(self.slots.len(), 0); self.len = 0; }