`preallocated_*` promotes unsoundness
Kixunil opened this issue · 3 comments
These methods take &mut [AlignedType]
which must be initialized because it does not contain MaybeUninit
. The API may entice people to write this code though:
unsafe {
let buffer = allocator.alloc(size, alignment); // returns uninitialized data
let buffer = core::slice::from_raw_parts(buffer, size);
Secp256k1::preallocated_new(buffer)
}
This is unsound and it's not obvious. (The whole maybe uninit business is subtle and not widely understood by people.)
I was thinking about making it accept &mut [MaybeUninit<AlignedType>]
but that may be annoying for consumers and promotes casting &mut T
to &mut MaybeUninit<T>
which, while not unsound itself, needs to be treated carefully. So the only sane option that I can think of is defining AlignedType
as struct AlignedType(MaybeUninit<[u8, 16]>)
, providing conversions from pointers/MaybeUninit<[AlignedType]>
and documenting that it's fine to have it uninitialized.
Great catch! And I like your solution (assuming it is possible to do all the conversions we want :))
You're right :)
I think either MaybeUninit was still out of our MSRV back then, or I mostly thought of people calling vec![AlignedType::ZERORED,len];
instead of manually allocating, but MaybeUninit more accurately describes a generic memory stream (as C might write padding bytes into it)
Yeah, I remember that MSRV at some point didn't include MaybeUninit
. It just occurred to me that going forward we should try our best to resolve things like this by #[cfg(rust_*)]
, if it ever happens again, since it could affect soundness.
Now I realized that maybe we should take PreallocatedBufferMut<'a>
which will just wrap the reference and later we could change it to type PreallocatedBufferMut<'a> = &'a mut PreallocatedBuffer
where PreallocatedBuffer
is an extern type.