Replace mem::forget with ManuallyDrop::new (#3515)

* Replace `mem::forget` with `ManuallyDrop::new`

It's way less footgunny w.r.t. pointer aliasing:
  - At worst, it's equivalent;
  - But the advantage of `ManuallyDrop::new()`, is that it can be used
    _before_ extracting data off the value, such as pointers, thereby
    avoiding the danger of aliasing invalidation that `mem::forget` may
    apply.


Co-authored-by: Daniel Henry-Mantilla <[email protected]>

Author: Manishearth

utils/zerovec/src/ule/encode.rs

@@ -86,12 +86,11 @@ pub fn encode_varule_to_box<S: EncodeAsVarULE<T>, T: VarULE + ?Sized>(x: &S) ->
     // zero-fill the vector to avoid uninitialized data UB
     vec.resize(x.encode_var_ule_len(), 0);
     x.encode_var_ule_write(&mut vec);
-    let boxed = vec.into_boxed_slice();
+    let boxed = mem::ManuallyDrop::new(vec.into_boxed_slice());
     unsafe {
         // Safety: `ptr` is a box, and `T` is a VarULE which guarantees it has the same memory layout as `[u8]`
         // and can be recouped via from_byte_slice_unchecked()
         let ptr: *mut T = T::from_byte_slice_unchecked(&boxed) as *const T as *mut T;
-        mem::forget(boxed);
 
         // Safety: we can construct an owned version since we have mem::forgotten the older owner
         Box::from_raw(ptr)

utils/zerovec/src/ule/mod.rs

@@ -357,13 +357,12 @@ pub unsafe trait VarULE: 'static {
     #[inline]
     fn to_boxed(&self) -> Box<Self> {
         let bytesvec = self.as_byte_slice().to_owned().into_boxed_slice();
+        let bytesvec = mem::ManuallyDrop::new(bytesvec);
         unsafe {
             // Get the pointer representation
             let ptr: *mut Self =
                 Self::from_byte_slice_unchecked(&bytesvec) as *const Self as *mut Self;
-            assert_eq!(Layout::for_value(&*ptr), Layout::for_value(&*bytesvec));
-            // Forget the allocation
-            mem::forget(bytesvec);
+            assert_eq!(Layout::for_value(&*ptr), Layout::for_value(&**bytesvec));
             // Transmute the pointer to an owned pointer
             Box::from_raw(ptr)
         }

utils/zerovec/src/yoke_impls.rs

@@ -33,8 +33,8 @@ unsafe impl<'a, T: 'static + AsULE + ?Sized> Yokeable<'a> for ZeroVec<'static, T
     #[inline]
     unsafe fn make(from: Self::Output) -> Self {
         debug_assert!(mem::size_of::<Self::Output>() == mem::size_of::<Self>());
-        let ptr: *const Self = (&from as *const Self::Output).cast();
-        mem::forget(from);
+        let from = mem::ManuallyDrop::new(from);
+        let ptr: *const Self = (&*from as *const Self::Output).cast();
         ptr::read(ptr)
     }
     #[inline]
@@ -61,8 +61,8 @@ unsafe impl<'a, T: 'static + VarULE + ?Sized> Yokeable<'a> for VarZeroVec<'stati
     #[inline]
     unsafe fn make(from: Self::Output) -> Self {
         debug_assert!(mem::size_of::<Self::Output>() == mem::size_of::<Self>());
-        let ptr: *const Self = (&from as *const Self::Output).cast();
-        mem::forget(from);
+        let from = mem::ManuallyDrop::new(from);
+        let ptr: *const Self = (&*from as *const Self::Output).cast();
         ptr::read(ptr)
     }
     #[inline]
@@ -89,8 +89,8 @@ unsafe impl<'a> Yokeable<'a> for FlexZeroVec<'static> {
     #[inline]
     unsafe fn make(from: Self::Output) -> Self {
         debug_assert!(mem::size_of::<Self::Output>() == mem::size_of::<Self>());
-        let ptr: *const Self = (&from as *const Self::Output).cast();
-        mem::forget(from);
+        let from = mem::ManuallyDrop::new(from);
+        let ptr: *const Self = (&*from as *const Self::Output).cast();
         ptr::read(ptr)
     }
     #[inline]
@@ -127,16 +127,16 @@ where
         unsafe {
             // Similar problem as transform(), but we need to use ptr::read since
             // the compiler isn't sure of the sizes
-            let ptr: *const Self::Output = (&self as *const Self).cast();
-            mem::forget(self);
+            let this = mem::ManuallyDrop::new(self);
+            let ptr: *const Self::Output = (&*this as *const Self).cast();
             ptr::read(ptr)
         }
     }
     #[inline]
     unsafe fn make(from: Self::Output) -> Self {
         debug_assert!(mem::size_of::<Self::Output>() == mem::size_of::<Self>());
-        let ptr: *const Self = (&from as *const Self::Output).cast();
-        mem::forget(from);
+        let from = mem::ManuallyDrop::new(from);
+        let ptr: *const Self = (&*from as *const Self::Output).cast();
         ptr::read(ptr)
     }
     #[inline]
@@ -173,16 +173,16 @@ where
         unsafe {
             // Similar problem as transform(), but we need to use ptr::read since
             // the compiler isn't sure of the sizes
-            let ptr: *const Self::Output = (&self as *const Self).cast();
-            mem::forget(self);
+            let this = mem::ManuallyDrop::new(self);
+            let ptr: *const Self::Output = (&*this as *const Self).cast();
             ptr::read(ptr)
         }
     }
     #[inline]
     unsafe fn make(from: Self::Output) -> Self {
         debug_assert!(mem::size_of::<Self::Output>() == mem::size_of::<Self>());
-        let ptr: *const Self = (&from as *const Self::Output).cast();
-        mem::forget(from);
+        let from = mem::ManuallyDrop::new(from);
+        let ptr: *const Self = (&*from as *const Self::Output).cast();
         ptr::read(ptr)
     }
     #[inline]
@@ -221,16 +221,16 @@ where
         unsafe {
             // Similar problem as transform(), but we need to use ptr::read since
             // the compiler isn't sure of the sizes
-            let ptr: *const Self::Output = (&self as *const Self).cast();
-            mem::forget(self);
+            let this = mem::ManuallyDrop::new(self);
+            let ptr: *const Self::Output = (&*this as *const Self).cast();
             ptr::read(ptr)
         }
     }
     #[inline]
     unsafe fn make(from: Self::Output) -> Self {
         debug_assert!(mem::size_of::<Self::Output>() == mem::size_of::<Self>());
-        let ptr: *const Self = (&from as *const Self::Output).cast();
-        mem::forget(from);
+        let from = mem::ManuallyDrop::new(from);
+        let ptr: *const Self = (&*from as *const Self::Output).cast();
         ptr::read(ptr)
     }
     #[inline]
@@ -269,16 +269,16 @@ where
         unsafe {
             // Similar problem as transform(), but we need to use ptr::read since
             // the compiler isn't sure of the sizes
-            let ptr: *const Self::Output = (&self as *const Self).cast();
-            mem::forget(self);
+            let this = mem::ManuallyDrop::new(self);
+            let ptr: *const Self::Output = (&*this as *const Self).cast();
             ptr::read(ptr)
         }
     }
     #[inline]
     unsafe fn make(from: Self::Output) -> Self {
         debug_assert!(mem::size_of::<Self::Output>() == mem::size_of::<Self>());
-        let ptr: *const Self = (&from as *const Self::Output).cast();
-        mem::forget(from);
+        let from = mem::ManuallyDrop::new(from);
+        let ptr: *const Self = (&*from as *const Self::Output).cast();
         ptr::read(ptr)
     }
     #[inline]

utils/zerovec/src/zerovec/mod.rs

@@ -22,6 +22,7 @@ use core::marker::PhantomData;
 use core::mem;
 use core::num::NonZeroUsize;
 use core::ops::Deref;
+use core::ptr;
 
 /// A zero-copy, byte-aligned vector for fixed-width types.
 ///
@@ -286,14 +287,14 @@ where
     /// If you have a slice of `&[T]`s, prefer using
     /// [`Self::alloc_from_slice()`].
     #[inline]
-    pub fn new_owned(mut vec: Vec<T::ULE>) -> Self {
+    pub fn new_owned(vec: Vec<T::ULE>) -> Self {
         // Deconstruct the vector into parts
         // This is the only part of the code that goes from Vec
         // to ZeroVec, all other such operations should use this function
-        let slice: &mut [T::ULE] = &mut vec;
-        let slice = slice as *mut [_];
         let capacity = vec.capacity();
-        mem::forget(vec);
+        let len = vec.len();
+        let ptr = mem::ManuallyDrop::new(vec).as_mut_ptr();
+        let slice = ptr::slice_from_raw_parts_mut(ptr, len);
         Self {
             vector: EyepatchHackVector {
                 buf: slice,
@@ -894,21 +895,18 @@ where
     /// the logical equivalent of this type's internal representation
     #[inline]
     pub fn into_cow(self) -> Cow<'a, [T::ULE]> {
-        if self.is_owned() {
+        let this = mem::ManuallyDrop::new(self);
+        if this.is_owned() {
             let vec = unsafe {
                 // safe to call: we know it's owned,
-                // and we mem::forget self immediately afterwards
-                self.vector.get_vec()
+                // and `self`/`this` are thenceforth no longer used or dropped
+                { this }.vector.get_vec()
             };
-            mem::forget(self);
             Cow::Owned(vec)
         } else {
             // We can extend the lifetime of the slice to 'a
             // since we know it is borrowed
-            let slice = unsafe { self.vector.as_arbitrary_slice() };
-            // The borrowed destructor is a no-op, but we want to prevent
-            // the check being run
-            mem::forget(self);
+            let slice = unsafe { { this }.vector.as_arbitrary_slice() };
             Cow::Borrowed(slice)
         }
     }