From 5b941e0010fddea2974bdb9adb6ef335be819df3 Mon Sep 17 00:00:00 2001 From: rodrimati1992 Date: Mon, 21 Dec 2020 23:22:18 -0300 Subject: [PATCH] 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 https://github.com/rust-lang/rust/issues/60977 and https://github.com/rust-lang/rust/issues/78498 . This bug was reported in https://github.com/rodrimati1992/abi_stable_crates/issues/44 . Added adapted tests from std which test these bugs. * Updated changelog for patch --- Changelog.md | 4 ++ abi_stable/Cargo.toml | 2 +- abi_stable/src/std_types/string.rs | 18 ++++--- abi_stable/src/std_types/string/tests.rs | 18 ++++++- abi_stable/src/std_types/vec.rs | 1 + abi_stable/src/std_types/vec/iters.rs | 68 ++++++++++++++++++++---- abi_stable/src/std_types/vec/tests.rs | 58 ++++++++++++++++++-- 7 files changed, 147 insertions(+), 22 deletions(-) diff --git a/Changelog.md b/Changelog.md index 6ffc86e6..89183b6c 100644 --- a/Changelog.md +++ b/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, diff --git a/abi_stable/Cargo.toml b/abi_stable/Cargo.toml index 146a1f59..9299a859 100644 --- a/abi_stable/Cargo.toml +++ b/abi_stable/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "abi_stable" -version = "0.9.0" +version = "0.9.1" authors = ["rodrimati1992 "] edition="2018" license = "MIT/Apache-2.0" diff --git a/abi_stable/src/std_types/string.rs b/abi_stable/src/std_types/string.rs index 708502d8..25fc3525 100644 --- a/abi_stable/src/std_types/string.rs +++ b/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); } } diff --git a/abi_stable/src/std_types/string/tests.rs b/abi_stable/src/std_types/string/tests.rs index 1b5ea119..8d66bfec 100644 --- a/abi_stable/src/std_types/string/tests.rs +++ b/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()); + } } diff --git a/abi_stable/src/std_types/vec.rs b/abi_stable/src/std_types/vec.rs index 7e8afbab..8f9dc9b5 100644 --- a/abi_stable/src/std_types/vec.rs +++ b/abi_stable/src/std_types/vec.rs @@ -812,6 +812,7 @@ impl RVec { del: 0, old_len, pred: |x| !pred(x), + panic_flag: false, }; } diff --git a/abi_stable/src/std_types/vec/iters.rs b/abi_stable/src/std_types/vec/iters.rs index d9f4fcaa..d6791120 100644 --- a/abi_stable/src/std_types/vec/iters.rs +++ b/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 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 { 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 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 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); } } } - diff --git a/abi_stable/src/std_types/vec/tests.rs b/abi_stable/src/std_types/vec/tests.rs index 65f4a496..4c11f6e8 100644 --- a/abi_stable/src/std_types/vec/tests.rs +++ b/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!(©[..], <&[i32]>::default()); + assert_eq!(©[..], &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]); -} \ No newline at end of file +} + +// 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>>, + } + + 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 = (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); + } +} + +