From d52989a949e9eb977ed1f2a6f19490dfaf066d2b Mon Sep 17 00:00:00 2001 From: CAD97 Date: Wed, 18 Mar 2020 19:34:19 -0400 Subject: [PATCH 1/6] Add failing leak detection test --- crates/slice-dst/tests/leak.rs | 68 ++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 crates/slice-dst/tests/leak.rs diff --git a/crates/slice-dst/tests/leak.rs b/crates/slice-dst/tests/leak.rs new file mode 100644 index 0000000..5d1825d --- /dev/null +++ b/crates/slice-dst/tests/leak.rs @@ -0,0 +1,68 @@ +use { + slice_dst::*, + std::{ + panic, + sync::atomic::{AtomicUsize, Ordering::SeqCst}, + }, +}; + +struct DropTracking<'a> { + place: &'a AtomicUsize, +} + +impl<'a> DropTracking<'a> { + fn new(place: &'a AtomicUsize) -> Self { + place.fetch_add(1, SeqCst); + DropTracking { place } + } +} + +impl Drop for DropTracking<'_> { + fn drop(&mut self) { + self.place.fetch_sub(1, SeqCst); + } +} + +#[test] +#[cfg_attr( + all(miri, target_os = "windows"), + ignore = "miri does not support panicking on windows rust-lang/miri#1059" +)] +fn bad_exactsizeiterator() { + struct Iter<'a> { + counter: &'a AtomicUsize, + len: usize, + } + + impl ExactSizeIterator for Iter<'_> { + fn len(&self) -> usize { + self.len + } + } + + impl<'a> Iterator for Iter<'a> { + type Item = DropTracking<'a>; + + fn next(&mut self) -> Option { + match self.len { + 0 | 1 => None, + _ => { + self.len -= 1; + Some(DropTracking::new(self.counter)) + } + } + } + } + + let mut counter = AtomicUsize::new(0); + let _ = std::panic::catch_unwind(|| { + let _: Box<_> = SliceWithHeader::new::, _>( + (), + Iter { + counter: &counter, + len: 5, + }, + ); + }); + assert_eq!(*counter.get_mut(), 0); +} From 9111999427ca58ff4da77b47903627da089cf2b9 Mon Sep 17 00:00:00 2001 From: CAD97 Date: Wed, 18 Mar 2020 20:01:44 -0400 Subject: [PATCH 2/6] Clean up leak on bad ExactSizeIter impl --- crates/slice-dst/src/lib.rs | 117 +++++++++++++++++++++++++++--------- 1 file changed, 88 insertions(+), 29 deletions(-) diff --git a/crates/slice-dst/src/lib.rs b/crates/slice-dst/src/lib.rs index a3798cf..31491f2 100644 --- a/crates/slice-dst/src/lib.rs +++ b/crates/slice-dst/src/lib.rs @@ -88,7 +88,10 @@ extern crate alloc; -use core::ptr::NonNull; +#[cfg(has_ptr_slice_from_raw_parts)] +use core::ptr::slice_from_raw_parts_mut as slice_from_raw_parts; +#[cfg(not(has_ptr_slice_from_raw_parts))] +use core::slice::from_raw_parts_mut as slice_from_raw_parts; #[cfg(feature = "erasable")] use erasable::{Erasable, ErasedPtr}; use { @@ -96,7 +99,7 @@ use { alloc::{alloc, handle_alloc_error}, boxed::Box, }, - core::{alloc::Layout, ptr, slice}, + core::{alloc::Layout, mem::ManuallyDrop, ptr, slice}, }; /// A custom slice-based dynamically sized type. @@ -221,7 +224,7 @@ unsafe impl SliceDst for SliceWithHeader { Self::layout(len).0 } - fn retype(ptr: NonNull<[()]>) -> NonNull { + fn retype(ptr: ptr::NonNull<[()]>) -> ptr::NonNull { unsafe { ptr::NonNull::new_unchecked(ptr.as_ptr() as *mut _) } } } @@ -246,30 +249,91 @@ impl SliceWithHeader { I: IntoIterator, I::IntoIter: ExactSizeIterator, { - let mut items = items.into_iter(); + let items = items.into_iter(); let len = items.len(); - let (layout, [length_offset, header_offset, slice_offset]) = Self::layout(len); - - unsafe { - A::new_slice_dst(len, |ptr| { - let raw = ptr.as_ptr().cast::(); - ptr::write(raw.add(length_offset).cast(), len); - ptr::write(raw.add(header_offset).cast(), header); - let mut slice_ptr = raw.add(slice_offset).cast::(); - for _ in 0..len { - let item = items - .next() - .expect("ExactSizeIterator over-reported length"); - ptr::write(slice_ptr, item); - slice_ptr = slice_ptr.offset(1); + + struct InProgress { + raw: ptr::NonNull>, + written: usize, + layout: Layout, + length_offset: usize, + header_offset: usize, + slice_offset: usize, + } + + impl Drop for InProgress { + fn drop(&mut self) { + unsafe { + ptr::drop_in_place(slice_from_raw_parts( + self.raw().add(self.slice_offset).cast::(), + self.written, + )); } - assert!( - items.next().is_none(), - "ExactSizeIterator under-reported length" - ); - assert_eq!(layout, Layout::for_value(ptr.as_ref())); - }) + } } + + impl InProgress { + fn init( + len: usize, + header: Header, + mut items: impl Iterator + ExactSizeIterator, + ) -> impl FnOnce(ptr::NonNull>) { + move |ptr| { + let mut this = Self::new(len, ptr); + + unsafe { + for _ in 0..len { + let item = items + .next() + .expect("ExactSizeIterator over-reported length"); + this.push(item); + } + + assert!( + items.next().is_none(), + "ExactSizeIterator under-reported length" + ); + + this.finish(len, header) + } + } + } + + fn raw(&self) -> *mut u8 { + self.raw.as_ptr().cast() + } + + fn new(len: usize, raw: ptr::NonNull>) -> Self { + let (layout, [length_offset, header_offset, slice_offset]) = + SliceWithHeader::::layout(len); + InProgress { + raw, + written: 0, + layout, + length_offset, + header_offset, + slice_offset, + } + } + + unsafe fn push(&mut self, item: Item) { + self.raw() + .add(self.slice_offset) + .cast::() + .add(self.written) + .write(item); + self.written += 1; + } + + unsafe fn finish(self, len: usize, header: Header) { + let this = ManuallyDrop::new(self); + ptr::write(this.raw().add(this.length_offset).cast(), len); + ptr::write(this.raw().add(this.header_offset).cast(), header); + debug_assert_eq!(this.layout, Layout::for_value(this.raw.as_ref())) + } + } + + unsafe { A::new_slice_dst(len, InProgress::init(len, header, items)) } } } @@ -286,11 +350,6 @@ where #[cfg(feature = "erasable")] unsafe impl Erasable for SliceWithHeader { unsafe fn unerase(this: ErasedPtr) -> ptr::NonNull { - #[cfg(not(has_ptr_slice_from_raw_parts))] - let slice_from_raw_parts = slice::from_raw_parts_mut::<()>; - #[cfg(has_ptr_slice_from_raw_parts)] - let slice_from_raw_parts = ptr::slice_from_raw_parts_mut::<()>; - let len: usize = ptr::read(this.as_ptr().cast()); let raw = ptr::NonNull::new_unchecked(slice_from_raw_parts(this.as_ptr().cast(), len)); Self::retype(raw) From faedd306627657e5a821f3df1ff7f3653dfb9af4 Mon Sep 17 00:00:00 2001 From: CAD97 Date: Wed, 18 Mar 2020 20:03:16 -0400 Subject: [PATCH 3/6] Also check head for leak --- crates/slice-dst/tests/leak.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/slice-dst/tests/leak.rs b/crates/slice-dst/tests/leak.rs index 5d1825d..94ce62f 100644 --- a/crates/slice-dst/tests/leak.rs +++ b/crates/slice-dst/tests/leak.rs @@ -57,7 +57,7 @@ fn bad_exactsizeiterator() { let mut counter = AtomicUsize::new(0); let _ = std::panic::catch_unwind(|| { let _: Box<_> = SliceWithHeader::new::, _>( - (), + DropTracking::new(&counter), Iter { counter: &counter, len: 5, From 3f01453af344fcc05db1fc2f89c4edd1e50599e0 Mon Sep 17 00:00:00 2001 From: CAD97 Date: Wed, 18 Mar 2020 20:11:34 -0400 Subject: [PATCH 4/6] Use miri to test for leaks on init panic --- crates/slice-dst/tests/leak.rs | 39 ++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/crates/slice-dst/tests/leak.rs b/crates/slice-dst/tests/leak.rs index 94ce62f..fca52bb 100644 --- a/crates/slice-dst/tests/leak.rs +++ b/crates/slice-dst/tests/leak.rs @@ -1,8 +1,13 @@ use { slice_dst::*, std::{ - panic, - sync::atomic::{AtomicUsize, Ordering::SeqCst}, + alloc::Layout, + panic, ptr, + rc::Rc, + sync::{ + atomic::{AtomicUsize, Ordering::SeqCst}, + Arc, + }, }, }; @@ -66,3 +71,33 @@ fn bad_exactsizeiterator() { }); assert_eq!(*counter.get_mut(), 0); } + +struct S(u8); + +unsafe impl SliceDst for S { + fn layout_for(_: usize) -> Layout { + Layout::new::() + } + + fn retype(ptr: ptr::NonNull<[()]>) -> ptr::NonNull { + ptr.cast() + } +} + +#[test] +#[cfg_attr( + all(miri, target_os = "windows"), + ignore = "miri does not support panicking on windows rust-lang/miri#1059" +)] +fn panic_in_init() { + // This relies on miri to catch leaks + let _ = std::panic::catch_unwind(|| { + let _: Box = unsafe { AllocSliceDst::new_slice_dst(0, |_| panic!()) }; + }); + let _ = std::panic::catch_unwind(|| { + let _: Arc = unsafe { AllocSliceDst::new_slice_dst(0, |_| panic!()) }; + }); + let _ = std::panic::catch_unwind(|| { + let _: Rc = unsafe { AllocSliceDst::new_slice_dst(0, |_| panic!()) }; + }); +} From 986d94de2eed40a80ad6cb9bfd1189e81f51ba40 Mon Sep 17 00:00:00 2001 From: CAD97 Date: Wed, 18 Mar 2020 20:20:02 -0400 Subject: [PATCH 5/6] Plug leak on init panic --- crates/slice-dst/src/lib.rs | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/crates/slice-dst/src/lib.rs b/crates/slice-dst/src/lib.rs index 31491f2..7d06d01 100644 --- a/crates/slice-dst/src/lib.rs +++ b/crates/slice-dst/src/lib.rs @@ -96,7 +96,7 @@ use core::slice::from_raw_parts_mut as slice_from_raw_parts; use erasable::{Erasable, ErasedPtr}; use { alloc::{ - alloc::{alloc, handle_alloc_error}, + alloc::{alloc, dealloc, handle_alloc_error}, boxed::Box, }, core::{alloc::Layout, mem::ManuallyDrop, ptr, slice}, @@ -198,9 +198,31 @@ unsafe impl AllocSliceDst for Box { where I: FnOnce(ptr::NonNull), { - let ptr = alloc_slice_dst(len); - init(ptr); - Box::from_raw(ptr.as_ptr()) + struct RawBox(ptr::NonNull, Layout); + + impl RawBox { + unsafe fn new(len: usize) -> Self { + let layout = S::layout_for(len); + RawBox(alloc_slice_dst(len), layout) + } + + unsafe fn finalize(self) -> Box { + let this = ManuallyDrop::new(self); + Box::from_raw(this.0.as_ptr()) + } + } + + impl Drop for RawBox { + fn drop(&mut self) { + unsafe { + dealloc(self.0.as_ptr().cast(), self.1); + } + } + } + + let ptr = RawBox::new(len); + init(ptr.0); + ptr.finalize() } } From b6e4ed419c00b6ffc3dbe25b39e6642e18ce8efe Mon Sep 17 00:00:00 2001 From: CAD97 Date: Wed, 18 Mar 2020 20:20:50 -0400 Subject: [PATCH 6/6] Gate bors on Miri --- bors.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/bors.toml b/bors.toml index 2e90e3b..e799c0b 100644 --- a/bors.toml +++ b/bors.toml @@ -3,6 +3,7 @@ delete-merged-branches = true status = [ "Clippy", + "Miri", "MSRV Tests", "Rustfmt", "Tests",