From 84e9608596f14eba78b8ff0371f747f7513b523a Mon Sep 17 00:00:00 2001 From: Waffle Date: Thu, 4 Mar 2021 11:30:48 +0300 Subject: [PATCH 1/2] Fix leak in Vec::extend_from_within Previously vec's len was updated only after full copy, making the method leak if T::clone panic!s. This commit makes `Vec::extend_from_within` (or, more accurately, it's `T: Clone` specialization) update vec's len on every iteration, fixing the issue. `T: Copy` specialization was not affected by the issue b/c it doesn't call user specified code (as, e.g. `T::clone`), and instead calls `ptr::copy_nonoverlapping`. --- library/alloc/src/vec/mod.rs | 43 ++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/library/alloc/src/vec/mod.rs b/library/alloc/src/vec/mod.rs index 0028e290fac4e..369fa460863d9 100644 --- a/library/alloc/src/vec/mod.rs +++ b/library/alloc/src/vec/mod.rs @@ -1942,6 +1942,16 @@ impl Vec { #[unstable(feature = "vec_split_at_spare", issue = "81944")] #[inline] pub fn split_at_spare_mut(&mut self) -> (&mut [T], &mut [MaybeUninit]) { + // SAFETY: + // - len is ignored and so never changed + let (init, spare, _) = unsafe{ self.split_at_spare_mut_with_len() }; + (init, spare) + } + + /// Safety: changing returned .2 (&mut usize) is considered the same as calling `.set_len(_)`. + /// + /// This method is used to have unique access to all vec parts at once in `extend_from_within`. + unsafe fn split_at_spare_mut_with_len(&mut self) -> (&mut [T], &mut [MaybeUninit], &mut usize) { let Range { start: ptr, end: spare_ptr } = self.as_mut_ptr_range(); let spare_ptr = spare_ptr.cast::>(); let spare_len = self.buf.capacity() - self.len; @@ -1953,9 +1963,9 @@ impl Vec { let initialized = slice::from_raw_parts_mut(ptr, self.len); let spare = slice::from_raw_parts_mut(spare_ptr, spare_len); - (initialized, spare) + (initialized, spare, &mut self.len) } - } + } } impl Vec { @@ -2165,22 +2175,23 @@ trait ExtendFromWithinSpec { impl ExtendFromWithinSpec for Vec { default unsafe fn spec_extend_from_within(&mut self, src: Range) { - let initialized = { - let (this, spare) = self.split_at_spare_mut(); - - // SAFETY: - // - caller guaratees that src is a valid index - let to_clone = unsafe { this.get_unchecked(src) }; - - to_clone.iter().cloned().zip(spare.iter_mut()).map(|(e, s)| s.write(e)).count() - }; + // SAFETY: + // - len is increased only after initializing elements + let (this, spare, len) = unsafe { self.split_at_spare_mut_with_len() }; // SAFETY: - // - elements were just initialized - unsafe { - let new_len = self.len() + initialized; - self.set_len(new_len); - } + // - caller guaratees that src is a valid index + let to_clone = unsafe { this.get_unchecked(src) }; + + to_clone + .iter() + .cloned() + .zip(spare.iter_mut()) + .map(|(src, dst)| dst.write(src)) + // Note: + // - Element was just initialized with `MaybeUninit::write`, so it's ok to increace len + // - len is increased after each element to prevent leaks (see issue #82533) + .for_each(|_| *len += 1); } } From 1f031d95ded44337848f53e07aae05087ccf15f1 Mon Sep 17 00:00:00 2001 From: Waffle Date: Thu, 4 Mar 2021 12:19:52 +0300 Subject: [PATCH 2/2] Add regression test for `Vec::extend_from_within` leak --- library/alloc/src/vec/mod.rs | 8 ++++--- library/alloc/tests/vec.rs | 42 ++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/library/alloc/src/vec/mod.rs b/library/alloc/src/vec/mod.rs index 369fa460863d9..49758f672a89f 100644 --- a/library/alloc/src/vec/mod.rs +++ b/library/alloc/src/vec/mod.rs @@ -1944,14 +1944,16 @@ impl Vec { pub fn split_at_spare_mut(&mut self) -> (&mut [T], &mut [MaybeUninit]) { // SAFETY: // - len is ignored and so never changed - let (init, spare, _) = unsafe{ self.split_at_spare_mut_with_len() }; + let (init, spare, _) = unsafe { self.split_at_spare_mut_with_len() }; (init, spare) } /// Safety: changing returned .2 (&mut usize) is considered the same as calling `.set_len(_)`. /// /// This method is used to have unique access to all vec parts at once in `extend_from_within`. - unsafe fn split_at_spare_mut_with_len(&mut self) -> (&mut [T], &mut [MaybeUninit], &mut usize) { + unsafe fn split_at_spare_mut_with_len( + &mut self, + ) -> (&mut [T], &mut [MaybeUninit], &mut usize) { let Range { start: ptr, end: spare_ptr } = self.as_mut_ptr_range(); let spare_ptr = spare_ptr.cast::>(); let spare_len = self.buf.capacity() - self.len; @@ -1965,7 +1967,7 @@ impl Vec { (initialized, spare, &mut self.len) } - } + } } impl Vec { diff --git a/library/alloc/tests/vec.rs b/library/alloc/tests/vec.rs index fab450285854d..1ba2315ca73eb 100644 --- a/library/alloc/tests/vec.rs +++ b/library/alloc/tests/vec.rs @@ -7,6 +7,7 @@ use std::mem::{size_of, swap}; use std::ops::Bound::*; use std::panic::{catch_unwind, AssertUnwindSafe}; use std::rc::Rc; +use std::sync::atomic::{AtomicU32, Ordering}; use std::vec::{Drain, IntoIter}; struct DropCounter<'a> { @@ -2100,3 +2101,44 @@ fn test_extend_from_within() { assert_eq!(v, ["a", "b", "c", "b", "c", "a", "b"]); } + +// Regression test for issue #82533 +#[test] +fn test_extend_from_within_panicing_clone() { + struct Panic<'dc> { + drop_count: &'dc AtomicU32, + aaaaa: bool, + } + + impl Clone for Panic<'_> { + fn clone(&self) -> Self { + if self.aaaaa { + panic!("panic! at the clone"); + } + + Self { ..*self } + } + } + + impl Drop for Panic<'_> { + fn drop(&mut self) { + self.drop_count.fetch_add(1, Ordering::SeqCst); + } + } + + let count = core::sync::atomic::AtomicU32::new(0); + let mut vec = vec![ + Panic { drop_count: &count, aaaaa: false }, + Panic { drop_count: &count, aaaaa: true }, + Panic { drop_count: &count, aaaaa: false }, + ]; + + // This should clone&append one Panic{..} at the end, and then panic while + // cloning second Panic{..}. This means that `Panic::drop` should be called + // 4 times (3 for items already in vector, 1 for just appended). + // + // Previously just appended item was leaked, making drop_count = 3, instead of 4. + std::panic::catch_unwind(move || vec.extend_from_within(..)).unwrap_err(); + + assert_eq!(count.load(Ordering::SeqCst), 4); +}