- Notifications
You must be signed in to change notification settings - Fork5.2k
Use CryptographicallySecureRandomBytes for Guid generation#42770
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedSep 26, 2020
Tagging subscribers to this area:@bartonjs,@vcsjones,@krwq,@jeffhandley |
stephentoub commentedSep 26, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
What is the perf impact of this, on Linux and on macOS? I get that there's a desire for the Unix implementation to maintain undocumented behaviors that Windows happens to have, but if memory serves, the perf impact here especially on mac was substantial. I worry we're going to keep just going back and forth based on the latest issue filed. The latest issue cites a potential security pit of failure, but previous issues have cited essentially the same but for perf. |
jkotas commentedSep 26, 2020
This change has no perf impact on Linux. This change is only changing failure mode on Linux. Before this change, we would fallback to weak random number generator when something went wrong. After this change, exception will be thrown in this case. It should never happen in practice (famous last words). Yes, I expect that this be slower on macOS. I will try to get some numbers. |
jkotas commentedSep 26, 2020
The NewGuid performance is roughly the same on macOS and Linux with the current version of the change. On OSX 1015, this change makes This is due to underlying OS implementation changes. arc4random seems to have gotten much slower in OSX 1015. arc4random in OSX is actually implemented as wrapper over OSX core crypto APIs (see ifdef at the top ofhttps://opensource.apple.com/source/Libc/Libc-1353.100.2/gen/FreeBSD/arc4random.c.auto.html), so it picks up performance impacts of core crypto hardening. |
stephentoub commentedSep 26, 2020
That's much less than I was expecting. Thanks. |
stephentoub left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks
Uh oh!
There was an error while loading.Please reload this page.
vcsjones commentedSep 26, 2020
Just one question to help me understand...
But the numbers you posted show an improvement from 10.14 to 10.15. |
jkotas commentedSep 26, 2020
Here are numbers that I have collected in#42777: OSX 1014: OSX 1015: (Caveat: These numbers are for release build of the runtime + debug build of the libraries. They may be slightly smaller if everything is release build.) |
jkotas commentedSep 26, 2020
If we wanted to make this better, we may look at:
|
vcsjones commentedSep 26, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
As another data point, here are some numbers from my Mac on Or about 640ns for secure and 69ns for insecure. |
jkotas commentedSep 26, 2020
Hmm, I am wondering how the implementation of arc4random changed in the new macOS. |
am11 commentedSep 27, 2020
@vcsjones, just curious, are those numbers from Big Sur x64 or arm64? |
vcsjones commentedSep 27, 2020
@am11 x64. The ARM64 DTK agreement pretty strongly tells you to not post any benchmarks of any kind. (Or really even discuss it)😊 |
5f9b82c to5955cb5Compare
Fixes#42752