0 9.1 patch (#45)

* Fixed String::retain, RVec::retain. Bumped patch version to 0.9.1 .

These methods copied their implementation from the standard library,
which had memory safety bugs discovered in
rust-lang/rust#60977 and rust-lang/rust#78498 .

This bug was reported in #44 .

Added adapted tests from std which test these bugs.

* Updated changelog for patch

Changelog.md

@@ -2,6 +2,10 @@ This is the changelog,summarising changes in each version(some minor changes may
 
 # 0.9
 
+# 0.9.1
+
+Fixed a memory safety bug in RString::retain and RVec::retain.
+
 # 0.9.0
 
 Rewrote how prefix types work. now they aren't by reference, 

abi_stable/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "abi_stable"
-version = "0.9.0"
+version = "0.9.1"
 authors = ["rodrimati1992 <rodrimatt1985@gmail.com>"]
 edition="2018"
 license = "MIT/Apache-2.0"

abi_stable/src/std_types/string.rs

@@ -22,7 +22,8 @@ use crate::std_types::{RStr, RVec};
 
 mod iters;
 
-#[cfg(all(test,not(feature="only_new_tests")))]
+#[cfg(test)]
+// #[cfg(all(test,not(feature="only_new_tests")))]
 mod tests;
 
 pub use self::iters::{Drain, IntoIter};
@@ -643,10 +644,12 @@ impl RString {
         let mut del_bytes = 0;
         let mut idx = 0;
 
+        unsafe {
+            self.inner.set_len(0);
+        }
+
         while idx < len {
-            let ch = unsafe {
-                self.get_unchecked(idx..len).chars().next().unwrap()
-            };
+            let ch = unsafe { self.get_unchecked(idx..len).chars().next().unwrap() };
             let ch_len = ch.len_utf8();
 
             if !pred(ch) {
@@ -656,16 +659,17 @@ impl RString {
                     ptr::copy(
                         self.inner.as_ptr().add(idx),
                         self.inner.as_mut_ptr().add(idx - del_bytes),
-                        ch_len
+                        ch_len,
                     );
                 }
             }
 
+            // Point idx to the next char
             idx += ch_len;
         }
 
-        if del_bytes > 0 {
-            unsafe { self.inner.set_len(len - del_bytes); }
+        unsafe {
+            self.inner.set_len(len - del_bytes);
         }
     }
 

abi_stable/src/std_types/string/tests.rs

@@ -201,7 +201,23 @@ fn retain(){
 
         assert_eq!(&*rstr, &*string);
     }
-
+    {
+        // Copied from:
+        // https://github.com/rust-lang/rust/blob/48c4afbf9c29880dd946067d1c9aee1e7f75834a/library/alloc/tests/string.rs#L383    
+        let mut s = RString::from("0è0");
+        let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+            let mut count = 0;
+            s.retain(|_| {
+                count += 1;
+                match count {
+                    1 => false,
+                    2 => true,
+                    _ => panic!(),
+                }
+            });
+        }));
+        assert!(std::str::from_utf8(s.as_bytes()).is_ok());
+    }
 }
 
 

abi_stable/src/std_types/vec.rs

@@ -812,6 +812,7 @@ impl<T> RVec<T> {
             del: 0,
             old_len,
             pred: |x| !pred(x),
+            panic_flag: false,
         };
     }
 

abi_stable/src/std_types/vec/iters.rs

@@ -277,7 +277,8 @@ impl<'a, T> Drop for Drain<'a, T> {
 
 
 
-// copy-paste of the std library DrainFilter
+// copy of the std library DrainFilter, without the allocator parameter.
+// (from rustc 1.50.0-nightly (eb4fc71dc 2020-12-17))
 #[derive(Debug)]
 pub(crate) struct DrainFilter<'a, T, F>
     where F: FnMut(&mut T) -> bool,
@@ -287,20 +288,30 @@ pub(crate) struct DrainFilter<'a, T, F>
     pub(super) del: usize,
     pub(super) old_len: usize,
     pub(super) pred: F,
+    pub(super) panic_flag: bool,
 }
 
+// copy of the std library DrainFilter impl, without the allocator parameter.
+// (from rustc 1.50.0-nightly (eb4fc71dc 2020-12-17))
 impl<T, F> Iterator for DrainFilter<'_, T, F>
-    where F: FnMut(&mut T) -> bool,
+where
+    F: FnMut(&mut T) -> bool,
 {
     type Item = T;
 
     fn next(&mut self) -> Option<T> {
         unsafe {
-            while self.idx != self.old_len {
+            while self.idx < self.old_len {
                 let i = self.idx;
-                self.idx += 1;
                 let v = slice::from_raw_parts_mut(self.vec.as_mut_ptr(), self.old_len);
-                if (self.pred)(&mut v[i]) {
+                self.panic_flag = true;
+                let drained = (self.pred)(&mut v[i]);
+                self.panic_flag = false;
+                // Update the index *after* the predicate is called. If the index
+                // is updated prior and the predicate panics, the element at this
+                // index would be leaked.
+                self.idx += 1;
+                if drained {
                     self.del += 1;
                     return Some(ptr::read(&v[i]));
                 } else if self.del > 0 {
@@ -319,14 +330,51 @@ impl<T, F> Iterator for DrainFilter<'_, T, F>
     }
 }
 
+// copy of the std library DrainFilter impl, without the allocator parameter.
+// (from rustc 1.50.0-nightly (eb4fc71dc 2020-12-17))
 impl<T, F> Drop for DrainFilter<'_, T, F>
-    where F: FnMut(&mut T) -> bool,
+where
+    F: FnMut(&mut T) -> bool,
 {
     fn drop(&mut self) {
-        self.for_each(drop);
-        unsafe {
-            self.vec.set_len(self.old_len - self.del);
+        struct BackshiftOnDrop<'a, 'b, T, F>
+        where
+            F: FnMut(&mut T) -> bool,
+        {
+            drain: &'b mut DrainFilter<'a, T, F>,
+        }
+
+        impl<'a, 'b, T, F> Drop for BackshiftOnDrop<'a, 'b, T, F>
+        where
+            F: FnMut(&mut T) -> bool,
+        {
+            fn drop(&mut self) {
+                unsafe {
+                    if self.drain.idx < self.drain.old_len && self.drain.del > 0 {
+                        // This is a pretty messed up state, and there isn't really an
+                        // obviously right thing to do. We don't want to keep trying
+                        // to execute `pred`, so we just backshift all the unprocessed
+                        // elements and tell the vec that they still exist. The backshift
+                        // is required to prevent a double-drop of the last successfully
+                        // drained item prior to a panic in the predicate.
+                        let ptr = self.drain.vec.as_mut_ptr();
+                        let src = ptr.add(self.drain.idx);
+                        let dst = src.sub(self.drain.del);
+                        let tail_len = self.drain.old_len - self.drain.idx;
+                        src.copy_to(dst, tail_len);
+                    }
+                    self.drain.vec.set_len(self.drain.old_len - self.drain.del);
+                }
+            }
+        }
+
+        let backshift = BackshiftOnDrop { drain: self };
+
+        // Attempt to consume any remaining elements if the filter predicate
+        // has not yet panicked. We'll backshift any remaining elements
+        // whether we've already panicked or if the consumption here panics.
+        if !backshift.drain.panic_flag {
+            backshift.drain.for_each(drop);
         }
     }
 }
-

abi_stable/src/std_types/vec/tests.rs

@@ -165,7 +165,7 @@ fn truncate() {
 
 #[test]
 fn retain(){
-    let orig = vec![2, 3, 4 , 5, 6,7,8];
+    let orig = vec![2, 3, 4, 5, 6, 7, 8];
     let copy = orig.clone().into_(RVec::T);
     {
         let mut copy=copy.clone();
@@ -235,7 +235,7 @@ fn retain(){
                 true
             });
         }).unwrap();
-        assert_eq!(&copy[..], <&[i32]>::default());
+        assert_eq!(&copy[..], &orig[..]);
     }
 }
 
@@ -397,4 +397,56 @@ fn rvec_macro(){
     assert_eq!(RVec::from(vec![0,3]), rvec![0,3]);
     assert_eq!(RVec::from(vec![0,3,6]), rvec![0,3,6]);
     assert_eq!(RVec::from(vec![1;10]), rvec![1;10]);
-}
+}
+
+// Adapted from Vec tests 
+// (from rustc 1.50.0-nightly (eb4fc71dc 2020-12-17))
+#[test]
+fn retain_panic() {
+    use std::rc::Rc;
+    use std::sync::Mutex;
+    use std::panic::AssertUnwindSafe;
+
+    struct Check {
+        index: usize,
+        drop_counts: Rc<Mutex<RVec<usize>>>,
+    }
+
+    impl Drop for Check {
+        fn drop(&mut self) {
+            self.drop_counts.lock().unwrap()[self.index] += 1;
+            println!("drop: {}", self.index);
+        }
+    }
+
+    let check_count = 10;
+    let drop_counts = Rc::new(Mutex::new(rvec![0_usize; check_count]));
+    let mut data: RVec<Check> = (0..check_count)
+        .map(|index| Check { index, drop_counts: Rc::clone(&drop_counts) })
+        .collect();
+
+    let _ = std::panic::catch_unwind(AssertUnwindSafe(move || {
+        let filter = |c: &Check| {
+            if c.index == 2 {
+                panic!("panic at index: {}", c.index);
+            }
+            // Verify that if the filter could panic again on another element
+            // that it would not cause a double panic and all elements of the
+            // vec would still be dropped exactly once.
+            if c.index == 4 {
+                panic!("panic at index: {}", c.index);
+            }
+            c.index < 6
+        };
+        data.retain(filter);
+    }));
+
+    let drop_counts = drop_counts.lock().unwrap();
+    assert_eq!(check_count, drop_counts.len());
+
+    for (index, count) in drop_counts.iter().cloned().enumerate() {
+        assert_eq!(1, count, "unexpected drop count at index: {} (count: {})", index, count);
+    }
+}
+
+