Soundness fixes for RUSTSEC-2020-0041

Cargo.toml

@@ -15,10 +15,6 @@ exclude = ["release.toml", "proptest-regressions/**"]
 [package.metadata.docs.rs]
 all-features = true
 
-[[bench]]
-name = "sized_chunk"
-harness = false
-
 [features]
 default = ["std"]
 std = []
@@ -30,6 +26,3 @@ bitmaps = "2.0.0"
 array-ops = { version = "0.1", optional = true }
 refpool = { version = "0.4", optional = true }
 arbitrary = { version = "0.4", optional = true }
-
-[dev-dependencies]
-criterion = "0.3"

src/sized_chunk/mod.rs

@@ -124,11 +124,12 @@ where
 {
     fn clone(&self) -> Self {
         let mut out = Self::new();
-        out.left = self.left;
-        out.right = self.right;
         for index in self.left..self.right {
             unsafe { Chunk::force_write(index, (*self.ptr(index)).clone(), &mut out) }
         }
+        // must be set after writes for panic safety
+        out.left = self.left;
+        out.right = self.right;
         out
     }
 }
@@ -151,6 +152,7 @@ where
 
     /// Construct a new chunk with one item.
     pub fn unit(value: A) -> Self {
+        assert!(Self::CAPACITY >= 1, "capacity");
         let mut chunk = Self {
             left: 0,
             right: 1,
@@ -164,6 +166,7 @@ where
 
     /// Construct a new chunk with two items.
     pub fn pair(left: A, right: A) -> Self {
+        assert!(Self::CAPACITY >= 2, "capacity");
         let mut chunk = Self {
             left: 0,
             right: 2,
@@ -257,6 +260,7 @@ where
         (&self.data as *const _ as *const A).add(index)
     }
 
+    /// It has no bounds checks
     #[inline]
     unsafe fn mut_ptr(&mut self, index: usize) -> *mut A {
         (&mut self.data as *mut _ as *mut A).add(index)
@@ -268,7 +272,8 @@ where
         chunk.ptr(index).read()
     }
 
-    /// Write a value at an index without trying to drop what's already there
+    /// Write a value at an index without trying to drop what's already there.
+    /// It has no bounds checks.
     #[inline]
     unsafe fn force_write(index: usize, value: A, chunk: &mut Self) {
         chunk.mut_ptr(index).write(value)
@@ -568,6 +573,7 @@ where
     {
         let iter = iter.into_iter();
         let insert_size = iter.len();
+        let iter = iter.take(insert_size);
         if self.len() + insert_size > Self::CAPACITY {
             panic!(
                 "Chunk::insert_from: chunk cannot fit {} elements",
@@ -583,37 +589,42 @@ where
         if self.right == N::USIZE || (self.left >= insert_size && left_size < right_size) {
             unsafe {
                 Chunk::force_copy(self.left, self.left - insert_size, left_size, self);
-                let mut write_index = real_index - insert_size;
+                let write_index_start = real_index - insert_size;
+                let mut written = 0;
                 for value in iter {
-                    Chunk::force_write(write_index, value, self);
-                    write_index += 1;
+                    Chunk::force_write(write_index_start + written, value, self);
+                    written += 1;
                 }
+                // update after writes for panic safety
+                // update actual number written in case iterator stopped earlier than promised
+                self.left -= written;
             }
-            self.left -= insert_size;
         } else if self.left == 0 || (self.right + insert_size <= Self::CAPACITY) {
             unsafe {
                 Chunk::force_copy(real_index, real_index + insert_size, right_size, self);
                 let mut write_index = real_index;
                 for value in iter {
                     Chunk::force_write(write_index, value, self);
                     write_index += 1;
+                    self.right += 1; // don't trust iterator length, which may be less than insert_size
                 }
             }
-            self.right += insert_size;
         } else {
             unsafe {
                 Chunk::force_copy(self.left, 0, left_size, self);
                 Chunk::force_copy(real_index, left_size + insert_size, right_size, self);
+                // order is important for panic safety
+                self.right -= self.left;
+                self.left = 0;
                 let mut write_index = left_size;
                 for value in iter {
                     Chunk::force_write(write_index, value, self);
                     write_index += 1;
+                    self.right += 1; // don't trust iterator length, which may be less than insert_size
                 }
             }
-            self.right -= self.left;
-            self.right += insert_size;
-            self.left = 0;
         }
+        debug_assert!(self.right >= self.left);
     }
 
     /// Remove the value at index `index`, shifting all the following values to
@@ -808,6 +819,8 @@ where
 {
     #[inline]
     fn from(mut array: InlineArray<A, T>) -> Self {
+        // capacity comparison is to help optimize it out
+        assert!(Self::CAPACITY >= InlineArray::<A, T>::CAPACITY || Self::CAPACITY >= array.len(), "capacity");
         Self::from(&mut array)
     }
 }
@@ -976,6 +989,187 @@ where
 #[cfg(test)]
 mod test {
     use super::*;
+    use typenum::{U0, U1, U2, U3, U5};
+
+    #[test]
+    #[should_panic(expected = "capacity")]
+    fn issue_11_testcase1a() {
+        let _ = Chunk::<usize, U0>::unit(123);
+    }
+
+    #[test]
+    #[should_panic(expected = "capacity")]
+    fn issue_11_testcase1b() {
+        let _ = Chunk::<usize, U0>::pair(123, 456);
+    }
+
+    #[test]
+    #[should_panic(expected = "capacity")]
+    fn issue_11_testcase1c() {
+        let _ = Chunk::<usize, U1>::pair(123, 456);
+    }
+
+    #[test]
+    #[should_panic(expected = "Chunk::push_back: can't push to full chunk")]
+    fn issue_11_testcase1d() {
+        let mut chunk = Chunk::<usize, U2>::pair(123, 456);
+        chunk.push_back(789);
+    }
+
+    #[test]
+    #[should_panic(expected = "capacity")]
+    fn issue_11_testcase2a() {
+        let mut from = InlineArray::<u8, [u8; 256]>::new();
+        from.push(1);
+
+        let _ = Chunk::<u8, U0>::from(from);
+    }
+
+    #[test]
+    fn issue_11_testcase2b() {
+        let mut from = InlineArray::<u8, [u8; 256]>::new();
+        from.push(1);
+
+        let _ = Chunk::<u8, U1>::from(from);
+    }
+
+    struct DropDetector(u32);
+
+    impl DropDetector {
+        fn new(num: u32) -> Self {
+            println!("Creating {}", num);
+            DropDetector(num)
+        }
+    }
+
+    impl Drop for DropDetector {
+        fn drop(&mut self) {
+            println!("Dropping {}", self.0);
+        }
+    }
+
+    impl Clone for DropDetector {
+        fn clone(&self) -> Self {
+            if self.0 == 42 {
+                panic!("panic on clone")
+            }
+            DropDetector::new(self.0)
+        }
+    }
+
+    /// This is for miri to catch
+    #[test]
+    fn issue_11_testcase3a() {
+        let mut chunk = Chunk::<DropDetector, U3>::new();
+        chunk.push_back(DropDetector::new(42));
+        chunk.push_back(DropDetector::new(43));
+
+        let _ = std::panic::catch_unwind(|| {
+            let _ = chunk.clone();
+        });
+    }
+
+
+    struct PanickingIterator {
+        current: u32,
+        panic_at: u32,
+        len: usize,
+    }
+
+    impl Iterator for PanickingIterator {
+        type Item = DropDetector;
+
+        fn next(&mut self) -> Option<Self::Item> {
+            let num = self.current;
+
+            if num == self.panic_at {
+                panic!("panicking index")
+            }
+
+            self.current += 1;
+            Some(DropDetector::new(num))
+        }
+
+        fn size_hint(&self) -> (usize, Option<usize>) {
+            (self.len, Some(self.len))
+        }
+    }
+
+    impl ExactSizeIterator for PanickingIterator {}
+
+    #[test]
+    fn issue_11_testcase3b() {
+        let _ = std::panic::catch_unwind(|| {
+            let mut chunk = Chunk::<DropDetector, U5>::new();
+            chunk.push_back(DropDetector::new(1));
+            chunk.push_back(DropDetector::new(2));
+            chunk.push_back(DropDetector::new(3));
+
+            chunk.insert_from(
+                1,
+                PanickingIterator {
+                    current: 1,
+                    panic_at: 1,
+                    len: 1,
+                },
+            );
+        });
+    }
+
+    struct FakeSizeIterator { reported: usize, actual: usize }
+    impl Iterator for FakeSizeIterator {
+        type Item = u8;
+        fn next(&mut self) -> Option<Self::Item> {
+            if self.actual == 0 {
+                None
+            } else {
+                self.actual -= 1;
+                Some(1)
+            }
+        }
+
+        fn size_hint(&self) -> (usize, Option<usize>) {
+            (self.reported, Some(self.reported))
+        }
+    }
+
+    impl ExactSizeIterator for FakeSizeIterator {
+        fn len(&self) -> usize {
+            self.reported
+        }
+    }
+
+    #[test]
+    fn iterator_too_long() {
+        let mut chunk = Chunk::<u8, U5>::new();
+        chunk.push_back(0);
+        chunk.push_back(1);
+        chunk.push_back(2);
+        chunk.insert_from(1, FakeSizeIterator { reported: 1, actual: 10 });
+
+        let mut chunk = Chunk::<u8, U5>::new();
+        chunk.push_back(1);
+        chunk.insert_from(0, FakeSizeIterator { reported: 1, actual: 10 });
+
+        let mut chunk = Chunk::<u8, U5>::new();
+        chunk.insert_from(0, FakeSizeIterator { reported: 1, actual: 10 });
+    }
+
+    #[test]
+    fn iterator_too_short() {
+        let mut chunk = Chunk::<u8, U5>::new();
+        chunk.push_back(0);
+        chunk.push_back(1);
+        chunk.push_back(2);
+        chunk.insert_from(1, FakeSizeIterator { reported: 2, actual: 0 });
+
+        let mut chunk = Chunk::<u8, U5>::new();
+        chunk.push_back(1);
+        chunk.insert_from(1, FakeSizeIterator { reported: 4, actual: 0 });
+
+        let mut chunk = Chunk::<u8, U5>::new();
+        chunk.insert_from(0, FakeSizeIterator { reported: 4, actual: 0 });
+    }
 
     #[test]
     fn is_full() {