Tree Borrows support

[JoJoDeveloping]

Hi all,

I'm one of the people working on Tree Borrows, a candidate for a successor for Stacked Borrows. Stacked Borrows (and similarly Tree Borrows) of course aims to make Rust's references usable for optimizations. It's also included in Miri, and since you test deno_core under Miri, you already ensure that you comply with Stacked Borrow's rules.

While we designed Tree Borrows to be less strict in many cases, there are unfortunately some cases where it's more strict than SB (mostly because before, SB had some "dirty hacks"). It turns out that deno_core relies on such a hack. You can see it yourself by running miri with MIRIFLAGS="-Zmiri-tree-borrows".

The precise issue is hiding here:

  fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
    // SAFETY: We know the underlying futures are both pinned by their allocations
    unsafe {
      match self.get_mut() {
        Self::Arena(a) => {
          Pin::new_unchecked(a.ptr.assume_init().as_mut()).poll(cx) // the `a.ptr`
        }
        Self::Box(b) => b.as_mut().poll(cx),
      }
    }
  }
}

The code a.ptr in the Self::Arena case uses the Deref trait implicitly, and this creates a shared reference to the entire DynFutureInfoErased, including the data field which is pinned somewhere else., even though you only ever use this to access the ptr field (as far as I can see). Unfortunately, this invalidates the (pinned) mutable reference to the second field (which stores the future's internal data), causing futures to randomly have UB later (when the code rustc desugared them to does a write to the implicit struct storing locals).

One solution (that seems to work, according to preliminary testing) would be to first change the DynFutureInfoErased to the following:

struct DynFutureInfoErased<T, C> {
  ptr: MaybeUninit<NonNull<dyn ContextFuture<T, C>>>,
  data: UnsafeCell<TypeErased<MAX_ARENA_FUTURE_SIZE>>, // the unsafecell is new
}

And to then ensure that ArenaBox never gives out mutable references (since UnsafeCell only does its magic on shared ones).

I'm in the process of developing such a change, and it is not too invasive so far. But before I waste too much time on it, I would like to hear whether you'd appreciate it. I am also not sure I fully understand the internals of deno_core, so perhaps you have some more opinions/guidance/ideas on what, if any, you want to have done here.

[bartlomieju]

Hey @JoJoDeveloping, thanks for reaching out. deno_core is a critical piece of Deno's stack and we're happy to accept any changes that make it more stable. Does the proposed change also work correctly with the current Stack Borrows approach? If it passes cleanly with Miri right now I'm happy to accept the change.

[JoJoDeveloping]

Does the proposed change also work correctly with the current Stack Borrows approach?

Yes.

I'll spend some more time pondering the best way of doing the change and propose a PR soon.