Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

preallocated_* promotes unsoundness #665

Open
Kixunil opened this issue Nov 10, 2023 · 3 comments
Open

preallocated_* promotes unsoundness #665

Kixunil opened this issue Nov 10, 2023 · 3 comments

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Nov 10, 2023

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.

@apoelstra
Copy link
Member

Great catch! And I like your solution (assuming it is possible to do all the conversions we want :))

@elichai
Copy link
Member

elichai commented Nov 10, 2023

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)

@Kixunil
Copy link
Collaborator Author

Kixunil commented Nov 10, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants