From 0cf606177e79bc580fa27a82eb9c8b56e7253f46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Horstmann?= Date: Fri, 25 Mar 2022 11:39:11 +0100 Subject: [PATCH 1/4] Fix double drop of allocator in IntoIter impl of Vec --- library/alloc/src/vec/into_iter.rs | 15 +++++++++------ library/alloc/src/vec/mod.rs | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/library/alloc/src/vec/into_iter.rs b/library/alloc/src/vec/into_iter.rs index f17b8d71b3a..471042cd717 100644 --- a/library/alloc/src/vec/into_iter.rs +++ b/library/alloc/src/vec/into_iter.rs @@ -8,7 +8,8 @@ FusedIterator, InPlaceIterable, SourceIter, TrustedLen, TrustedRandomAccessNoCoerce, }; use core::marker::PhantomData; -use core::mem::{self}; +use core::mem::{self, ManuallyDrop}; +use core::ops::Deref; use core::ptr::{self, NonNull}; use core::slice::{self}; @@ -32,7 +33,9 @@ pub struct IntoIter< pub(super) buf: NonNull, pub(super) phantom: PhantomData, pub(super) cap: usize, - pub(super) alloc: A, + // the drop impl reconstructs a RawVec from buf, cap and alloc + // to avoid dropping the allocator twice we need to wrap it into ManuallyDrop + pub(super) alloc: ManuallyDrop, pub(super) ptr: *const T, pub(super) end: *const T, } @@ -295,11 +298,11 @@ unsafe impl TrustedRandomAccessNoCoerce for IntoIter impl Clone for IntoIter { #[cfg(not(test))] fn clone(&self) -> Self { - self.as_slice().to_vec_in(self.alloc.clone()).into_iter() + self.as_slice().to_vec_in(self.alloc.deref().clone()).into_iter() } #[cfg(test)] fn clone(&self) -> Self { - crate::slice::to_vec(self.as_slice(), self.alloc.clone()).into_iter() + crate::slice::to_vec(self.as_slice(), self.alloc.deref().clone()).into_iter() } } @@ -311,8 +314,8 @@ fn drop(&mut self) { impl Drop for DropGuard<'_, T, A> { fn drop(&mut self) { unsafe { - // `IntoIter::alloc` is not used anymore after this - let alloc = ptr::read(&self.0.alloc); + // `IntoIter::alloc` is not used anymore after this and will be dropped by RawVec + let alloc = ManuallyDrop::into_inner(ptr::read(&self.0.alloc)); // RawVec handles deallocation let _ = RawVec::from_raw_parts_in(self.0.buf.as_ptr(), self.0.cap, alloc); } diff --git a/library/alloc/src/vec/mod.rs b/library/alloc/src/vec/mod.rs index 9a66e69bdc0..96857c4bd6f 100644 --- a/library/alloc/src/vec/mod.rs +++ b/library/alloc/src/vec/mod.rs @@ -2575,7 +2575,7 @@ impl IntoIterator for Vec { fn into_iter(self) -> IntoIter { unsafe { let mut me = ManuallyDrop::new(self); - let alloc = ptr::read(me.allocator()); + let alloc = ManuallyDrop::new(ptr::read(me.allocator())); let begin = me.as_mut_ptr(); let end = if mem::size_of::() == 0 { arith_offset(begin as *const i8, me.len() as isize) as *const T From d14c0d2acb3ff1d0111920185109c9697a3cd460 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Horstmann?= Date: Fri, 25 Mar 2022 13:27:18 +0100 Subject: [PATCH 2/4] Use ManuallyDrop::take instead of into_inner Co-authored-by: Daniel Henry-Mantilla --- library/alloc/src/vec/into_iter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/alloc/src/vec/into_iter.rs b/library/alloc/src/vec/into_iter.rs index 471042cd717..1f9398faf22 100644 --- a/library/alloc/src/vec/into_iter.rs +++ b/library/alloc/src/vec/into_iter.rs @@ -315,7 +315,7 @@ impl Drop for DropGuard<'_, T, A> { fn drop(&mut self) { unsafe { // `IntoIter::alloc` is not used anymore after this and will be dropped by RawVec - let alloc = ManuallyDrop::into_inner(ptr::read(&self.0.alloc)); + let alloc = ManuallyDrop::take(&mut self.0.alloc); // RawVec handles deallocation let _ = RawVec::from_raw_parts_in(self.0.buf.as_ptr(), self.0.cap, alloc); } From 4b53f563bd5b0f0e9cad26dbf2514f9a72c3fa2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Horstmann?= Date: Fri, 25 Mar 2022 13:00:49 +0100 Subject: [PATCH 3/4] Add a test verifying the number of drop calls --- library/alloc/tests/vec.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/library/alloc/tests/vec.rs b/library/alloc/tests/vec.rs index ca0fcc855c7..ab2414a6dc0 100644 --- a/library/alloc/tests/vec.rs +++ b/library/alloc/tests/vec.rs @@ -1,3 +1,6 @@ +use core::alloc::{Allocator, Layout}; +use core::ptr::NonNull; +use std::alloc::System; use std::assert_matches::assert_matches; use std::borrow::Cow; use std::cell::Cell; @@ -991,6 +994,27 @@ fn test_into_iter_advance_by() { assert_eq!(i.len(), 0); } +#[test] +fn test_into_iter_drop_allocator() { + struct ReferenceCountedAllocator<'a>(DropCounter<'a>); + + unsafe impl Allocator for ReferenceCountedAllocator<'_> { + fn allocate(&self, layout: Layout) -> Result, core::alloc::AllocError> { + System.allocate(layout) + } + + unsafe fn deallocate(&self, ptr: NonNull, layout: Layout) { + System.deallocate(ptr, layout) + } + } + + let mut drop_count = 0; + let allocator = ReferenceCountedAllocator(DropCounter { count: &mut drop_count }); + let _ = Vec::::new_in(allocator).into_iter(); + + assert_eq!(drop_count, 1); +} + #[test] fn test_from_iter_specialization() { let src: Vec = vec![0usize; 1]; From d9a438dc73de6ff146ae3e6bc4050b7cea41b09e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Horstmann?= Date: Fri, 25 Mar 2022 16:57:59 +0100 Subject: [PATCH 4/4] Add another assertion without into_iter --- library/alloc/tests/vec.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/library/alloc/tests/vec.rs b/library/alloc/tests/vec.rs index ab2414a6dc0..19e39ebf910 100644 --- a/library/alloc/tests/vec.rs +++ b/library/alloc/tests/vec.rs @@ -1009,10 +1009,14 @@ unsafe fn deallocate(&self, ptr: NonNull, layout: Layout) { } let mut drop_count = 0; + + let allocator = ReferenceCountedAllocator(DropCounter { count: &mut drop_count }); + let _ = Vec::::new_in(allocator); + assert_eq!(drop_count, 1); + let allocator = ReferenceCountedAllocator(DropCounter { count: &mut drop_count }); let _ = Vec::::new_in(allocator).into_iter(); - - assert_eq!(drop_count, 1); + assert_eq!(drop_count, 2); } #[test]