Clarify unsafe requirements of AllocRef

[HeroicKatora]

The Safety section currently uses a number of non-standard terms and is rather small. The biggest pain point might be:

• 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 leaves a number of important requirements open. The term 'valid memory' isn't used anywhere else in the standard library documentation. It isn't even decided what the validity of a [u8] requires (see ucg-71). The wording doesn't make it clear that the caller must be allowed to write any contents to it, including uninitialized memory. It should also be mentioned that no other reference pointing to the memory region must be used while the block is allocated. Both of these requirements are already necessary to implement Box in terms of an AllocRef impl.

In comparison, MaybeUninit is arguably simpler and safer to use and has a far more extensive documentation.

[petertodd]

The Safety section currently uses a number of non-standard terms and is rather small. The biggest pain point might be: > * 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 leaves a number of important requirements open. The term 'valid memory' isn't used anywhere else in the standard library documentation. It isn't even decided what the validity of a `[u8]` requires (see [ucg-71](rust-lang/unsafe-code-guidelines#71)). The wording doesn't make it clear that the caller must be allowed to write _any_ contents to it, including uninitialized memory. It should also be mentioned that no other reference pointing to the memory region must be used while the block is allocated. Both of these requirements are already necessary to implement `Box` in terms of an `AllocRef` impl. In comparison, `MaybeUninit` is arguably simpler and _safer_ to use and has [a far more extensive documentation](https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html).

Note how if `AllocRef::alloc` returned a `&'static mut [MaybeUninit<u8>]`, it could remain a safe function, and it would be safe to do anything with the slice that you would be allowed to do with any other such slice. That might actually help clarify some of the requirements as to what exactly 'valid memory' constitutes by making clear it's uninitialized and mutable (thus sole ownership). `MaybeUninit` has a `as_mut_ptr()` method, so the ergonomics aren't that much different than what #61 suggests. (this has probably come up before, but you could also argue there should be the potential for a lifetime other than 'static in there) (and my apologies if the `MaybeUninit` possibility has been argued already! I may have missed that)

[SimonSapin]

Returning &'static mut is unsound since the reference becomes invalid after dealloc is called. The actual life time of heap allocations cannot be encoded statically in the borrow checker.

I think MaybeUninit is worth considering, and perhaps worth a dedicated thread (since this issue is to some extent about documentation).

[petertodd]

Returning `&'static mut` is unsound since the reference becomes invalid after `dealloc` is called. The actual life time of heap allocations cannot be encoded statically in the borrow checker.

Ah, yeah, I was thinking that's no worse than the existing problem of references being created from pointers returned by alloc. But thinking about it more I see how it's a footgun.

I think `MaybeUninit` is worth considering, and perhaps worth a dedicated thread (since this issue is to some extent about documentation).

+1

[Lokathor]

point of order: it's not unsound it's just a footgun.

Because it's not wrong: the memory is static. It would be up to the dealloc caller to prove that all references had ended. Since dealloc is unsafe, soundness is preserved.

[SimonSapin]

That’s not my understanding of what 'static means, but I don’t know what the exact formal definition is or whether we have one.

[Lokathor]

Static means "lives until the end of the program", but it doesn't have to be a compile time value. That's how Box::leak works.

[sollyucko]

1. If dealloc where to accept mutable AKA unique reference, the caller shouldn't be able to continue using the reference, right?
2. Another possibility is to have something implementing Drop, so that dealloc is also safe (see Crazy idea: Raw(Box/Vec/Deque)Impl<T> traits #63).

[TimDiekmann]

Another possibility is to have something implementing Drop, so that dealloc is also safe (see #63).

This will not work, as this is not the issue, why dealloc is unsafe. Leaking memory is not unsafe, but the caller have to ensure, that the provided pointer and layout are valid. We can't do much here to reduce the unsafeness, as allocators are a very low level API. Only a few user will ever need to use this API directly.

[TimDiekmann]

and my apologies if the MaybeUninit possibility has been argued already! I may have missed that

I think I have proposed it a while ago, but I can neither remember where nor can find it. I opened #66 for this. Possibly I also tried it out in one of my dozend branches at the alloc-wg crate 😄

[HeroicKatora]

Returning &'static _ would be a footgun for the use and implementation non-global allocators. The current requirement is for memory to be valid 'until the instance and all of its clones are dropped' which may in fact happen sooner than 'static. Since this inherently is not a scoped requirement but instead memory belongs to some owned AllocRef instance that is not borrowed from I don't think having a lifetime in the returned value is a good clarification. (It would nevertheless be sound, lifetimes have no semantic effect and the unsafe trait could require the caller to know).

[zakarumych]

Box::leak returns reference with arbitrary lifetime and it is safe to convert it back, provided that no other references to the value left.
So &'arbitrary mut [MaybeUninit<u8>] communicate guarantees better than NonNull<u8>.
However I must agree that code receiving this reference can be confused by lifetime. i.e. footgun.