VecStorage and DenseVecStorage are unsound

[leudz]

Hi all!

DenseVecStorage

The problem is located in this function. Having uninitialized data inside a slice is insta-UB, in this function data_id has uninitialized u32 thanks to the contract of set_len not being respected. Then get_unchecked_mut will create a slice and trigger UB. This slice is created in every function accessing data inside the storage too. This could be fixed by replacing uninitialized value by 0 or std::u32::MAX for example (it's never accessed anyway).

VecStorage

The problem is in insert too and spread to any function accessing data in the storage. This is basically the same thing, a call to set_len not respecting the contract followed by a call to get_unchecked_mut but this time it's a Vec<T>. I think the solution would be VecStorage<T>(Vec<MaybeUninit<T>>). An other way would be to index inside the Vec using raw pointer only but that's dangerous since any "classic" access would still trigger UB.

Also creating a reference to uninitialized data is not currently ok but might change in the future for some types (like u32) so it might be a good thing to replace them with raw pointer arithmetic.

[leudz]

Never mind, now I find the PR... I'll move this there. I was a bit too quick to close this, part of the PR is based on a wrong assumption that a slice can contain uninitialized elements as long as they are never accessed.

[willglynn]

This issue concerns the UB-ness of slices which contain uninitialized values. I have previously argued that this was okay, based on things like alloc::raw_vec::RawVec::into_box() docs:

pub unsafe fn into_box(self) -> Box<[T]>

Converts the entire buffer into Box<[T]>.

While it is not strictly Undefined Behavior to call this procedure while some of the RawVec is uninitialized, it certainly makes it trivial to trigger it.

Note that this will correctly reconstitute any cap changes that may have been performed. (see description of type for details)

MaybeUninit replaces this guidance. The docs above changed due to its stabilization and now read:

pub unsafe fn into_box(self) -> Box<[T]>

Converts the entire buffer into Box<[T]>.

Note that this will correctly reconstitute any cap changes that may have been performed. (see description of type for details)

Undefined Behavior

All elements of RawVec<T, Global> must be initialized. Notice that the rules around uninitialized boxed values are not finalized yet, but until they are, it is advisable to avoid them.

I concur that slices which contain uninitialized elements are indeed unsound in current Rust. #634 contains a patch to that effect. I can move it to a new PR if you'd like to consider it separately.

[OvermindDL1]

There was a set of issues that popped up in some rust programs that showed that having an invalid pointer ever to exist, ever, means that LLVM can start compiling in certain assumptions, specifically making all following code that depends on the unsound code to do absolutely crazy things. This is the reason that uninit and zero are to be deprecated and they are replaced with MaybeUninit. MaybeUninit just prevents an invalid pointer from existing in a way that it can be accessed without questionably unsafe code, thus making sure the compiled code is valid.

[Avi-D-coder]

Has anyone run specs under Miri? It can aid in discovering UB.

[WaDelma]

yes: https://github.com/slide-rs/specs/issues/644

[a1phyr]

I think this can be close thanks to #634