Relax A: 'static bound for boxed Pin APIs

[djkoloski]

In #79327, the Pin APIs for Box were restricted to A: 'static. This was overly cautious and restricts the Pin APIs unnecessarily. The justification for doing this was as follows:

Allocators has to retain their validity until the instance and all of its clones are dropped. When pinning a value, it must live forever, thus, the allocator requires a 'static lifetime for pinning a value. Example from reddit:

let alloc = MyAlloc(/* ... */);
let pinned = Box::pin_in(42, alloc);
mem::forget(pinned); // Now `value` must live forever
// Otherwise `Pin`'s invariants are violated, storage invalidated
// before Drop was called.
// borrow of `memory` can end here, there is no value keeping it.
drop(alloc); // Oh, value doesn't live forever.

In the comment thread, @TimDiekmann correctly observed that the given code produces a compiler error:

error[E0503]: cannot use `memory` because it was mutably borrowed
    |
    | let region = Region::new(&mut memory[..]);
    |              ----------------------------
    |              |                |
    |              |                borrow of `memory` occurs here
    |              argument requires that `memory` is borrowed for `'static`
...
    | drop(memory);
    |      ^^^^^^ use of borrowed `memory`

This is a hint that the reasoning may not be correct. Forgetting the box will also forget the allocator, which will prevent the memory from being freed and therefore reused. If you move a clone of alloc into the box, you're still covered by the safety conditions for Allocator:

• Memory blocks returned from an allocator must point to valid memory and retain their validity until the instance and all of its clones are dropped,

At least one clone of the allocator must be forgotten along with the pinned box. This is also why Box::leak is sound with a restriction of A: 'a; the allocator is not dropped when the box is leaked.

It is important to note that naive implementations of non-'static allocators cannot fulfill the safety requirements for Allocator. The allocator can be forgotten and its allocated memory will still be released at the end of its lifetime, which may cause the memory of undropped pinned values to be reused. The unsoundness is actually in the unsafe Allocator impl though, and not the boxed Pin APIs.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (or someone else) soon.

Please see the contribution instructions for more information.

[TimDiekmann]

cc @rust-lang/wg-allocators

[SNCPlay42]

At least one clone of the allocator must be forgotten along with the pinned box.

Is this true, given that there is a impl<A> Allocator for &'_ A?

[djkoloski]

It would suggest that the blanket impl for &'_ A is in conflict with the safety documentation, as forgetting a reference would not prevent memory managed by the underlying allocator from being freed. Could I get some clarification on what exactly an "instance" is? Having &'_ A implement Allocator makes that unclear.

[Wodann]

Can you add the "broken case" as a test? That way we're guaranteed that it'll keep working - given future changes.

[djkoloski]

I would be happy to add some tests that exercise these APIs. First, I think there's a need for some clarification on the safety requirements of the Allocator API, especially with regards to dropping and the duration of memory validity.

[bjorn3]

What about something like:

let mut an_allocator = Allocator::new();
an_allocator.with_sub_allocator(|sub_allocator| {
    let pinned = Box::pin_in(42, sub_allocator);
    mem::forget(pinned);
});

where the closure has the type for<'a> impl FnOnce(SubAllocator<'a>) and with_sub_allocator automatically frees all not yet freed memory even if the SubAllocator is leaked? The for<'a> ensures that SubAllocator can't escape the with_sub_allocator call, so in the presence of the A: 'static bound for boxed Pin APIs it would be sound, but in the absence one of the Pin invariants can now be violated.

Edit: Never mind. That violates a safety condition of Allocator as you already pointed out.

[djkoloski]

In your example, the suballocator in pinned is not dropped but the memory it allocated is still freed which violates the safety requirements for Allocator:

• Memory blocks returned from an allocator must point to valid memory and retain their validity until the instance and all of its clones are dropped,

Such a sub-allocator is not sound under these requirements.

[talchas]

• Memory blocks returned from an allocator must point to valid memory and retain their validity until the instance and all of its clones are dropped,

Such a sub-allocator is not sound under these requirements.

TBH this requirement (the explicit "dropped" requirement ala Pin) seems sketchy to have on all Allocators as opposed to an additional requirement for Pin. It basically means that you need a refcount on the allocator or equivalently flags for individual allocations, and for stack allocators you have to abort if any allocation has been forgotten.

[bors]

☔ The latest upstream changes (presumably #92556) made this pull request unmergeable. Please resolve the merge conflicts.

[yaahc]

Hey, I've begun reviewing this PR and as part of that I'm trying to familiarize myself with the Allocator trait and associated APIs as well as the various concerns in this issue. Right now I'm very nervous to make this change and would like to make sure I understand all of the implications of this relaxation and that we've resolved all the concerns mentioned in this PR so far. There's a lot to think about at once so I'm going to start by outlining the current unresolved questions:

• what usage of Box::pin_in does this change enable that is currently disallowed previously, or more generally, why do you need this change?
• Relax A: 'static bound for boxed Pin APIs #90822 (comment) How would adopting this change affect the validity of impl<A> Allocator for &'_ A
• How do we uphold Pin's drop guarantee if we've adopted this change?
• Relax A: 'static bound for boxed Pin APIs #90822 (comment): the relationship between Pin and Allocator and the responsibilities of each
• Does this change prevent or enable stack local allocators? I need to read more of the historical discussions around the Allocator but intuitively this seems like a pretty important use case to support and is probably the most relevant usecase to this change.
  • Is it at all possible to use stack local allocators and Pin together? My guess is no, or that it would only work with "abort on leak" when freeing the stack memory.

---

The first question is my own and is probably the best starting point.

[djkoloski]

Thanks for taking a look. The Allocator trait in particular is a very thorny API and I appreciate your willingness to stick through it. I'll do my best to directly answer your questions and provide some motivating examples. I apologize in advance for any long-windedness as this issue has proven particularly difficult to communicate clearly.

What usage of Box::pin_in does this change enable that is currently disallowed previously, or more generally, why do you need this change?

I was experimenting with pinned stack allocations, specifically some Box<T, A> where A is a stack allocator and cannot fulfill A: 'static. This change would also enable allocators that are not 'static, for example allocators that allocate on the heap but have a finite lifetime.

How would adopting this change affect the validity of impl<A> Allocator for &'_ A?

This change does not affect the validity of impl<A> Allocator for &'_ A. I believe that there is already a soundness issue with that impl that this change would make easier to cause. In explicit terms, relaxing the A: 'static bound is sound and the soundness hole with that impl is technically a separate issue. I would be happy to plug that soundness hole in this PR if desired though.

Why impl<A> Allocator for &'_ A is unsound

The safety documentation for the Allocator trait states:

• Memory blocks returned from an allocator must point to valid memory and retain their validity until the instance and all of its clones are dropped,

This documentation has some vagueness in its usage of "the instance and all of its clones". I'll work under the assumption that this means any instance of an allocator type or .clone() of that instance.

A traditional heap allocator like the system allocator, jemalloc, or TLSF tracks free and allocated segments of memory using a control structure. This control structure may implement Allocator, but it's not very useful if it's exclusively owned by a single object. It's more common to put it behind an owning pointer like Rc<Control> so that multiple objects can allocate with it at the same time.

It's also possible to access the control structure through a &Control, but we must make sure to uphold the safety documentation. Specifically, we must ensure that any memory allocated stays valid until all &Controls are dropped. This is a crucial difference between owning pointers and shared references: owning pointers guarantee that the pointee will not be dropped if any instances are forgotten. On the other hand, it is possible to forget a shared reference and still drop the pointee later on. This violates the allocated memory safety condition of Allocator. The only shared reference that this does not occur with is a 'static reference, as it is never dropped.

It is for this reason that I believe that the blanket impl<'_, A> Allocator for &'_ A where A: Allocator + ?Sized is unsound. &'_ A cannot meet the required safety conditions of the Allocator trait. Instead, this blanket impl should be impl<A> Allocator for &'static A where A: Allocator + ?Sized since static shared references can uphold the safety conditions of Allocator.

Here is a playground link demonstrating the unsoundness of the impl. Running yields:

value pinned at 0x56434b2da9e0
value pinned at 0x56434b2da9e0
value pinned at 0x56434b2da9e0
value pinned at 0x56434b2da9e0
value pinned at 0x56434b2da9e0
value pinned at 0x56434b2da9e0
value pinned at 0x56434b2da9e0
value pinned at 0x56434b2da9e0
value pinned at 0x56434b2da9e0
value pinned at 0x56434b2da9e0

Note that changing alloc_and_forget(&simple_allocator) to alloc_and_forget(simple_allocator) or alloc_and_forget(simple_allocator.clone()) corrects this behavior.

How do we uphold Pin's drop guarantee if we've adopted this change?

Pin's drop guarantee states that pinned memory "will not get invalidated or repurposed from the moment it gets pinned until when drop is called". When handling a Pin<P> where P is some owning pointer (e.g. Box, Vec, Arc, etc.), it must be safe to forget that Pin<P> as explained by forget's documentation:

forget is not marked as unsafe, because Rust's safety guarantees do not include a guarantee that destructors will always run.

And it is safe because forgetting a Pin<P> will also forget any allocators used by P. For Box<T, A> as an example, forgetting a Pin<Box<T, A>> will forget the A allocator and prevent its drop from being called. Because the allocator is never dropped, the memory allocated by it and any of its clones must retain their validity indefinitely per Allocator's safety docs. This will prevent any pinned values from ever being dropped, which upholds Pin's Drop guarantee.

The relationship between Pin and Allocator and the responsibilities of each

Pin is only responsible for upholding its drop guarantee. Allocator's safety requirements make it pin-safe. A result of the interaction between Pin and Allocator is that memory allocated by an Allocator must only be pinned with the allocator itself. It is not sound to pin allocated memory that is not alongside the allocator it belongs to. This is not a new safety requirement, just a consequence of the existing safety requirements of the two types.

Does this change prevent or enable stack local allocators?

This change enables stack local allocators with Box. To uphold the safety requirements of Allocator, stack allocators must panic abort if they have any outstanding allocations when dropped.

Is it at all possible to use stack local allocators and Pin together?

Yes, but it requires the aforementioned abort-on-leak behavior.

[yaahc]

Regarding the impl Allocator for &'_ A impl soundness issue, lets go ahead and open a separate PR to fix that. That seems reasonably straight forward and independent so we should be able to merge that quickly, though I will want wg-allocators to sign off on the change as well.

---

• Memory blocks returned from an allocator must point to valid memory and retain their validity until the instance and all of its clones are dropped,

Such a sub-allocator is not sound under these requirements.

TBH this requirement (the explicit "dropped" requirement ala Pin) seems sketchy to have on all Allocators as opposed to an additional requirement for Pin. It basically means that you need a refcount on the allocator or equivalently flags for individual allocations, and for stack allocators you have to abort if any allocation has been forgotten.

Regarding the pin drop requirements by either using reference counting or by abort-on-leak, @talchas you seem to be dissatisfied with those as available options. Do you have an example usecase for Allocator where neither of these strategies would be acceptable?

Also, its unclear to me on if you're talking about the allocator requirement that memory remains valid until all allocators are dropped vs the pin requirement that memory remain valid until drop is called. Are these in-fact the same requirement? I'm wondering if this requirement got added to allocator because of Pin or if there were separate reasons why allocator would need this invariant to be upheld.

I'm trying to understand what it would look like to remove this requirement from allocator and my best guess is that it would disallow pinning any data from an allocator that isn't 'static, so more or less completely disallowing this change. Does that sound accurate?

[talchas]

Also, its unclear to me on if you're talking about the allocator requirement that memory remains valid until all allocators are dropped vs the pin requirement that memory remain valid until drop is called. Are these in-fact the same requirement? I'm wondering if this requirement got added to allocator because of Pin or if there were separate reasons why allocator would need this invariant to be upheld.

Good question, I don't know the answer :P

I'm trying to understand what it would look like to remove this requirement from allocator and my best guess is that it would disallow pinning any data from an allocator that isn't 'static, so more or less completely disallowing this change. Does that sound accurate?

It would have to be unsafe or involve another trait, yes. Fundamentally refcounting to amplify leaks feels to me like it should be the user's responsibility, not the Allocator's. The cpu/memory cost isn't exactly high though.

[Aaron1011]

To uphold the safety requirements of Allocator, stack allocators must panic if they have any outstanding allocations when dropped.

Is panicking sufficient? What happens if the panic gets caught?

[djkoloski]

Ah, no that is not sufficient and stack allocators must abort in that situation. I've corrected the referenced section.

[CAD97]

Here's one argument against.

With the even more experimental storages API, Box uses a SingleElementStorage.

This is done in such a way that MaybeUninit<T> [can be SingleElementStorage]; alloc just returns a pointer¹ to the MaybeUninit, and free is a noöp.

Importantly, there's no extra state involved, so Box<T, InlineStorage<T>> is equivalent to T. Unfortunately... this means InlineStorage is both 'static and can't fulfil the Pin requirements.

So if we're going to use storages (cc @TimDiekmann again), we'll need a "pinning compatible" bound for storages/allocators anyway. It's probably better to spell this out separately, anyway, rather than having it implicit in Alloc + 'static.

So put me down as a +1 to replacing the pinning guarantee on Alloc with a separate PinAlloc unsafe marker extension trait and replacing the 'static bound with that. (Then a region allocator just doesn't impl PinAlloc.)

Footnotes

1. Modulo details; the full storage API, to handle such inline storage, actually works over _handle_s. Handles remain valid as the storage moves, and you ask the storage to resolve the handle into a concrete pointer which is valid until the storage is moved². ↩

2. For single element storages at least; for multi element storages, the pointer is valid until the a new handle is allocated and the storage reallocates. ↩

[djkoloski]

So put me down as a +1 to replacing the pinning guarantee on Alloc with a separate PinAlloc unsafe marker extension trait and replacing the 'static bound with that. (Then a region allocator just doesn't impl PinAlloc.)

As long as Allocator returns a pointer to a memory block (as opposed to a Handle or something equivalent), I don't think it makes sense to relax the pointer stability guarantee on Allocator. Boxed allocators and pinned boxes (that is, Pin<&mut Box<T, A>> not Pin<Box<T, A>>) don't seem like they have a use case. Everything would just end up trading Allocator for PinAllocator and nobody would use Allocator.

[bors]

☔ The latest upstream changes (presumably #94901) made this pull request unmergeable. Please resolve the merge conflicts.

[djkoloski]

Abandoning in favor of #94114