From bff26fcaa8d6db8bf88f3f3eb6ee689a93413ddd Mon Sep 17 00:00:00 2001 From: bluss Date: Thu, 20 Aug 2015 23:12:10 +0200 Subject: [PATCH 1/2] Panic safety test for SmallVec::drop A test for issue #14 --- lib.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lib.rs b/lib.rs index 46090a8..09869cd 100644 --- a/lib.rs +++ b/lib.rs @@ -578,4 +578,21 @@ pub mod tests { assert_eq!(&v.iter().map(|v| **v).collect::>(), &[0, 3, 2]); } + + #[test] + #[should_panic] + fn test_drop_panic_smallvec() { + // This test should only panic once, and not double panic, + // which would mean a double drop + struct DropPanic; + + impl Drop for DropPanic { + fn drop(&mut self) { + panic!("drop"); + } + } + + let mut v = SmallVec::<[_; 1]>::new(); + v.push(DropPanic); + } } From fbaf0959b9e90ccea3bdded90b85137bed96f5f1 Mon Sep 17 00:00:00 2001 From: bluss Date: Fri, 21 Aug 2015 15:11:53 +0200 Subject: [PATCH 2/2] Fix panic safety issue when an element panics in drop The summary is: SmallVec::drop first attempts to drop every element, then it inhibits the drop of the inner array. The panic safety issue is that a panic during drop of an element means the inhibition is never reached, so the inner data can be dropped again. If Drop is split betweeen SmallVec and SmallVecData, this issue is avoided because the SmallVecData drop will be called even in the panic case. This solution incurs the overhead of an additional drop flag on SmallVecData. Fixes #14 --- lib.rs | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/lib.rs b/lib.rs index 09869cd..2368c7d 100644 --- a/lib.rs +++ b/lib.rs @@ -83,6 +83,23 @@ enum SmallVecData { Heap { ptr: *mut A::Item, capacity: usize }, } +impl Drop for SmallVecData { + fn drop(&mut self) { + unsafe { + match *self { + ref mut inline @ Inline { .. } => { + // Inhibit the array destructor. + ptr::write(inline, Heap { + ptr: ptr::null_mut(), + capacity: 0, + }); + } + Heap { ptr, capacity } => deallocate(ptr, capacity), + } + } + } +} + pub struct SmallVec { len: usize, @@ -378,22 +395,13 @@ impl SmallVec { impl Drop for SmallVec { fn drop(&mut self) { + // Note on panic safety: dropping an element may panic, + // but the inner SmallVecData destructor will still run unsafe { let ptr = self.as_ptr(); for i in 0 .. self.len { ptr::read(ptr.offset(i as isize)); } - - match self.data { - Inline { .. } => { - // Inhibit the array destructor. - ptr::write(&mut self.data, Heap { - ptr: ptr::null_mut(), - capacity: 0, - }); - } - Heap { ptr, capacity } => deallocate(ptr, capacity), - } } } }