Fix aliasing bugs revealed by Tree Borrows (#889)

This PR fixes some aliasing bugs caused by improper usage of shared and
mutable references intermixed with raw pointer accesses. These have been
discovered by running the testsuite under [Tree
Borrows](https://www.ralfj.de/blog/2023/06/02/tree-borrows.html), a new
aliasing model for Rust and a potential replacement for Stacked Borrows.
While TB is in general more lenient than SB, there are some cases where
it is more strict due to SB relying on gross hacks, which have been
removed from TB.

Turns out that `deno_core` relied on such hacks. The problem is with the
`DynFutureInfoErased`, which has arbitrary data that is being modified
by a future while other references to it are held in the runtime. To
prevent this, that is put into an `UnsafeCell` which makes such
modifications allowed.

But `UnsafeCell` only ensures that shared references can be modified
without causing UB, it does _not_ permit mutable references to be
aliased. Thus, one should take care not to create mutable references
here. The easiest way of doing this is to remove the implementation of
`DerefMut` and `AsMut` for `ArenaBox<T>`, and work around the few cases
where that leads to breakage (by using raw pointers, which is also more
robust in terms of the aliasing model).

This fixes #884. See that issue for more information.

I have not audited the entire crate, so it is possible that other TB
bugs are lurking elsewhere; especially in the parts that Miri can
currently not test properly. But this at least makes all the test that
could work, do work.

Author: JoJoDeveloping

core/arena/unique_arena.rs

@@ -60,8 +60,8 @@ impl<T: 'static> ArenaBox<T> {
   /// This function returns a raw pointer without managing the memory, potentially leading to
   /// memory leaks if the pointer is not properly handled or deallocated.
   #[inline(always)]
-  pub fn into_raw(mut alloc: ArenaBox<T>) -> NonNull<T> {
-    let ptr = NonNull::from(alloc.data_mut());
+  pub fn into_raw(alloc: ArenaBox<T>) -> NonNull<T> {
+    let ptr: NonNull<ArenaBoxData<T>> = alloc.ptr;
     std::mem::forget(alloc);
     unsafe { Self::ptr_from_data(ptr) }
   }
@@ -94,8 +94,10 @@ impl<T> ArenaBox<T> {
   }
 
   #[inline(always)]
-  fn data_mut(&mut self) -> &mut ArenaBoxData<T> {
-    unsafe { self.ptr.as_mut() }
+  pub(crate) fn deref_data(&self) -> NonNull<T> {
+    unsafe {
+      NonNull::new_unchecked(std::ptr::addr_of_mut!((*self.ptr.as_ptr()).data))
+    }
   }
 }
 
@@ -123,20 +125,6 @@ impl<T> std::convert::AsRef<T> for ArenaBox<T> {
   }
 }
 
-impl<T> std::ops::DerefMut for ArenaBox<T> {
-  #[inline(always)]
-  fn deref_mut(&mut self) -> &mut Self::Target {
-    &mut self.data_mut().data
-  }
-}
-
-impl<T> std::convert::AsMut<T> for ArenaBox<T> {
-  #[inline(always)]
-  fn as_mut(&mut self) -> &mut T {
-    &mut self.data_mut().data
-  }
-}
-
 impl<F, R> std::future::Future for ArenaBox<F>
 where
   F: Future<Output = R>,
@@ -145,10 +133,10 @@ where
 
   #[inline(always)]
   fn poll(
-    mut self: Pin<&mut Self>,
+    self: Pin<&mut Self>,
     cx: &mut std::task::Context<'_>,
   ) -> std::task::Poll<Self::Output> {
-    unsafe { F::poll(Pin::new_unchecked(&mut self.data_mut().data), cx) }
+    unsafe { F::poll(Pin::new_unchecked(self.deref_data().as_mut()), cx) }
   }
 }
 

core/runtime/op_driver/erased_future.rs

@@ -41,8 +41,11 @@ impl<const MAX_SIZE: usize> TypeErased<MAX_SIZE> {
   }
 
   #[inline(always)]
-  pub fn raw_ptr<R>(&mut self) -> NonNull<R> {
-    unsafe { NonNull::new_unchecked(self.memory.as_mut_ptr() as *mut _) }
+  /// Safety: This uses a place projection to `this.memory`, which must be in-bounds.
+  pub unsafe fn raw_ptr<R>(this: *mut Self) -> NonNull<R> {
+    unsafe {
+      NonNull::new_unchecked(std::ptr::addr_of_mut!((*this).memory) as *mut _)
+    }
   }
 
   #[inline(always)]
@@ -73,7 +76,7 @@ impl<const MAX_SIZE: usize> TypeErased<MAX_SIZE> {
 
 impl<const MAX_SIZE: usize> Drop for TypeErased<MAX_SIZE> {
   fn drop(&mut self) {
-    (self.drop)(self.raw_ptr::<()>())
+    (self.drop)(unsafe { Self::raw_ptr(self) })
   }
 }
 

core/runtime/op_driver/future_arena.rs

@@ -4,6 +4,7 @@ use super::erased_future::TypeErased;
 use crate::arena::ArenaBox;
 use crate::arena::ArenaUnique;
 use pin_project::pin_project;
+use std::cell::UnsafeCell;
 use std::future::Future;
 use std::marker::PhantomData;
 use std::mem::MaybeUninit;
@@ -26,7 +27,7 @@ pub trait FutureContextMapper<T, C, R> {
 
 struct DynFutureInfoErased<T, C> {
   ptr: MaybeUninit<NonNull<dyn ContextFuture<T, C>>>,
-  data: TypeErased<MAX_ARENA_FUTURE_SIZE>,
+  data: UnsafeCell<TypeErased<MAX_ARENA_FUTURE_SIZE>>,
 }
 
 pub trait ContextFuture<T, C>: Future<Output = T> {
@@ -191,19 +192,20 @@ impl<T, C: Clone> FutureArena<T, C> {
     {
       unsafe {
         if let Some(reservation) = self.arena.reserve_space() {
-          let mut alloc = self.arena.complete_reservation(
+          let alloc = self.arena.complete_reservation(
             reservation,
             DynFutureInfoErased {
               ptr: MaybeUninit::uninit(),
-              data: TypeErased::new(DynFutureInfo {
+              data: UnsafeCell::new(TypeErased::new(DynFutureInfo {
                 context,
                 future,
                 _phantom: PhantomData,
-              }),
+              })),
             },
           );
-          let ptr = alloc.data.raw_ptr::<DynFutureInfo<T, C, M, F>>();
-          alloc.ptr.write(ptr);
+          let ptr =
+            TypeErased::raw_ptr::<DynFutureInfo<T, C, M, F>>(alloc.data.get());
+          (*alloc.deref_data().as_ptr()).ptr.write(ptr);
           return TypedFutureAllocation {
             inner: FutureAllocation::Arena(alloc),
             ptr,