rust-bitcoin/rust-secp256k1

`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.