Pinned Box

[cynecx]

Fixes #226.
Fixes #186.

This adds a new "pinned" Box type (pin::Box<T>), that guarantees running T's drop impl whenever the box is leaked and the Bump allocator is reset or dropped.

The impl utilizes a "intrusive circular doubly linked list" that links every "pinned" allocation that needs a cleanup. Each list entry detaches when the Box itself is dropped.
That way, when the Bump allocator is cleared, we can run the Drop impls by traversing the list.

The pinned Box is still one pointer-sized big. We can calculate the each allocation's header pointer by doing some pointer arithmetic. Note that this only passes miri with tree-borrows enabled (not compatible with stacked borrows and reports UB) due to the kind of container-of style pointer arithmetic being used.

As I mentioned above this has the implication that drop impls have to run when the bump allocator is cleared which goes against bumpaloo's current situation that resets are "extremely fast" and don't run any Drop impls.
So this change is a breaking change and needs discussion first. Whether or not we want to actually support this. One simple alternative would be to simply remove the support for pinned Boxes.

TODO

• Some more cleanups
• Right now the docs are basically copy and pasted and needs to be rewritten
• Lots of unsafe. Those need at least a "Safety" comment.
• More testing

[fitzgen]

I don't have time to dig in yet, but here are some initial thoughts:

• I don't mind losing the reset-is-just-a-pointer-update property because (a) that isn't quite true already when there are multiple chunks and (b) presumably if the user doesn't allocate any pinned boxes in the arena, then it doesn't impose any overhead other than a single compare and branch on the empty intrusive list, which is fine. If the implementation doesn't reflect (b) then we should investigate ways to make it so.

• I have vague plans for adding sub-arenas/checkpoints in the next breaking release of bumpalo (that said, nothing is concretely planned yet, and it might even never happen). This would take advantage of the LIFO nature of bump allocation to reset the bump pointer to a place in the middle of a chunk where the user made a checkpoint, keeping alive earlier allocations but freeing everything allocated since that checkpoint. To be compatible with this, the drop list would need to be stored in youngest-to-oldest allocation order so that we could keep dropping pinned boxes while they were past the checkpoint and stop as soon as we encounter one that is within the checkpoint. If the list was in the opposite order, resetting to a checkpoint would need to walk over every pinned box within the checkpoint before getting to the ones to drop that are outside the checkpoint, effectively making the operation O(total pin boxes) rather than O(pin boxes to drop). Is this ordering requirement problematic? I don't think it should be, but I also haven't looked into the details yet.

• It would be very good to have extensive quickcheck tests for this stuff.

• Note that this only passes miri with tree-borrows enabled (not compatible with stacked borrows and reports UB) due to the kind of container-of style pointer arithmetic being used.

  Is this fundamental to the intrusive list design or an implementation detail? All else being equal it would be nice to support both models, but it wouldn't be the end of the world to support only tree borrows. Would it help to hold a pointer to the header inside the box and then access the data as an offset of the header, rather than the other way around? I don't know if that would better propagate provenance or whatever.

[cynecx]

I don't mind losing the reset-is-just-a-pointer-update property

👍

presumably if the user doesn't allocate any pinned boxes in the arena, then it doesn't impose any overhead other...

Yes, that's how it's currently implemented. (Besides, the sentinel node is also currently lazily allocated and initialized.)

I have vague plans for adding sub-arenas/checkpoints in the next breaking release of bumpalo...

Exciting stuff!

Is this ordering requirement problematic? I don't think it should be, but I also haven't looked into the details yet.

Yeah, I don't think the ordering requirement is going to be a problem. The current impl uses a doubly linked list, so you could just iterate forwards or backwards regardless in what order you push the drop entries.

However, the way some APIs are currently implemented might pose an issue here:

Both pin::Box<T>::into_raw and pin::Box<T>::from_raw unlink/re-add the drop entry when called.
Therefore, invalidating the ordering invariant that we'd possibly want to preserve.
Perhaps the question is rather whether these APIs should've done that in the first place.
Or perhaps we should just keep the drop entry attached, and store a is_detached: bool in the header and toggle that when into_raw/from_raw is called.

Also, I am not exactly sure about the design you have in mind but I can also imagine one could maintain multiple drop lists for each sub-arena/checkpoint,
but that kind of depends on how it's actually implemented...

It would be very good to have extensive quickcheck tests for this stuff.

I've added some basic tests that test the API surface of the pinned Box. But we should probably add some tests around the internals (drop list) too.

Is this fundamental to the intrusive list design or an implementation detail?

It's more of an implementation detail (See below).

Would it help to hold a pointer to the header inside the box and then access the data as an offset of the header, rather than the other way around?

That won't work with SB because it's still doing "pointer arithmetic" (playground). IIUC, pointers can only be derived by actually "reborrowing" from a pointer,
aka, we'd have to repr the pinned Box like struct Box<T>(*const DropEntry<T>) and derive the data pointer with something like &(*self.0).data.
However, that representation is somewhat wonky because unsized coercion doesn't work anymore, which is quite unfortunate because we can't have a Box<dyn Future> anymore.

Although, I admit I'm not an expert in the SB/Tree-Borrows pointer aliasing models, so I might be totally wrong about this 😅.

[zetanumbers]

T must satisfy T: 'a (or be able to outlive 'a) so that drop within the bump would not be called on invalid value.

Suggested change

	impl<'a, T> Box<'a, T> {
	impl<'a, T> Box<'a, T>
	where
	T: 'a
	{

[zetanumbers]

Actually 'a is just an lifetime to an immutable reference of Bump, which can be subtyped into smaller lifetimes via covariance, so this is not good enough. Appropriate fix for would be to add a covariant lifetime argument 'b to Bump itself to restrict it and then restrict T: 'b.

[cynecx]

The marker PhantomData<&'a mut T> inside Box<'a, T> should produce the implied bound T: 'a, so your suggested change should be a noop.

Actually 'a is just an lifetime to an immutable reference of Bump, which can be subtyped into smaller lifetimes via covariance, so this is not good enough.

I don't see how this is unsound. Even if you'd have &'short Bump, T must outlive 'short (and 'long: 'short). The returned ref to T will be borrowed for the shorter lifetime and can't live longer than that.

It would be great if you could provide an example where this unsoundness is demonstrated :)

[zetanumbers]

You drop T: 'short object within the Bump: 'long's drop. Just as i've said it in the first comment.

[cynecx]

Ah I see the issue now. Yeh, it's actually very easy to trigger a UAF with something like

#[test]
fn pinned_failure() {
    struct Foo<'a>(&'a String);

    impl<'a> Drop for Foo<'a> {
        fn drop(&mut self) {
            println!("{}", self.0);
        }
    }

    let bump = Bump::new();

    let s = "hello".to_string();
    let p = Box::new_in(Foo(&s), &bump);
    mem::forget(p);
    drop(s);

    drop(bump);
}

[cynecx]

I've added a T: 'static bound (for now) which should hopefully make this more sound. It is obviously more restrictive now but it doesn't require more "invasive" api changes to Bump. Haven't really thought much about this, but I'll have a more deeper look into this in the upcoming days. Also thank you @zetanumbers for noticing this :)

[zachs18]

Instead of being removed entirely, it would also be sound to restrict it to 'static, i.e. add a separate impl block for Box<'static, T> with

impl<T: ?Sized> Box<'static, T> {
    /// Constructs a new `Pin<Box<'static, T>>`. If `T` does not implement `Unpin`, then
    /// `x` will be pinned in memory and unable to be moved. This requires borrowing
    /// the `Bump` for `'static` to ensure that it can never be reset or dropped.
    #[inline(always)]
    pub fn pin_in(x: T, a: &'static Bump) -> Pin<Box<'static, T>> {
        Box(a.alloc(x)).into()
    }
}

[zachs18]

Likewise, this impl could just be restricted to 'static.

impl<T: ?Sized> From<Box<'static, T>> for Pin<Box<'static, T>> {
    /// Converts a `Box<'static, T>` into a `Pin<Box<'static, T>>`.
    ///
    /// This conversion does not allocate and happens in place. This requires borrowing
    /// the `Bump` for `'static` to ensure that it can never be reset or dropped.
    fn from(boxed: Box<'static, T>) -> Self {
        // It's not possible to move or replace the insides of a `Pin<Box<T>>`
        // when `T: !Unpin`, and because the lifetime the `Bump` is borrowed
        // for is `'static`, the memory can never be re-used, so it's safe to pin
        // it directly without any additional requirements.
        unsafe { Pin::new_unchecked(boxed) }
    }
}