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.

Sign up for GitHub

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

Jump to bottom

Add a comment explaining why SecRandomCopyBytes is not used on MacOS #59879

Merged
merged 1 commit into from
Apr 16, 2019
Merged

Add a comment explaining why SecRandomCopyBytes is not used on MacOS #59879

merged 1 commit into from
Apr 16, 2019

Conversation

ebarnard
Copy link
Contributor

SecRandomCopyBytes is available since MacOS 10.7 which is the minimum supported version and which was suggested in #58901 (comment) is the earliest version currently in use.

This matches the behaviour of other platforms which have a random number generator syscall available.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2019
@sfackler
Copy link
Member

Are we already linking libstd to Security.framework or are we going to need to handle that too?

@ebarnard
Copy link
Contributor Author

Good catch, no. We now are and this time I've tested locally.

@sfackler
Copy link
Member

r? @alexcrichton

SGTM but just to make sure our version support's consistent.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 11, 2019

📌 Commit 22f2afe has been approved by alexcrichton

@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 Apr 11, 2019
Centril added a commit to Centril/rust that referenced this pull request Apr 12, 2019
@Centril
Rollup merge of rust-lang#59879 - ebarnard:patch-1, r=alexcrichton
020ed28 
Use SecRandomCopyBytes instead of /dev/urandom on MacOS

SecRandomCopyBytes is [available since MacOS 10.7](https://developer.apple.com/documentation/security/1399291-secrandomcopybytes?language=objc) which is the minimum supported version and which was suggested in rust-lang#58901 (comment) is the earliest version currently in use.

This matches the behaviour of other platforms which have a random number generator syscall available.
@Centril Centril mentioned this pull request Apr 12, 2019
Closed
@Aaron1011 Aaron1011 mentioned this pull request Apr 13, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Apr 13, 2019
@Centril
Rollup merge of rust-lang#59879 - ebarnard:patch-1, r=alexcrichton
4f34501 
Use SecRandomCopyBytes instead of /dev/urandom on MacOS

SecRandomCopyBytes is [available since MacOS 10.7](https://developer.apple.com/documentation/security/1399291-secrandomcopybytes?language=objc) which is the minimum supported version and which was suggested in rust-lang#58901 (comment) is the earliest version currently in use.

This matches the behaviour of other platforms which have a random number generator syscall available.
@Centril Centril mentioned this pull request Apr 13, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Apr 13, 2019
@Centril
Rollup merge of rust-lang#59937 - Centril:rollup-6pousju, r=Centril
30a61c9 
Rollup of 18 pull requests

Successful merges:

 - rust-lang#59612 (Use normal newtype_index macro for MIR dataflows)
 - rust-lang#59675 (Stabilize the `alloc` crate.)
 - rust-lang#59708 (Mark variables captured by reference as mutable correctly)
 - rust-lang#59735 (remove lookup_char_pos_adj)
 - rust-lang#59747 (Copy book.toml unstable book generator)
 - rust-lang#59796 (Retire `IsNotConst` naming)
 - rust-lang#59804 (Clean up jobserver integration)
 - rust-lang#59818 (Eliminate `FnBox` usages from libstd.)
 - rust-lang#59830 (Fix links on keyword docs.)
 - rust-lang#59835 (Re-export NonZero signed variant in std)
 - rust-lang#59852 (std: Add `{read,write}_vectored` for more types)
 - rust-lang#59855 (Fix attributes position in type declaration)
 - rust-lang#59858 (Make duplicate matcher bindings a hard error)
 - rust-lang#59879 (Use SecRandomCopyBytes instead of /dev/urandom on MacOS)
 - rust-lang#59899 (In `-Zprint-type-size` output, sort enum variants by size.)
 - rust-lang#59912 (MaybeUninit: remove deprecated functions)
 - rust-lang#59925 (Fix paste error in split_ascii_whitespace docs.)
 - rust-lang#59930 (Exclude some copies of old book editions from search engines)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Apr 13, 2019
@bors
Auto merge of #59937 - Centril:rollup-6pousju, r=Centril
f1b5f80 
Rollup of 18 pull requests

Successful merges:

 - #59612 (Use normal newtype_index macro for MIR dataflows)
 - #59675 (Stabilize the `alloc` crate.)
 - #59708 (Mark variables captured by reference as mutable correctly)
 - #59735 (remove lookup_char_pos_adj)
 - #59747 (Copy book.toml unstable book generator)
 - #59796 (Retire `IsNotConst` naming)
 - #59804 (Clean up jobserver integration)
 - #59818 (Eliminate `FnBox` usages from libstd.)
 - #59830 (Fix links on keyword docs.)
 - #59835 (Re-export NonZero signed variant in std)
 - #59852 (std: Add `{read,write}_vectored` for more types)
 - #59855 (Fix attributes position in type declaration)
 - #59858 (Make duplicate matcher bindings a hard error)
 - #59879 (Use SecRandomCopyBytes instead of /dev/urandom on MacOS)
 - #59899 (In `-Zprint-type-size` output, sort enum variants by size.)
 - #59912 (MaybeUninit: remove deprecated functions)
 - #59925 (Fix paste error in split_ascii_whitespace docs.)
 - #59930 (Exclude some copies of old book editions from search engines)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Apr 13, 2019
@Centril
Rollup merge of rust-lang#59879 - ebarnard:patch-1, r=alexcrichton
0f8eace 
Use SecRandomCopyBytes instead of /dev/urandom on MacOS

SecRandomCopyBytes is [available since MacOS 10.7](https://developer.apple.com/documentation/security/1399291-secrandomcopybytes?language=objc) which is the minimum supported version and which was suggested in rust-lang#58901 (comment) is the earliest version currently in use.

This matches the behaviour of other platforms which have a random number generator syscall available.
@Centril Centril mentioned this pull request Apr 13, 2019
Closed
@Centril
Copy link
Contributor

Centril commented Apr 14, 2019

@bors r- since a r0llup worked without it.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 14, 2019
@ebarnard
Copy link
Contributor Author

I'm now less sure about the value of this PR. At least on MacOS SecRandomCopyBytes calls CCRandomCopyBytes with kCCRandomDefault. Internally CCRandomCopyBytes manages a CSPRNG which is seeded from /dev/random and which runs on its own thread accessed via GCD. This seems needlessly heavyweight for generating two u64s once per thread in hashmap_random_keys.

Security.framework also pulls in something which sets the __CF_USER_TEXT_ENCODING env var which was calling the rollup to fail.

I'm inclined to leave a comment to this effect in rand.rs and keep the existing behaviour of using /dev/urandom.

@alexcrichton
Copy link
Member

@ebarnard sounds reasonable to me!

@ebarnard ebarnard changed the title Use SecRandomCopyBytes instead of /dev/urandom on MacOS Add a comment explaining why SecRandomCopyBytes is not used on MacOS Apr 16, 2019
@ebarnard
Copy link
Contributor Author

Done

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 16, 2019

📌 Commit f1da89a has been approved by alexcrichton

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2019
@bors
Copy link
Contributor

bors commented Apr 16, 2019

⌛ Testing commit f1da89a with merge 70f1309...

bors added a commit that referenced this pull request Apr 16, 2019
@bors
Auto merge of #59879 - ebarnard:patch-1, r=alexcrichton
70f1309 
Add a comment explaining why SecRandomCopyBytes is not used on MacOS

SecRandomCopyBytes is [available since MacOS 10.7](https://developer.apple.com/documentation/security/1399291-secrandomcopybytes?language=objc) which is the minimum supported version and which was suggested in #58901 (comment) is the earliest version currently in use.

This matches the behaviour of other platforms which have a random number generator syscall available.
@bors
Copy link
Contributor

bors commented Apr 16, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 70f1309 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 16, 2019
@bors bors merged commit f1da89a into rust-lang:master Apr 16, 2019
This was referenced May 2, 2019
Closed
Closed
@RalfJung RalfJung mentioned this pull request Jun 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ebarnard @rust-highfive @sfackler @alexcrichton @bors @Centril @dtolnay