From fa61678a7d471cfc8bf166979674d4574a5d3bec Mon Sep 17 00:00:00 2001 From: Giacomo Stevanato Date: Sat, 10 Sep 2022 11:26:24 +0200 Subject: Fix leaking in inplace collection when destructor panics --- library/alloc/src/vec/in_place_collect.rs | 8 ++++++-- library/alloc/src/vec/in_place_drop.rs | 15 +++++++++++++++ library/alloc/src/vec/mod.rs | 2 +- 3 files changed, 22 insertions(+), 3 deletions(-) (limited to 'library/alloc/src') diff --git a/library/alloc/src/vec/in_place_collect.rs b/library/alloc/src/vec/in_place_collect.rs index 55dcb84ad16..359f1a9cec2 100644 --- a/library/alloc/src/vec/in_place_collect.rs +++ b/library/alloc/src/vec/in_place_collect.rs @@ -138,7 +138,7 @@ use core::iter::{InPlaceIterable, SourceIter, TrustedRandomAccessNoCoerce}; use core::mem::{self, ManuallyDrop}; use core::ptr::{self}; -use super::{InPlaceDrop, SpecFromIter, SpecFromIterNested, Vec}; +use super::{InPlaceDrop, InPlaceDstBufDrop, SpecFromIter, SpecFromIterNested, Vec}; /// Specialization marker for collecting an iterator pipeline into a Vec while reusing the /// source allocation, i.e. executing the pipeline in place. @@ -193,12 +193,16 @@ where // Drop any remaining values at the tail of the source but prevent drop of the allocation // itself once IntoIter goes out of scope. - // If the drop panics then we also leak any elements collected into dst_buf. + // If the drop panics then we also try to drop the destination buffer and its elements. + // This is safe because `forget_allocation_drop_remaining` forgets the allocation *before* + // trying to drop the remaining elements. // // Note: This access to the source wouldn't be allowed by the TrustedRandomIteratorNoCoerce // contract (used by SpecInPlaceCollect below). But see the "O(1) collect" section in the // module documenttation why this is ok anyway. + let dst_guard = InPlaceDstBufDrop { ptr: dst_buf, len, cap }; src.forget_allocation_drop_remaining(); + mem::forget(dst_guard); let vec = unsafe { Vec::from_raw_parts(dst_buf, len, cap) }; diff --git a/library/alloc/src/vec/in_place_drop.rs b/library/alloc/src/vec/in_place_drop.rs index 1b1ef9130fa..25ca33c6a7b 100644 --- a/library/alloc/src/vec/in_place_drop.rs +++ b/library/alloc/src/vec/in_place_drop.rs @@ -22,3 +22,18 @@ impl Drop for InPlaceDrop { } } } + +// A helper struct for in-place collection that drops the destination allocation and elements, +// to avoid leaking them if some other destructor panics. +pub(super) struct InPlaceDstBufDrop { + pub(super) ptr: *mut T, + pub(super) len: usize, + pub(super) cap: usize, +} + +impl Drop for InPlaceDstBufDrop { + #[inline] + fn drop(&mut self) { + unsafe { super::Vec::from_raw_parts(self.ptr, self.len, self.cap) }; + } +} diff --git a/library/alloc/src/vec/mod.rs b/library/alloc/src/vec/mod.rs index fa9f2131c0c..acfbb98272d 100644 --- a/library/alloc/src/vec/mod.rs +++ b/library/alloc/src/vec/mod.rs @@ -125,7 +125,7 @@ use self::set_len_on_drop::SetLenOnDrop; mod set_len_on_drop; #[cfg(not(no_global_oom_handling))] -use self::in_place_drop::InPlaceDrop; +use self::in_place_drop::{InPlaceDrop, InPlaceDstBufDrop}; #[cfg(not(no_global_oom_handling))] mod in_place_drop; -- cgit 1.4.1-3-g733a5 From f52082f54321e56fa4dbd9194c1cfd61089e2729 Mon Sep 17 00:00:00 2001 From: Giacomo Stevanato Date: Sat, 10 Sep 2022 12:29:16 +0200 Subject: Update documentation --- library/alloc/src/vec/in_place_collect.rs | 7 +++++-- library/alloc/src/vec/into_iter.rs | 7 ++++++- 2 files changed, 11 insertions(+), 3 deletions(-) (limited to 'library/alloc/src') diff --git a/library/alloc/src/vec/in_place_collect.rs b/library/alloc/src/vec/in_place_collect.rs index 359f1a9cec2..83ed3d915a2 100644 --- a/library/alloc/src/vec/in_place_collect.rs +++ b/library/alloc/src/vec/in_place_collect.rs @@ -55,6 +55,9 @@ //! This is handled by the [`InPlaceDrop`] guard for sink items (`U`) and by //! [`vec::IntoIter::forget_allocation_drop_remaining()`] for remaining source items (`T`). //! +//! If dropping any remaining source item (`T`) panics then [`InPlaceDstBufDrop`] will handle dropping +//! the already collected sink items (`U`) and freeing the allocation. +//! //! [`vec::IntoIter::forget_allocation_drop_remaining()`]: super::IntoIter::forget_allocation_drop_remaining() //! //! # O(1) collect @@ -194,8 +197,8 @@ where // Drop any remaining values at the tail of the source but prevent drop of the allocation // itself once IntoIter goes out of scope. // If the drop panics then we also try to drop the destination buffer and its elements. - // This is safe because `forget_allocation_drop_remaining` forgets the allocation *before* - // trying to drop the remaining elements. + // This is safe because `forget_allocation_drop_remaining` immediately forgets the allocation + // and won't panic before that. // // Note: This access to the source wouldn't be allowed by the TrustedRandomIteratorNoCoerce // contract (used by SpecInPlaceCollect below). But see the "O(1) collect" section in the diff --git a/library/alloc/src/vec/into_iter.rs b/library/alloc/src/vec/into_iter.rs index 1b483e3fc77..b5a03392a17 100644 --- a/library/alloc/src/vec/into_iter.rs +++ b/library/alloc/src/vec/into_iter.rs @@ -96,13 +96,16 @@ impl IntoIter { } /// Drops remaining elements and relinquishes the backing allocation. + /// This method guarantees it won't panic before relinquishing + /// the backing allocation. /// /// This is roughly equivalent to the following, but more efficient /// /// ``` /// # let mut into_iter = Vec::::with_capacity(10).into_iter(); + /// let mut into_iter = std::mem::replace(&mut into_iter, Vec::new().into_iter()); /// (&mut into_iter).for_each(core::mem::drop); - /// unsafe { core::ptr::write(&mut into_iter, Vec::new().into_iter()); } + /// std::mem::forget(into_iter); /// ``` /// /// This method is used by in-place iteration, refer to the vec::in_place_collect @@ -119,6 +122,8 @@ impl IntoIter { self.ptr = self.buf.as_ptr(); self.end = self.buf.as_ptr(); + // Dropping the remaining elements can panic, so this needs to be + // done only after updating the other fields. unsafe { ptr::drop_in_place(remaining); } -- cgit 1.4.1-3-g733a5 From 1750c7bdd36ec18324423bd30867e39d787d5977 Mon Sep 17 00:00:00 2001 From: Giacomo Stevanato Date: Sat, 10 Sep 2022 15:00:37 +0200 Subject: Clarify documentation --- library/alloc/src/vec/in_place_collect.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'library/alloc/src') diff --git a/library/alloc/src/vec/in_place_collect.rs b/library/alloc/src/vec/in_place_collect.rs index 83ed3d915a2..9f2555ee161 100644 --- a/library/alloc/src/vec/in_place_collect.rs +++ b/library/alloc/src/vec/in_place_collect.rs @@ -194,11 +194,10 @@ where ); } - // Drop any remaining values at the tail of the source but prevent drop of the allocation - // itself once IntoIter goes out of scope. - // If the drop panics then we also try to drop the destination buffer and its elements. + // The ownership of the allocation and the new `T` values is temporarily moved into `dst_guard`. // This is safe because `forget_allocation_drop_remaining` immediately forgets the allocation - // and won't panic before that. + // before any panic can occur in order to avoid any double free, and then proceeds to drop + // any remaining values at the tail of the source. // // Note: This access to the source wouldn't be allowed by the TrustedRandomIteratorNoCoerce // contract (used by SpecInPlaceCollect below). But see the "O(1) collect" section in the -- cgit 1.4.1-3-g733a5