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

macos: Cleanup SecRandom type #28

Merged
merged 2 commits into from Jun 12, 2019
Merged

macos: Cleanup SecRandom type #28

merged 2 commits into from Jun 12, 2019

Conversation

josephlr
Copy link
Member

Right now SecRandom is an uninhabited type. However, opaque pointers
are not supposed to be pointers to uninhabited types or ZSTs. This
issue was solved in core by introducing core::ffi:c_void that just
does all the right things.

This PR:

  • Changes SecRandom to be a transparent wrapper around c_void.
  • Links kSecRandomDefault instead of defining it ourself.
  • Should not change any behavior on macos.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of this. I found the section on Handling Zero-Sized Types in the nomicon, but it only talks about issues with allocation and iterators — we don't appear to do either here.

What our version of OpaquePointer is missing is alignment info, though given that we only ever construct the address 0 it shouldn't matter.

Also, I guess usize is always the same as libc::size_t but can we actually rely on that?

@newpavlov
Copy link
Member

IIUC this is one of the use-cases for extern types (currently unstable) and uninhabited types is a common workaround, which I've seen in several places. I am not sure if this PR is correct, citing the c_void docs:

To model pointers to opaque types in FFI, until extern type is stabilized, it is recommended to use a newtype wrapper around an empty byte array.

Right now `SecRandom` is an uninhabited type. However, opaque pointers
are not supposed to be pointers to uninhabited types or ZSTs. This
issue was solved in `core` by introducing `core::ffi:c_void` that just
does all the right things.

This PR:

  - Changes `SecRandom` to be a `transparent` wrapper around `c_void`.
  - Links `kSecRandomDefault` instead of defining it ourself.
  - Should not change any behavior on macos.
src/macos.rs Outdated
@@ -11,29 +11,23 @@ extern crate std;

use crate::Error;
use std::io;
use core::ffi::c_void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this import anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@josephlr
Copy link
Member Author

I wasn't aware of this. I found the section on Handling Zero-Sized Types in the nomicon, but it only talks about issues with allocation and iterators — we don't appear to do either here.

What our version of OpaquePointer is missing is alignment info, though given that we only ever construct the address 0 it shouldn't matter.

IIUC this is one of the use-cases for extern types (currently unstable) and uninhabited types is a common workaround, which I've seen in several places. I am not sure if this PR is correct, citing the c_void docs:

Whoops, missed that. Fixed to do what the nomicon says (use a private ZST field) and link to the Github issue on extern_type.

Also, I guess usize is always the same as libc::size_t but can we actually rely on that?

So this assumption is built deep into how Rust interfaces with C libraries. If a platform is supportted by libc, size_t is a typedef for usize. This is assumption is also built into things like memcpy. So I'd say it is about as safe a relying on u8, u16, u32, u64 being the same as libc::{uint8_t, uint16_t, uint32_t, uint64_t}

@newpavlov newpavlov merged commit bcad3fb into rust-random:master Jun 12, 2019
@josephlr josephlr deleted the mac branch June 12, 2019 11:48
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

Successfully merging this pull request may close these issues.

None yet

3 participants