Merge pull request #6 from bluss/nodrop-restored

Use NoDrop to fix panic safety issues

.travis.yml

@@ -3,6 +3,7 @@ sudo: false
 matrix:
   include:
     - rust: stable
+    - rust: beta
     - rust: nightly
     - rust: nightly
       env:
@@ -11,6 +12,7 @@ script:
   - |
       [ -z "$NODROP_FEATURES" ] && cargo build --verbose --features "$FEATURES"
       [ -z "$NODROP_FEATURES" ] && cargo test --verbose --features "$FEATURES"
+      [ -z "$NODROP_FEATURES" ] && cargo test --release --verbose --features "$FEATURES"
       [ -z "$NODROP_FEATURES" ] && cargo bench --verbose --features "$FEATURES" -- --test
       [ -z "$NODROP_FEATURES" ] && cargo doc --verbose --features "$FEATURES"
       cargo test --verbose --manifest-path=nodrop/Cargo.toml --features "$NODROP_FEATURES"

Cargo.toml

@@ -13,6 +13,6 @@ keywords = ["stack", "vector", "array", "container", "data-structure"]
 [dependencies.odds]
 version = "0.2"
 
-[dev-dependencies]
+[dependencies.nodrop]
-nodrop = "0.1"
+version = "0.1.4"
-
+path = "nodrop"

Makefile

@@ -22,7 +22,6 @@ subst: $(DOCCRATES)
 
 mkdocs: Cargo.toml
 	cargo doc --features=$(FEATURES)
-	cargo doc -p nodrop
 	rm -rf ./doc
 	cp -r ./target/doc ./doc
 	-cat ./custom.css >> doc/main.css

src/lib.rs

@@ -1,4 +1,5 @@
 extern crate odds;
+extern crate nodrop;
 
 use std::iter;
 use std::mem;
@@ -9,13 +10,13 @@ use std::ops::{
 };
 use std::slice;
 
+use nodrop::NoDrop;
+
 // extra traits
 use std::borrow::{Borrow, BorrowMut};
 use std::hash::{Hash, Hasher};
 use std::fmt;
 
-use odds::debug_assert_unreachable;
-
 mod array;
 pub use array::Array;
 pub use odds::IndexRange as RangeArgument;
@@ -30,13 +31,6 @@ unsafe fn new_array<A: Array>() -> A {
     mem::uninitialized()
 }
 
-/// repr(u8) - Make sure the non-nullable pointer optimization does not occur!
-#[repr(u8)]
-enum NoDrop<T> {
-    Alive(T),
-    Dropped,
-}
-
 /// A vector with a fixed capacity.
 ///
 /// The **ArrayVec** is a vector backed by a fixed size array. It keeps track of
@@ -57,12 +51,12 @@ pub struct ArrayVec<A: Array> {
 impl<A: Array> Drop for ArrayVec<A> {
     fn drop(&mut self) {
         // clear all elements
-        while let Some(_) = self.pop() { }
+        while let Some(_) = self.pop() {
-
-        // inhibit drop
-        unsafe {
-            ptr::write(&mut self.xs, NoDrop::Dropped);
         }
+
+        // NoDrop inhibits array's drop
+        // panic safety: NoDrop::drop will trigger on panic, so the inner
+        // array will not drop even after panic.
     }
 }
 
@@ -84,7 +78,7 @@ impl<A: Array> ArrayVec<A> {
     /// ```
     pub fn new() -> ArrayVec<A> {
         unsafe {
-            ArrayVec { xs: NoDrop::Alive(new_array()), len: Index::zero() }
+            ArrayVec { xs: NoDrop::new(new_array()), len: Index::zero() }
         }
     }
 
@@ -375,31 +369,6 @@ impl<A: Array> DerefMut for ArrayVec<A> {
     }
 }
 
-impl<T> Deref for NoDrop<T> {
-    type Target = T;
-
-    // Use type invariant, always Alive.
-    #[inline]
-    fn deref(&self) -> &T {
-        match *self {
-            NoDrop::Alive(ref inner) => inner,
-            _ => unsafe { debug_assert_unreachable() }
-        }
-    }
-}
-
-impl<T> DerefMut for NoDrop<T> {
-    // Use type invariant, always Alive.
-    #[inline]
-    fn deref_mut(&mut self) -> &mut T {
-        match *self {
-            NoDrop::Alive(ref mut inner) => inner,
-            _ => unsafe { debug_assert_unreachable() }
-        }
-    }
-}
-
-
 /// Create an **ArrayVec** from an array.
 ///
 /// ## Examples
@@ -412,7 +381,7 @@ impl<T> DerefMut for NoDrop<T> {
 /// ```
 impl<A: Array> From<A> for ArrayVec<A> {
     fn from(array: A) -> Self {
-        ArrayVec { xs: NoDrop::Alive(array), len: Index::from(A::capacity()) }
+        ArrayVec { xs: NoDrop::new(array), len: Index::from(A::capacity()) }
     }
 }
 
@@ -524,10 +493,16 @@ impl<A: Array> ExactSizeIterator for IntoIter<A> { }
 
 impl<A: Array> Drop for IntoIter<A> {
     fn drop(&mut self) {
-        // exhaust iterator and clear the vector
+        // panic safety: Set length to 0 before dropping elements.
-        while let Some(_) = self.next() { }
+        let index = self.index.to_usize();
+        let len = self.v.len();
         unsafe {
             self.v.set_len(0);
+            let elements = slice::from_raw_parts(self.v.get_unchecked_mut(index),
+                                                 len - index);
+            for elt in elements {
+                ptr::read(elt);
+            }
         }
     }
 }
@@ -588,6 +563,8 @@ impl<'a, A: Array> Drop for Drain<'a, A>
     where A::Item: 'a
 {
     fn drop(&mut self) {
+        // len is currently 0 so panicking while dropping will not cause a double drop.
+
         // exhaust self first
         while let Some(_) = self.next() { }
 

tests/tests.rs

@@ -130,12 +130,12 @@ fn test_compact_size() {
     // 4 elements size + 1 len + 1 enum tag + [1 drop flag]
     type ByteArray = ArrayVec<[u8; 4]>;
     println!("{}", mem::size_of::<ByteArray>());
-    assert!(mem::size_of::<ByteArray>() <= 7);
+    assert!(mem::size_of::<ByteArray>() <= 8);
 
     // 12 element size + 1 enum tag + 3 padding + 1 len + 1 drop flag + 2 padding
     type QuadArray = ArrayVec<[u32; 3]>;
     println!("{}", mem::size_of::<QuadArray>());
-    assert!(mem::size_of::<QuadArray>() <= 20);
+    assert!(mem::size_of::<QuadArray>() <= 24);
 }
 
 #[test]
@@ -163,6 +163,37 @@ fn test_drain_oob() {
     v.drain(0..8);
 }
 
+#[test]
+#[should_panic]
+fn test_drop_panic() {
+    struct DropPanic;
+
+    impl Drop for DropPanic {
+        fn drop(&mut self) {
+            panic!("drop");
+        }
+    }
+
+    let mut array = ArrayVec::<[DropPanic; 1]>::new();
+    array.push(DropPanic);
+}
+
+#[test]
+#[should_panic]
+fn test_drop_panic_into_iter() {
+    struct DropPanic;
+
+    impl Drop for DropPanic {
+        fn drop(&mut self) {
+            panic!("drop");
+        }
+    }
+
+    let mut array = ArrayVec::<[DropPanic; 1]>::new();
+    array.push(DropPanic);
+    array.into_iter();
+}
+
 #[test]
 fn test_insert() {
     let mut v = ArrayVec::from([]);