Plug soundness hold around mem::forgetting MoveRefs

[mcy]

The Pin drop invariant requires that pinned data be properly destroyed before its memory be re-used. This is a problem: MoveRef is responsible for running the destructor of storage it generally does not own. This means we must leak the storage of a forgotten MoveRef. This commit introduces explicit library-tracked drop flags to manage this corner case.

In the case of stack-pinned MoveRefs via slot, we can get into situations (such as if a constructor panics) where we cannot properly destroy the pointee. For now, we double-panic to force an abort, but we may be able to handle this better in the future.

[djkoloski]

Here's a relatively reduced counterexample (run in release):

use core::mem;
use moveit::{DerefMove, MoveRef, move_ref::DropFlag, slot};

struct Foo(u32);

impl Drop for Foo {
    fn drop(&mut self) {
        println!("Drop");
    }
}

fn oh_no() {
    slot!(slot: Foo);
    let bar = slot.put(Foo(1));
    println!("{:p}", &*bar);
    let mut deinit = DerefMove::deinit(bar);
    *deinit.ptrs().1 = DropFlag::Dead;
    let bar = unsafe {
        // SAFETY: this is called on the return value from `deinit` and is only called once
        MoveRef::<'_, Foo>::deref_move(&mut deinit)
    };
    let baz = MoveRef::into_pin(baz);
    mem::forget(baz);
}

fn main() {
    for _ in 0..10 {
        oh_no();
    }
}

Soundness: the same address for Foo will be used multiple times, but the destructor is never run and the memory for the variables is pinned.

To plug this soundness hole, you either need to escalate the DropFlag checks to asserts (so they run in release) or remove/modify the public ptrs() function to make manual drop flag manipulation impossible.

[mcy]

So I fundamentally think this code is wrong, because it's violating this invariant of MoveRef:

  /// `drop_flag`'s value *must* be [`DropFlag::Alive`], and must be a drop
  /// flag governing the destruction of `*ptr`.

I think the problem is that I would like for deinit to not be touchable between when deinit() is called and when deref_move() is called, but the language doesn't give us a way to avoid that. In fact, even this is unsound:

fn oh_no() {
    slot!(slot: Foo);
    let bar = slot.put(Foo(1));
    println!("{:p}", &*bar);
    let mut deinit = DerefMove::deinit(bar);
}

This will fail to call Foo's dtor, but free the storage. Oops! I think the observation here is that deinit needs to be unsafe and have the caveat that it cannot be called unless you can guarantee that you will call deref_move with no intermediate operations. These two operations should really be one function, but they can't be... so we wind up working around the language here.

---

That said, there may be another option. Fundamentally what we want out of DerefMove is to stick the storage somewhere. So maybe we need to redefine the trait to look like this:

unsafe trait DerefMove: DerefMut {
  type Uninit: Sized;
  fn deref_move<'frame>(self, storage: Slot<'frame, Self::Uninit>) -> MoveRef<'frame, Self::Target>;
}

We can then implement this thus for e.g. Box:

unsafe impl<T> DerefMove for Box<T> {
  type Uninit = ForgetIfForgotten<Box<MaybeUninit<T>>>;
  fn deref_move<'frame>(self, storage: Slot<'frame, Self::Uninit>) -> MoveRef<'frame, Self::Target> {
    // 1. Get the deinit'd version of `self` into `storage` without arming the forget() trap.
    // 2. Convert the FiF<Box> into a MoveRef as before.
  }
}

It's important to not arm the trap in storage, since we want to silently leak the data. Alternatively, we could not do that, and just always make forgetting a MoveRef be a crash... not sure what the best option is. I may need to sleep on this to design a clearer API.

[djkoloski]

The code is intentionally malicious, but it's following all the required safety documentation. It should be sufficient to make deinit unsafe and add conditions requiring a specific pattern of use. I think it may be harder for users to verify "no intermediate operations" than it sounds, but it's impossible to conjecture until there's something to poke at.

In the smaller example you gave, I don't actually think there's any unsoundness. I don't think there's any guarantee that objects will be dropped before their memory is freed except when those objects are pinned, and nothing gets pinned in that code.

---

I think the newer formulation of DerefMove seems more coherent and leaves fewer openings for soundness issues. Nice! I can poke at this some more if you switch the impls over.

[mcy]

@djkoloski I've implemented this in https://github.com/google/moveit/pull/25. Please LMK if you can poke holes in it. >:)

[mcy]

(#25 closes this)