From e990441afe3eeddd6af928cfc97d429f6732c8a1 Mon Sep 17 00:00:00 2001 From: Pavel Grigorenko Date: Mon, 24 Aug 2026 21:08:28 +0300 Subject: [PATCH] Use `drop_guard` in some places in {core,alloc,std} --- library/alloc/src/boxed/thin.rs | 41 +-- .../alloc/src/collections/binary_heap/mod.rs | 14 +- library/alloc/src/collections/btree/map.rs | 18 +- library/alloc/src/collections/btree/node.rs | 16 +- library/alloc/src/collections/linked_list.rs | 22 +- .../alloc/src/collections/vec_deque/drain.rs | 241 +++++++++--------- .../src/collections/vec_deque/into_iter.rs | 54 ++-- .../alloc/src/collections/vec_deque/mod.rs | 30 +-- library/alloc/src/rc.rs | 40 +-- library/alloc/src/slice.rs | 35 ++- library/alloc/src/sync.rs | 55 ++-- library/alloc/src/vec/drain.rs | 41 ++- library/alloc/src/vec/into_iter.rs | 18 +- library/alloctests/lib.rs | 1 + library/std/src/sys/fs/unix.rs | 36 +-- library/std/src/sys/pal/unix/sync/condvar.rs | 21 +- library/std/src/sys/process/unix/unix.rs | 68 ++--- library/std/src/sys/process/windows/tests.rs | 10 +- 18 files changed, 292 insertions(+), 469 deletions(-) diff --git a/library/alloc/src/boxed/thin.rs b/library/alloc/src/boxed/thin.rs index 22c3d89e3ccdb..02b3af1411733 100644 --- a/library/alloc/src/boxed/thin.rs +++ b/library/alloc/src/boxed/thin.rs @@ -11,7 +11,7 @@ use core::marker::PhantomData; use core::marker::Unsize; #[cfg(not(no_global_oom_handling))] use core::mem; -use core::mem::SizedTypeProperties; +use core::mem::{DropGuard, SizedTypeProperties}; use core::ops::{Deref, DerefMut}; use core::ptr::{self, NonNull, Pointee}; @@ -360,38 +360,23 @@ impl WithHeader { // - Assumes that either `value` can be dereferenced, or is the // `NonNull::dangling()` we use when both `T` and `H` are ZSTs. unsafe fn drop(&self, value: *mut T) { - struct DropGuard { - ptr: NonNull, - value_layout: Layout, - _marker: PhantomData, - } - - impl Drop for DropGuard { - fn drop(&mut self) { - // All ZST are allocated statically. - if self.value_layout.size() == 0 { - return; - } + unsafe { + // `_guard` will deallocate the memory when dropped, even if `drop_in_place` unwinds. + let _guard = + DropGuard::new((self.0, Layout::for_value_raw(value)), |(ptr, value_layout)| { + // All ZST are allocated statically. + if value_layout.size() == 0 { + return; + } - unsafe { // SAFETY: Layout must have been computable if we're in drop let (layout, value_offset) = - WithHeader::::alloc_layout(self.value_layout).unwrap_unchecked(); + WithHeader::::alloc_layout(value_layout).unwrap_unchecked(); // Since we only allocate for non-ZSTs, the layout size cannot be zero. - debug_assert!(layout.size() != 0); - alloc::dealloc(self.ptr.as_ptr().sub(value_offset), layout); - } - } - } - - unsafe { - // `_guard` will deallocate the memory when dropped, even if `drop_in_place` unwinds. - let _guard = DropGuard { - ptr: self.0, - value_layout: Layout::for_value_raw(value), - _marker: PhantomData::, - }; + debug_assert_ne!(layout.size(), 0); + alloc::dealloc(ptr.as_ptr().sub(value_offset), layout); + }); // We only drop the value because the Pointee trait requires that the metadata is copy // aka trivially droppable. diff --git a/library/alloc/src/collections/binary_heap/mod.rs b/library/alloc/src/collections/binary_heap/mod.rs index 98192e1fb5b01..e84f9e44771ff 100644 --- a/library/alloc/src/collections/binary_heap/mod.rs +++ b/library/alloc/src/collections/binary_heap/mod.rs @@ -145,7 +145,7 @@ use core::alloc::Allocator; use core::iter::{FusedIterator, InPlaceIterable, SourceIter, TrustedFused, TrustedLen}; -use core::mem::{self, ManuallyDrop, swap}; +use core::mem::{DropGuard, ManuallyDrop, swap}; use core::num::NonZero; use core::ops::{Deref, DerefMut}; use core::{fmt, ptr}; @@ -1907,18 +1907,10 @@ impl<'a, T: Ord, A: Allocator> DrainSorted<'a, T, A> { impl<'a, T: Ord, A: Allocator> Drop for DrainSorted<'a, T, A> { /// Removes heap elements in heap order. fn drop(&mut self) { - struct DropGuard<'r, 'a, T: Ord, A: Allocator>(&'r mut DrainSorted<'a, T, A>); - - impl<'r, 'a, T: Ord, A: Allocator> Drop for DropGuard<'r, 'a, T, A> { - fn drop(&mut self) { - while self.0.inner.pop().is_some() {} - } - } - while let Some(item) = self.inner.pop() { - let guard = DropGuard(self); + let guard = DropGuard::new(&mut *self, |this| while this.inner.pop().is_some() {}); drop(item); - mem::forget(guard); + DropGuard::dismiss(guard); } } } diff --git a/library/alloc/src/collections/btree/map.rs b/library/alloc/src/collections/btree/map.rs index d8421d3c3f70a..7be88865b8b3a 100644 --- a/library/alloc/src/collections/btree/map.rs +++ b/library/alloc/src/collections/btree/map.rs @@ -5,7 +5,7 @@ use core::fmt::{self, Debug}; use core::hash::{Hash, Hasher}; use core::iter::{FusedIterator, TrustedLen}; use core::marker::PhantomData; -use core::mem::{self, ManuallyDrop}; +use core::mem::{self, DropGuard, ManuallyDrop}; use core::ops::{Bound, Index, RangeBounds}; use core::ptr; @@ -1904,24 +1904,18 @@ impl IntoIterator for BTreeMap { #[stable(feature = "btree_drop", since = "1.7.0")] impl Drop for IntoIter { fn drop(&mut self) { - struct DropGuard<'a, K, V, A: Allocator + Clone>(&'a mut IntoIter); - - impl<'a, K, V, A: Allocator + Clone> Drop for DropGuard<'a, K, V, A> { - fn drop(&mut self) { + while let Some(kv) = self.dying_next() { + let guard = DropGuard::new(&mut *self, |this| { // Continue the same loop we perform below. This only runs when unwinding, so we // don't have to care about panics this time (they'll abort). - while let Some(kv) = self.0.dying_next() { + while let Some(kv) = this.dying_next() { // SAFETY: we consume the dying handle immediately. unsafe { kv.drop_key_val() }; } - } - } - - while let Some(kv) = self.dying_next() { - let guard = DropGuard(self); + }); // SAFETY: we don't touch the tree before consuming the dying handle. unsafe { kv.drop_key_val() }; - mem::forget(guard); + DropGuard::dismiss(guard); } } } diff --git a/library/alloc/src/collections/btree/node.rs b/library/alloc/src/collections/btree/node.rs index 0c7afcc63b9b7..e60ef9734136f 100644 --- a/library/alloc/src/collections/btree/node.rs +++ b/library/alloc/src/collections/btree/node.rs @@ -32,7 +32,7 @@ // an edge both identifies a position and contains a pointer to a child node. use core::marker::PhantomData; -use core::mem::{self, MaybeUninit}; +use core::mem::{self, DropGuard, MaybeUninit}; use core::num::NonZero; use core::ptr::{self, NonNull}; use core::slice::SliceIndex; @@ -1192,23 +1192,13 @@ impl Handle, marker::KV> /// The node that the handle refers to must not yet have been deallocated. #[inline] pub(super) unsafe fn drop_key_val(mut self) { - // Run the destructor of the value even if the destructor of the key panics. - struct Dropper<'a, T>(&'a mut MaybeUninit); - impl Drop for Dropper<'_, T> { - #[inline] - fn drop(&mut self) { - unsafe { - self.0.assume_init_drop(); - } - } - } - debug_assert!(self.idx < self.node.len()); let leaf = self.node.as_leaf_dying(); unsafe { let key = leaf.keys.get_unchecked_mut(self.idx); let val = leaf.vals.get_unchecked_mut(self.idx); - let _guard = Dropper(val); + // Run the destructor of the value even if the destructor of the key panics. + let _guard = DropGuard::new(val, |val| val.assume_init_drop()); key.assume_init_drop(); // dropping the guard will drop the value } diff --git a/library/alloc/src/collections/linked_list.rs b/library/alloc/src/collections/linked_list.rs index 8939b2f12f49c..fc7b8d02541bd 100644 --- a/library/alloc/src/collections/linked_list.rs +++ b/library/alloc/src/collections/linked_list.rs @@ -17,6 +17,7 @@ use core::cmp::Ordering; use core::hash::{Hash, Hasher}; use core::iter::{FusedIterator, TrustedLen}; use core::marker::PhantomData; +use core::mem::DropGuard; use core::ptr::NonNull; use core::{fmt, mem}; @@ -1177,20 +1178,15 @@ impl LinkedList { #[stable(feature = "rust1", since = "1.0.0")] unsafe impl<#[may_dangle] T, A: Allocator> Drop for LinkedList { fn drop(&mut self) { - struct DropGuard<'a, T, A: Allocator>(&'a mut LinkedList); - - impl<'a, T, A: Allocator> Drop for DropGuard<'a, T, A> { - fn drop(&mut self) { - // Continue the same loop we do below. This only runs when a destructor has - // panicked. If another one panics this will abort. - while self.0.pop_front_node().is_some() {} - } - } - // Wrap self so that if a destructor panics, we can try to keep looping - let guard = DropGuard(self); - while guard.0.pop_front_node().is_some() {} - mem::forget(guard); + let mut guard = DropGuard::new(self, |this| { + // Continue the same loop we do below. This only runs when a destructor has + // panicked. If another one panics this will abort. + while this.pop_front_node().is_some() {} + }); + + while guard.pop_front_node().is_some() {} + DropGuard::dismiss(guard); } } diff --git a/library/alloc/src/collections/vec_deque/drain.rs b/library/alloc/src/collections/vec_deque/drain.rs index da4b803c64d56..ff8f5c97886bb 100644 --- a/library/alloc/src/collections/vec_deque/drain.rs +++ b/library/alloc/src/collections/vec_deque/drain.rs @@ -1,6 +1,6 @@ use core::iter::FusedIterator; use core::marker::PhantomData; -use core::mem::{self, SizedTypeProperties}; +use core::mem::{self, DropGuard, SizedTypeProperties}; use core::ptr::NonNull; use core::{fmt, ptr}; @@ -93,140 +93,133 @@ unsafe impl Send for Drain<'_, T, A> {} #[stable(feature = "drain", since = "1.6.0")] impl Drop for Drain<'_, T, A> { fn drop(&mut self) { - struct DropGuard<'r, 'a, T, A: Allocator>(&'r mut Drain<'a, T, A>); - - let guard = DropGuard(self); - - if mem::needs_drop::() && guard.0.remaining != 0 { - unsafe { - // SAFETY: We just checked that `self.remaining != 0`. - let (front, back) = guard.0.as_slices(); - // since idx is a logical index, we don't need to worry about wrapping. - guard.0.idx += front.len(); - guard.0.remaining -= front.len(); - ptr::drop_in_place(front); - guard.0.remaining = 0; - ptr::drop_in_place(back); - } - } - // Dropping `guard` handles moving the remaining elements into place. - impl<'r, 'a, T, A: Allocator> Drop for DropGuard<'r, 'a, T, A> { - #[inline] - fn drop(&mut self) { - if mem::needs_drop::() && self.0.remaining != 0 { - unsafe { - // SAFETY: We just checked that `self.remaining != 0`. - let (front, back) = self.0.as_slices(); - ptr::drop_in_place(front); - ptr::drop_in_place(back); - } + let mut guard = DropGuard::new(self, |this| { + if mem::needs_drop::() && this.remaining != 0 { + unsafe { + // SAFETY: We just checked that `self.remaining != 0`. + let (front, back) = this.as_slices(); + ptr::drop_in_place(front); + ptr::drop_in_place(back); } + } - let source_deque = unsafe { self.0.deque.as_mut() }; + let source_deque = unsafe { this.deque.as_mut() }; - let drain_len = self.0.drain_len; - let head_len = source_deque.len; // #elements in front of the drain - let tail_len = self.0.tail_len; // #elements behind the drain - let new_len = head_len + tail_len; + let drain_len = this.drain_len; + let head_len = source_deque.len; // #elements in front of the drain + let tail_len = this.tail_len; // #elements behind the drain + let new_len = head_len + tail_len; - if T::IS_ZST { - // no need to copy around any memory if T is a ZST - source_deque.len = new_len; - return; - } + if T::IS_ZST { + // no need to copy around any memory if T is a ZST + source_deque.len = new_len; + return; + } - // Next, we will fill the hole left by the drain with as few writes as possible. - // The code below handles the following control flow and reduces the amount of - // branches under the assumption that `head_len == 0 || tail_len == 0`, i.e. - // draining at the front or at the back of the dequeue is especially common. - // - // H = "head index" = `deque.head` - // h = elements in front of the drain - // d = elements in the drain - // t = elements behind the drain - // - // Note that the buffer may wrap at any point and the wrapping is handled by - // `wrap_copy` and `to_physical_idx`. - // - // Case 1: if `head_len == 0 && tail_len == 0` - // Everything was drained, reset the head index back to 0. - // H - // [ . . . . . d d d d . . . . . ] - // H - // [ . . . . . . . . . . . . . . ] - // - // Case 2: else if `tail_len == 0` - // Don't move data or the head index. - // H - // [ . . . h h h h d d d d . . . ] - // H - // [ . . . h h h h . . . . . . . ] - // - // Case 3: else if `head_len == 0` - // Don't move data, but move the head index. - // H - // [ . . . d d d d t t t t . . . ] - // H - // [ . . . . . . . t t t t . . . ] - // - // Case 4: else if `tail_len <= head_len` - // Move data, but not the head index. - // H - // [ . . h h h h d d d d t t . . ] - // H - // [ . . h h h h t t . . . . . . ] - // - // Case 5: else - // Move data and the head index. - // H - // [ . . h h d d d d t t t t . . ] - // H - // [ . . . . . . h h t t t t . . ] + // Next, we will fill the hole left by the drain with as few writes as possible. + // The code below handles the following control flow and reduces the amount of + // branches under the assumption that `head_len == 0 || tail_len == 0`, i.e. + // draining at the front or at the back of the dequeue is especially common. + // + // H = "head index" = `deque.head` + // h = elements in front of the drain + // d = elements in the drain + // t = elements behind the drain + // + // Note that the buffer may wrap at any point and the wrapping is handled by + // `wrap_copy` and `to_physical_idx`. + // + // Case 1: if `head_len == 0 && tail_len == 0` + // Everything was drained, reset the head index back to 0. + // H + // [ . . . . . d d d d . . . . . ] + // H + // [ . . . . . . . . . . . . . . ] + // + // Case 2: else if `tail_len == 0` + // Don't move data or the head index. + // H + // [ . . . h h h h d d d d . . . ] + // H + // [ . . . h h h h . . . . . . . ] + // + // Case 3: else if `head_len == 0` + // Don't move data, but move the head index. + // H + // [ . . . d d d d t t t t . . . ] + // H + // [ . . . . . . . t t t t . . . ] + // + // Case 4: else if `tail_len <= head_len` + // Move data, but not the head index. + // H + // [ . . h h h h d d d d t t . . ] + // H + // [ . . h h h h t t . . . . . . ] + // + // Case 5: else + // Move data and the head index. + // H + // [ . . h h d d d d t t t t . . ] + // H + // [ . . . . . . h h t t t t . . ] - // When draining at the front (`.drain(..n)`) or at the back (`.drain(n..)`), - // we don't need to copy any data. The number of elements copied would be 0. - if head_len != 0 && tail_len != 0 { - join_head_and_tail_wrapping(source_deque, drain_len, head_len, tail_len); - // Marking this function as cold helps LLVM to eliminate it entirely if - // this branch is never taken. - // We use `#[cold]` instead of `#[inline(never)]`, because inlining this - // function into the general case (`.drain(n..m)`) is fine. - // See `tests/codegen-llvm/vecdeque-drain.rs` for a test. - #[cold] - fn join_head_and_tail_wrapping( - source_deque: &mut VecDeque, - drain_len: usize, - head_len: usize, - tail_len: usize, - ) { - // Pick whether to move the head or the tail here. - let (src, dst, len); - if head_len < tail_len { - src = source_deque.head; - dst = source_deque.to_wrapped_index(drain_len); - len = head_len; - } else { - src = source_deque.to_wrapped_index(head_len + drain_len); - dst = source_deque.to_wrapped_index(head_len); - len = tail_len; - }; + // When draining at the front (`.drain(..n)`) or at the back (`.drain(n..)`), + // we don't need to copy any data. The number of elements copied would be 0. + if head_len != 0 && tail_len != 0 { + join_head_and_tail_wrapping(source_deque, drain_len, head_len, tail_len); + // Marking this function as cold helps LLVM to eliminate it entirely if + // this branch is never taken. + // We use `#[cold]` instead of `#[inline(never)]`, because inlining this + // function into the general case (`.drain(n..m)`) is fine. + // See `tests/codegen-llvm/vecdeque-drain.rs` for a test. + #[cold] + fn join_head_and_tail_wrapping( + source_deque: &mut VecDeque, + drain_len: usize, + head_len: usize, + tail_len: usize, + ) { + // Pick whether to move the head or the tail here. + let (src, dst, len); + if head_len < tail_len { + src = source_deque.head; + dst = source_deque.to_wrapped_index(drain_len); + len = head_len; + } else { + src = source_deque.to_wrapped_index(head_len + drain_len); + dst = source_deque.to_wrapped_index(head_len); + len = tail_len; + }; - unsafe { - source_deque.wrap_copy(src, dst, len); - } + unsafe { + source_deque.wrap_copy(src, dst, len); } } + } - if new_len == 0 { - // Special case: If the entire deque was drained, reset the head back to 0, - // like `.clear()` does. - source_deque.head = WrappedIndex::zero(); - } else if head_len < tail_len { - // If we moved the head above, then we need to adjust the head index here. - source_deque.head = source_deque.to_wrapped_index(drain_len); - } - source_deque.len = new_len; + if new_len == 0 { + // Special case: If the entire deque was drained, reset the head back to 0, + // like `.clear()` does. + source_deque.head = WrappedIndex::zero(); + } else if head_len < tail_len { + // If we moved the head above, then we need to adjust the head index here. + source_deque.head = source_deque.to_wrapped_index(drain_len); + } + source_deque.len = new_len; + }); + + if mem::needs_drop::() && guard.remaining != 0 { + unsafe { + // SAFETY: We just checked that `self.remaining != 0`. + let (front, back) = guard.as_slices(); + // since idx is a logical index, we don't need to worry about wrapping. + guard.idx += front.len(); + guard.remaining -= front.len(); + ptr::drop_in_place(front); + guard.remaining = 0; + ptr::drop_in_place(back); } } } diff --git a/library/alloc/src/collections/vec_deque/into_iter.rs b/library/alloc/src/collections/vec_deque/into_iter.rs index e18b85dd4b694..7c83fff6c4ab1 100644 --- a/library/alloc/src/collections/vec_deque/into_iter.rs +++ b/library/alloc/src/collections/vec_deque/into_iter.rs @@ -1,5 +1,5 @@ use core::iter::{FusedIterator, TrustedLen}; -use core::mem::MaybeUninit; +use core::mem::{DropGuard, MaybeUninit}; use core::num::NonZero; use core::ops::Try; use core::{array, fmt, ptr}; @@ -78,28 +78,20 @@ impl Iterator for IntoIter { F: FnMut(B, Self::Item) -> R, R: Try, { - struct Guard<'a, T, A: Allocator> { - deque: &'a mut VecDeque, - // `consumed <= deque.len` always holds. - consumed: usize, - } - - impl<'a, T, A: Allocator> Drop for Guard<'a, T, A> { - fn drop(&mut self) { - self.deque.len -= self.consumed; - self.deque.head = self.deque.to_wrapped_index(self.consumed); - } - } - - let mut guard = Guard { deque: &mut self.inner, consumed: 0 }; + // `consumed <= deque.len` always holds. + let mut guard = DropGuard::new((&mut self.inner, 0), |(deque, consumed)| { + deque.len -= consumed; + deque.head = deque.to_wrapped_index(consumed); + }); - let (head, tail) = guard.deque.as_slices(); + let (deque, consumed) = &mut *guard; + let (head, tail) = deque.as_slices(); init = head .iter() .map(|elem| { - guard.consumed += 1; - // SAFETY: Because we incremented `guard.consumed`, the + *consumed += 1; + // SAFETY: Because we incremented `consumed`, the // deque effectively forgot the element, so we can take // ownership unsafe { ptr::read(elem) } @@ -108,7 +100,7 @@ impl Iterator for IntoIter { tail.iter() .map(|elem| { - guard.consumed += 1; + *consumed += 1; // SAFETY: Same as above. unsafe { ptr::read(elem) } }) @@ -201,26 +193,18 @@ impl DoubleEndedIterator for IntoIter { F: FnMut(B, Self::Item) -> R, R: Try, { - struct Guard<'a, T, A: Allocator> { - deque: &'a mut VecDeque, - // `consumed <= deque.len` always holds. - consumed: usize, - } - - impl<'a, T, A: Allocator> Drop for Guard<'a, T, A> { - fn drop(&mut self) { - self.deque.len -= self.consumed; - } - } - - let mut guard = Guard { deque: &mut self.inner, consumed: 0 }; + // `consumed <= deque.len` always holds. + let mut guard = DropGuard::new((&mut self.inner, 0), |(deque, consumed)| { + deque.len -= consumed; + }); - let (head, tail) = guard.deque.as_slices(); + let (deque, consumed) = &mut *guard; + let (head, tail) = deque.as_slices(); init = tail .iter() .map(|elem| { - guard.consumed += 1; + *consumed += 1; // SAFETY: See `try_fold`'s safety comment. unsafe { ptr::read(elem) } }) @@ -228,7 +212,7 @@ impl DoubleEndedIterator for IntoIter { head.iter() .map(|elem| { - guard.consumed += 1; + *consumed += 1; // SAFETY: Same as above. unsafe { ptr::read(elem) } }) diff --git a/library/alloc/src/collections/vec_deque/mod.rs b/library/alloc/src/collections/vec_deque/mod.rs index 9095fc0d4abf4..b4bd806697061 100644 --- a/library/alloc/src/collections/vec_deque/mod.rs +++ b/library/alloc/src/collections/vec_deque/mod.rs @@ -17,7 +17,7 @@ use core::iter::{ByRefSized, repeat_n, repeat_with}; // failures in linkchecker even though rustdoc built the docs just fine. #[allow(unused_imports)] use core::mem; -use core::mem::{ManuallyDrop, SizedTypeProperties}; +use core::mem::{DropGuard, ManuallyDrop, SizedTypeProperties}; use core::ops::{Index, IndexMut, Range, RangeBounds}; use core::{fmt, ptr, slice}; @@ -632,35 +632,23 @@ impl VecDeque { mut iter: impl Iterator, len: usize, ) -> usize { - struct Guard<'a, T, A: Allocator> { - deque: &'a mut VecDeque, - written: usize, - } - - impl<'a, T, A: Allocator> Drop for Guard<'a, T, A> { - fn drop(&mut self) { - self.deque.len += self.written; - } - } - let head_room = self.capacity() - dst.as_index(); - let mut guard = Guard { deque: self, written: 0 }; + let mut guard = DropGuard::new((self, 0), |(deque, written)| { + deque.len += written; + }); + let (deque, written) = &mut *guard; if head_room >= len { - unsafe { guard.deque.write_iter(dst, iter, &mut guard.written) }; + unsafe { deque.write_iter(dst, iter, written) }; } else { unsafe { - guard.deque.write_iter( - dst, - ByRefSized(&mut iter).take(head_room), - &mut guard.written, - ); - guard.deque.write_iter(WrappedIndex::zero(), iter, &mut guard.written) + deque.write_iter(dst, ByRefSized(&mut iter).take(head_room), written); + deque.write_iter(WrappedIndex::zero(), iter, written) }; } - guard.written + *written } /// Frobs the head and tail sections around to handle the fact that we diff --git a/library/alloc/src/rc.rs b/library/alloc/src/rc.rs index e4a803f28e121..603c0f1c13c9d 100644 --- a/library/alloc/src/rc.rs +++ b/library/alloc/src/rc.rs @@ -2388,45 +2388,31 @@ impl Rc<[T]> { /// Behavior is undefined should the size be wrong. #[cfg(not(no_global_oom_handling))] unsafe fn from_iter_exact(iter: impl Iterator, len: usize) -> Rc<[T]> { - // Panic guard while cloning T elements. - // In the event of a panic, elements that have been written - // into the new RcInner will be dropped, then the memory freed. - struct Guard { - mem: NonNull, - elems: *mut T, - layout: Layout, - n_elems: usize, - } - - impl Drop for Guard { - fn drop(&mut self) { - unsafe { - let slice = from_raw_parts_mut(self.elems, self.n_elems); - ptr::drop_in_place(slice); - - Global.deallocate(self.mem, self.layout); - } - } - } + use core::mem::DropGuard; unsafe { let ptr = Self::allocate_for_slice(len); - - let mem = ptr as *mut _ as *mut u8; let layout = Layout::for_value_raw(ptr); // Pointer to first element - let elems = (&raw mut (*ptr).value) as *mut T; + let elems = (&raw mut (*ptr).value).as_mut_ptr(); - let mut guard = Guard { mem: NonNull::new_unchecked(mem), elems, layout, n_elems: 0 }; + // Panic guard while cloning T elements. + // In the event of a panic, elements that have been written + // into the new RcInner will be dropped, then the memory freed. + let mut guard = DropGuard::new(0, |n_elems| { + let slice = from_raw_parts_mut(elems, n_elems); + ptr::drop_in_place(slice); + Global.deallocate(NonNull::new_unchecked(ptr.cast()), layout); + }); for (i, item) in iter.enumerate() { ptr::write(elems.add(i), item); - guard.n_elems += 1; + *guard += 1; } - // All clear. Forget the guard so it doesn't free the new RcInner. - mem::forget(guard); + // All clear. Dismiss the guard so it doesn't free the new RcInner. + DropGuard::dismiss(guard); Self::from_ptr(ptr) } diff --git a/library/alloc/src/slice.rs b/library/alloc/src/slice.rs index e6b540f093ba5..ff7dc0f4ea17d 100644 --- a/library/alloc/src/slice.rs +++ b/library/alloc/src/slice.rs @@ -408,35 +408,30 @@ impl [T] { impl ConvertVec for T { #[inline] default fn to_vec(s: &[Self], alloc: A) -> Vec { - struct DropGuard<'a, T, A: Allocator> { - vec: &'a mut Vec, - num_init: usize, - } - impl<'a, T, A: Allocator> Drop for DropGuard<'a, T, A> { - #[inline] - fn drop(&mut self) { + use core::mem::DropGuard; + + let mut guard = DropGuard::new( + (0, Vec::with_capacity_in(s.len(), alloc)), + |(num_init, mut vec)| { // SAFETY: // items were marked initialized in the loop below - unsafe { - self.vec.set_len(self.num_init); - } - } - } - let mut vec = Vec::with_capacity_in(s.len(), alloc); - let mut guard = DropGuard { vec: &mut vec, num_init: 0 }; - let slots = guard.vec.spare_capacity_mut(); + unsafe { vec.set_len(num_init) } + }, + ); + let (num_init, vec) = &mut *guard; + + let slots = vec.spare_capacity_mut(); // .take(slots.len()) is necessary for LLVM to remove bounds checks // and has better codegen than zip. for (i, b) in s.iter().enumerate().take(slots.len()) { - guard.num_init = i; + *num_init = i; slots[i].write(b.clone()); } - core::mem::forget(guard); + + let (_, mut vec) = DropGuard::dismiss(guard); // SAFETY: // the vec was allocated and initialized above to at least this length. - unsafe { - vec.set_len(s.len()); - } + unsafe { vec.set_len(s.len()) }; vec } } diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 625a29dd9b7a0..56b89411e3ce1 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -19,6 +19,8 @@ use core::intrinsics::abort; #[cfg(not(no_global_oom_handling))] use core::iter; use core::marker::{PhantomData, Unsize}; +#[cfg(not(no_global_oom_handling))] +use core::mem::DropGuard; use core::mem::{self, Alignment, ManuallyDrop}; use core::num::NonZeroUsize; use core::ops::{CoerceUnsized, Deref, DerefMut, DerefPure, DispatchFromDyn, LegacyReceiver}; @@ -2352,45 +2354,30 @@ impl Arc<[T]> { /// Behavior is undefined should the size be wrong. #[cfg(not(no_global_oom_handling))] unsafe fn from_iter_exact(iter: impl Iterator, len: usize) -> Arc<[T]> { - // Panic guard while cloning T elements. - // In the event of a panic, elements that have been written - // into the new ArcInner will be dropped, then the memory freed. - struct Guard { - mem: NonNull, - elems: *mut T, - layout: Layout, - n_elems: usize, - } - - impl Drop for Guard { - fn drop(&mut self) { - unsafe { - let slice = from_raw_parts_mut(self.elems, self.n_elems); - ptr::drop_in_place(slice); - - Global.deallocate(self.mem, self.layout); - } - } - } - unsafe { let ptr = Self::allocate_for_slice(len); - - let mem = ptr as *mut _ as *mut u8; let layout = Layout::for_value_raw(ptr); // Pointer to first element - let elems = (&raw mut (*ptr).data) as *mut T; + let elems = (&raw mut (*ptr).data).as_mut_ptr(); + + // Panic guard while cloning T elements. + // In the event of a panic, elements that have been written + // into the new ArcInner will be dropped, then the memory freed. + let mut guard = DropGuard::new(0, |n_elems| { + let slice = from_raw_parts_mut(elems, n_elems); + ptr::drop_in_place(slice); - let mut guard = Guard { mem: NonNull::new_unchecked(mem), elems, layout, n_elems: 0 }; + Global.deallocate(NonNull::new_unchecked(ptr.cast()), layout); + }); for (i, item) in iter.enumerate() { ptr::write(elems.add(i), item); - guard.n_elems += 1; + *guard += 1; } - // All clear. Forget the guard so it doesn't free the new ArcInner. - mem::forget(guard); + // All clear. Dismiss the guard so it doesn't free the new ArcInner. + DropGuard::dismiss(guard); Self::from_ptr(ptr) } @@ -2608,15 +2595,7 @@ impl Arc { // If we unwind before the Arc is overwritten, we expose a strong // count of 0, resulting in a UAF (#155746, #157203). // Until the new Arc is written, the old Arc must remain valid - struct Guard<'a, T: ?Sized> { - inner: &'a ArcInner, - } - impl<'a, T: ?Sized> Drop for Guard<'a, T> { - fn drop(&mut self) { - self.inner.strong.store(1, Release); - } - } - let guard = Guard { inner: this.inner() }; + let guard = DropGuard::new(this.inner(), |inner| inner.strong.store(1, Release)); // Can just steal the data, all that's left is Weaks // Note that this can panic in two ways: @@ -2636,7 +2615,7 @@ impl Arc { ); // We are now safe from panics. - mem::forget(guard); + DropGuard::dismiss(guard); // Materialize our own implicit weak pointer, so that it can clean // up the ArcInner as needed. diff --git a/library/alloc/src/vec/drain.rs b/library/alloc/src/vec/drain.rs index d12dea20b33cb..327fa00233863 100644 --- a/library/alloc/src/vec/drain.rs +++ b/library/alloc/src/vec/drain.rs @@ -1,5 +1,5 @@ use core::iter::{FusedIterator, TrustedLen}; -use core::mem::{self, ManuallyDrop, SizedTypeProperties}; +use core::mem::{self, DropGuard, ManuallyDrop, SizedTypeProperties}; use core::ptr::{self, NonNull}; use core::{fmt, slice}; @@ -172,28 +172,6 @@ impl DoubleEndedIterator for Drain<'_, T, A> { #[stable(feature = "drain", since = "1.6.0")] impl Drop for Drain<'_, T, A> { fn drop(&mut self) { - /// Moves back the un-`Drain`ed elements to restore the original `Vec`. - struct DropGuard<'r, 'a, T, A: Allocator>(&'r mut Drain<'a, T, A>); - - impl<'r, 'a, T, A: Allocator> Drop for DropGuard<'r, 'a, T, A> { - fn drop(&mut self) { - if self.0.tail_len > 0 { - unsafe { - let source_vec = self.0.vec.as_mut(); - // memmove back untouched tail, update to new length - let start = source_vec.len(); - let tail = self.0.tail_start; - if tail != start { - let src = source_vec.as_ptr().add(tail); - let dst = source_vec.as_mut_ptr().add(start); - ptr::copy(src, dst, self.0.tail_len); - } - source_vec.set_len(start + self.0.tail_len); - } - } - } - } - let iter = mem::take(&mut self.iter); let drop_len = iter.len(); @@ -213,7 +191,22 @@ impl Drop for Drain<'_, T, A> { } // ensure elements are moved back into their appropriate places, even when drop_in_place panics - let _guard = DropGuard(self); + let _guard = DropGuard::new(self, |this| { + if this.tail_len > 0 { + unsafe { + let source_vec = this.vec.as_mut(); + // memmove back untouched tail, update to new length + let start = source_vec.len(); + let tail = this.tail_start; + if tail != start { + let src = source_vec.as_ptr().add(tail); + let dst = source_vec.as_mut_ptr().add(start); + ptr::copy(src, dst, this.tail_len); + } + source_vec.set_len(start + this.tail_len); + } + } + }); if drop_len == 0 { return; diff --git a/library/alloc/src/vec/into_iter.rs b/library/alloc/src/vec/into_iter.rs index 4b25634326e16..7c486aed84d62 100644 --- a/library/alloc/src/vec/into_iter.rs +++ b/library/alloc/src/vec/into_iter.rs @@ -3,7 +3,7 @@ use core::iter::{ TrustedRandomAccessNoCoerce, }; use core::marker::PhantomData; -use core::mem::{ManuallyDrop, MaybeUninit, SizedTypeProperties}; +use core::mem::{DropGuard, ManuallyDrop, MaybeUninit, SizedTypeProperties}; use core::num::NonZero; #[cfg(not(no_global_oom_handling))] use core::ops::Deref; @@ -581,21 +581,9 @@ impl Clone for IntoIter { #[stable(feature = "rust1", since = "1.0.0")] unsafe impl<#[may_dangle] T, A: Allocator> Drop for IntoIter { fn drop(&mut self) { - struct DropGuard<'a, T, A: Allocator>(&'a mut IntoIter); - - impl Drop for DropGuard<'_, T, A> { - fn drop(&mut self) { - unsafe { - self.0.dealloc_only(); - } - } - } - - let guard = DropGuard(self); + let mut guard = DropGuard::new(self, |this| unsafe { this.dealloc_only() }); // destroy the remaining elements - unsafe { - ptr::drop_in_place(guard.0.as_raw_mut_slice()); - } + unsafe { ptr::drop_in_place(guard.as_raw_mut_slice()) } // now `guard` will be dropped and do the rest } } diff --git a/library/alloctests/lib.rs b/library/alloctests/lib.rs index 83b017b7625b9..7e4cc3dbaa43e 100644 --- a/library/alloctests/lib.rs +++ b/library/alloctests/lib.rs @@ -28,6 +28,7 @@ #![feature(const_try)] #![feature(copied_into_inner)] #![feature(core_intrinsics)] +#![feature(drop_guard)] #![feature(exact_size_is_empty)] #![feature(extend_one)] #![feature(extend_one_unchecked)] diff --git a/library/std/src/sys/fs/unix.rs b/library/std/src/sys/fs/unix.rs index b33ebadebe4ad..aff608fa56aa3 100644 --- a/library/std/src/sys/fs/unix.rs +++ b/library/std/src/sys/fs/unix.rs @@ -2239,19 +2239,6 @@ pub fn copy(from: &Path, to: &Path) -> io::Result { #[cfg(target_vendor = "apple")] pub fn copy(from: &Path, to: &Path) -> io::Result { const COPYFILE_ALL: libc::copyfile_flags_t = libc::COPYFILE_METADATA | libc::COPYFILE_DATA; - - struct FreeOnDrop(libc::copyfile_state_t); - impl Drop for FreeOnDrop { - fn drop(&mut self) { - // The code below ensures that `FreeOnDrop` is never a null pointer - unsafe { - // `copyfile_state_free` returns -1 if the `to` or `from` files - // cannot be closed. However, this is not considered an error. - libc::copyfile_state_free(self.0); - } - } - } - let (reader, reader_metadata) = open_from(from)?; let clonefile_result = run_path_with_cstr(to, &|to| { @@ -2272,24 +2259,29 @@ pub fn copy(from: &Path, to: &Path) -> io::Result { // Fall back to using `fcopyfile` if `fclonefileat` does not succeed. let (writer, writer_metadata) = open_to_and_set_permissions(to, &reader_metadata)?; - // We ensure that `FreeOnDrop` never contains a null pointer so it is + let state = unsafe { libc::copyfile_state_alloc() }; + // We ensure that the guard never contains a null pointer so it is // always safe to call `copyfile_state_free` - let state = unsafe { - let state = libc::copyfile_state_alloc(); - if state.is_null() { - return Err(crate::io::Error::last_os_error()); + if state.is_null() { + return Err(crate::io::Error::last_os_error()); + } + let state = crate::mem::DropGuard::new(state, |state| { + // SAFETY: just checked it's not null + unsafe { + // `copyfile_state_free` returns -1 if the `to` or `from` files + // cannot be closed. However, this is not considered an error. + libc::copyfile_state_free(state); } - FreeOnDrop(state) - }; + }); let flags = if writer_metadata.is_file() { COPYFILE_ALL } else { libc::COPYFILE_DATA }; - cvt(unsafe { libc::fcopyfile(reader.as_raw_fd(), writer.as_raw_fd(), state.0, flags) })?; + cvt(unsafe { libc::fcopyfile(reader.as_raw_fd(), writer.as_raw_fd(), *state, flags) })?; let mut bytes_copied: libc::off_t = 0; cvt(unsafe { libc::copyfile_state_get( - state.0, + *state, libc::COPYFILE_STATE_COPIED as u32, (&raw mut bytes_copied) as *mut libc::c_void, ) diff --git a/library/std/src/sys/pal/unix/sync/condvar.rs b/library/std/src/sys/pal/unix/sync/condvar.rs index 51162296ea3fc..4cbb41a370bbf 100644 --- a/library/std/src/sys/pal/unix/sync/condvar.rs +++ b/library/std/src/sys/pal/unix/sync/condvar.rs @@ -150,26 +150,19 @@ impl Condvar { /// # Safety /// May only be called once per instance of `Self`. pub unsafe fn init(self: Pin<&mut Self>) { - use crate::mem::MaybeUninit; - - struct AttrGuard<'a>(pub &'a mut MaybeUninit); - impl Drop for AttrGuard<'_> { - fn drop(&mut self) { - unsafe { - let result = libc::pthread_condattr_destroy(self.0.as_mut_ptr()); - assert_eq!(result, 0); - } - } - } + use crate::mem::{DropGuard, MaybeUninit}; unsafe { let mut attr = MaybeUninit::::uninit(); let r = libc::pthread_condattr_init(attr.as_mut_ptr()); assert_eq!(r, 0); - let attr = AttrGuard(&mut attr); - let r = libc::pthread_condattr_setclock(attr.0.as_mut_ptr(), Self::CLOCK); + let mut attr = DropGuard::new(&mut attr, |attr| { + let result = libc::pthread_condattr_destroy(attr.as_mut_ptr()); + assert_eq!(result, 0); + }); + let r = libc::pthread_condattr_setclock(attr.as_mut_ptr(), Self::CLOCK); assert_eq!(r, 0); - let r = libc::pthread_cond_init(self.raw(), attr.0.as_ptr()); + let r = libc::pthread_cond_init(self.raw(), attr.as_ptr()); assert_eq!(r, 0); } } diff --git a/library/std/src/sys/process/unix/unix.rs b/library/std/src/sys/process/unix/unix.rs index 6103fa3576f37..0b99b2433272d 100644 --- a/library/std/src/sys/process/unix/unix.rs +++ b/library/std/src/sys/process/unix/unix.rs @@ -394,19 +394,13 @@ impl Command { // want to be sure to restore the global environment back to what it // once was, ensuring that our temporary override, when free'd, doesn't // corrupt our process's environment. - let mut _reset = None; + let _reset; if let Some(envp) = maybe_envp { - struct Reset(*const *const libc::c_char); + use core::mem::DropGuard; - impl Drop for Reset { - fn drop(&mut self) { - unsafe { - *sys::env::environ() = self.0; - } - } - } - - _reset = Some(Reset(*sys::env::environ())); + _reset = DropGuard::new(*sys::env::environ(), |prev| unsafe { + *sys::env::environ() = prev; + }); *sys::env::environ() = envp.as_ptr(); } @@ -677,65 +671,51 @@ impl Command { let pgroup = self.get_pgroup(); - struct PosixSpawnFileActions<'a>(&'a mut MaybeUninit); - - impl Drop for PosixSpawnFileActions<'_> { - fn drop(&mut self) { - unsafe { - libc::posix_spawn_file_actions_destroy(self.0.as_mut_ptr()); - } - } - } - - struct PosixSpawnattr<'a>(&'a mut MaybeUninit); - - impl Drop for PosixSpawnattr<'_> { - fn drop(&mut self) { - unsafe { - libc::posix_spawnattr_destroy(self.0.as_mut_ptr()); - } - } - } - unsafe { + use core::mem::DropGuard; + let mut attrs = MaybeUninit::uninit(); cvt_nz(libc::posix_spawnattr_init(attrs.as_mut_ptr()))?; - let attrs = PosixSpawnattr(&mut attrs); + let mut attrs = DropGuard::new(&mut attrs, |attrs| { + libc::posix_spawnattr_destroy(attrs.as_mut_ptr()); + }); let mut flags = 0; let mut file_actions = MaybeUninit::uninit(); cvt_nz(libc::posix_spawn_file_actions_init(file_actions.as_mut_ptr()))?; - let file_actions = PosixSpawnFileActions(&mut file_actions); + let mut file_actions = DropGuard::new(&mut file_actions, |file_actions| { + libc::posix_spawn_file_actions_destroy(file_actions.as_mut_ptr()); + }); if let Some(fd) = stdio.stdin.fd() { cvt_nz(libc::posix_spawn_file_actions_adddup2( - file_actions.0.as_mut_ptr(), + file_actions.as_mut_ptr(), fd, libc::STDIN_FILENO, ))?; } if let Some(fd) = stdio.stdout.fd() { cvt_nz(libc::posix_spawn_file_actions_adddup2( - file_actions.0.as_mut_ptr(), + file_actions.as_mut_ptr(), fd, libc::STDOUT_FILENO, ))?; } if let Some(fd) = stdio.stderr.fd() { cvt_nz(libc::posix_spawn_file_actions_adddup2( - file_actions.0.as_mut_ptr(), + file_actions.as_mut_ptr(), fd, libc::STDERR_FILENO, ))?; } if let Some((f, cwd)) = addchdir { - cvt_nz(f(file_actions.0.as_mut_ptr(), cwd.as_ptr()))?; + cvt_nz(f(file_actions.as_mut_ptr(), cwd.as_ptr()))?; } if let Some(pgroup) = pgroup { flags |= libc::POSIX_SPAWN_SETPGROUP; - cvt_nz(libc::posix_spawnattr_setpgroup(attrs.0.as_mut_ptr(), pgroup))?; + cvt_nz(libc::posix_spawnattr_setpgroup(attrs.as_mut_ptr(), pgroup))?; } // Inherit the signal mask from this process rather than resetting it (i.e. do not call @@ -754,7 +734,7 @@ impl Command { cvt(sigaddset(default_set.as_mut_ptr(), libc::SIGLOST))?; } cvt_nz(libc::posix_spawnattr_setsigdefault( - attrs.0.as_mut_ptr(), + attrs.as_mut_ptr(), default_set.as_ptr(), ))?; flags |= libc::POSIX_SPAWN_SETSIGDEF; @@ -771,7 +751,7 @@ impl Command { } } - cvt_nz(libc::posix_spawnattr_setflags(attrs.0.as_mut_ptr(), flags as _))?; + cvt_nz(libc::posix_spawnattr_setflags(attrs.as_mut_ptr(), flags as _))?; // Make sure we synchronize access to the global `environ` resource let _env_lock = sys::env::env_read_lock(); @@ -788,8 +768,8 @@ impl Command { let spawn_res = pidfd_spawnp.get().unwrap()( &mut pidfd, self.get_program_cstr().as_ptr(), - file_actions.0.as_ptr(), - attrs.0.as_ptr(), + file_actions.as_ptr(), + attrs.as_ptr(), self.get_argv().as_ptr() as *const _, envp as *const _, ); @@ -830,8 +810,8 @@ impl Command { let spawn_res = spawn_fn( &mut p.pid, self.get_program_cstr().as_ptr(), - file_actions.0.as_ptr(), - attrs.0.as_ptr(), + file_actions.as_ptr(), + attrs.as_ptr(), self.get_argv().as_ptr() as *const _, envp as *const _, ); diff --git a/library/std/src/sys/process/windows/tests.rs b/library/std/src/sys/process/windows/tests.rs index bc5e0d5c7fc97..b6fc36b1af5d1 100644 --- a/library/std/src/sys/process/windows/tests.rs +++ b/library/std/src/sys/process/windows/tests.rs @@ -1,6 +1,7 @@ use super::child_pipe::{Pipes, child_pipe}; use super::{Arg, make_command_line}; use crate::ffi::{OsStr, OsString}; +use crate::mem::DropGuard; use crate::os::windows::io::AsHandle; use crate::process::{Command, Stdio}; use crate::time::Duration; @@ -36,14 +37,7 @@ fn test_thread_handle() { assert!(p.is_ok()); // Ensure the process is killed in the event something goes wrong. - struct DropGuard(crate::process::Child); - impl Drop for DropGuard { - fn drop(&mut self) { - let _ = self.0.kill(); - } - } - let mut p = DropGuard(p.unwrap()); - let p = &mut p.0; + let mut p = DropGuard::new(p.unwrap(), |p| p.kill()); unsafe extern "system" { unsafe fn ResumeThread(hHandle: BorrowedHandle<'_>) -> u32;