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

Use getentropy when possible on all Apple platforms #101011

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

BlackHoleFox
Copy link
Contributor

@BlackHoleFox BlackHoleFox commented Aug 25, 2022

As the current code comments say, SecRandomCopyBytes is very heavyweight (regardless of purpose) compared to just asking the kernel directly for bytes from its own CSPRNG. We were not previously making an attempt to use the more efficient getentropy call on other Apple targets, instead solely using it on macOS. As the function is available on newer versions of Apple's different OSes, this changes the random filling to always attempt it first everywhere, only falling back to the less ideal alternatives after. This also cleans up the multiple Apple imp blocks into one.

It also should give a perf improvement, even if its likely unnoticeably small.

Refed XCode header for getentropy in the SDK:

int getentropy(void* buffer, size_t size) __OSX_AVAILABLE(10.12) __IOS_AVAILABLE(10.0) __TVOS_AVAILABLE(10.0) __WATCHOS_AVAILABLE(3.0);

r? @thomcc

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 25, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2022
#[cfg(target_os = "watchos")]
#[cold]
fn fallback_fill_bytes(_: &mut [u8]) {
unreachable!()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a behavioral change, but not a breaking one afaict due to watchOS version support.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's not a behavioral change -- IIUC we don't support those versions of watchOS and tell the linker as much. Also, watchOS doesn't really work at the moment until rust-lang/libs-team#75 is resolved.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll r+ after CI comes back green.

#[cfg(target_os = "watchos")]
#[cold]
fn fallback_fill_bytes(_: &mut [u8]) {
unreachable!()
Copy link
Member

Choose a reason for hiding this comment

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

I think it's not a behavioral change -- IIUC we don't support those versions of watchOS and tell the linker as much. Also, watchOS doesn't really work at the moment until rust-lang/libs-team#75 is resolved.

@thomcc
Copy link
Member

thomcc commented Aug 25, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Aug 25, 2022

📌 Commit 3fc35b5 has been approved by thomcc

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 25, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 31, 2022
…ents, r=thomcc

Use getentropy when possible on all Apple platforms

As the current code comments say, `SecRandomCopyBytes` is very heavyweight (regardless of purpose) compared to just asking the kernel directly for bytes from its own CSPRNG. We were not previously making an attempt to use the more efficient `getentropy` call on other Apple targets, instead solely using it on macOS. As the function is available on newer versions of Apple's different OSes, this changes the random filling to always attempt it first everywhere, only falling back to the less ideal alternatives after. This also cleans up the multiple Apple `imp` blocks into one.

It also should give a perf improvement, even if its likely unnoticeably small.

Refed XCode header for `getentropy` in the SDK:
```h
int getentropy(void* buffer, size_t size) __OSX_AVAILABLE(10.12) __IOS_AVAILABLE(10.0) __TVOS_AVAILABLE(10.0) __WATCHOS_AVAILABLE(3.0);
```

r? `@thomcc`
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2022
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#100970 (Allow deriving multipart suggestions)
 - rust-lang#100984 (Reinstate preloading of some dll imports)
 - rust-lang#101011 (Use getentropy when possible on all Apple platforms)
 - rust-lang#101025 (Add tier-3 support for powerpc64 and riscv64 openbsd)
 - rust-lang#101049 (Remove span fatal from ast lowering)
 - rust-lang#101100 (Make call suggestions more general and more accurate)
 - rust-lang#101171 (Fix UB from misalignment and provenance widening in `std::sys::windows`)
 - rust-lang#101185 (Tweak `WellFormedLoc`s a bit)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1484742 into rust-lang:master Aug 31, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 31, 2022
@BlackHoleFox BlackHoleFox deleted the apple-random-improvements branch August 31, 2022 23:59
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 24, 2023
…omcc

Remove Apple RNG fallbacks and simplify implementation

Now that we have [higher Apple platform requirements](rust-lang#104385), the RNG code can be simplified a lot. Since `getentropy` still doesn't look to be usable outside macOS this implementation:
- Removes any macOS fallback paths and unconditionally links to `getentropy`
- Minimizes the implementation for everything else (iOS, watchOS, etc).

`CCRandomGenerateBytes` was added in iOS 8 which means that we can use it now. It and `SecRandomCopyBytes` have the exact same functionality, but the former has a simpler API and no longer requires libstd to link to `Security.framework` for one function. Its also available in all the other target's SDKs.

Why care about `getentropy` then though on macOS? Well, its still much more performant. Benchmarking shows it runs at ~2x the speed of `CCRandomGenerateBytes`, which makes sense since it directly pulls from the kernel vs going through its own generator etc.

Semi-related to a previous, but reverted, attempt at improving this logic in rust-lang#101011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants