Moving to RandomNumberGenerator as CryptRandom is not supported in Mono #857
Conversation
|
I verified that a private with this fix makes antiforgery support work on mono. |
| @@ -17,6 +18,7 @@ namespace Microsoft.AspNet.Mvc | |||
| [DebuggerDisplay("{DebuggerString}")] | |||
| internal sealed class BinaryBlob : IEquatable<BinaryBlob> | |||
| { | |||
| private static readonly RandomNumberGenerator _randomNumberGenerator = RandomNumberGenerator.Create(); | |||
There was a problem hiding this comment.
What is the reason for caching the generator. Is there a significant cost to create the generator?
There was a problem hiding this comment.
It does mention that members aren't thread safe - http://msdn.microsoft.com/en-us/library/system.security.cryptography.randomnumbergenerator(v=vs.110).aspx. So invoking GetBytes concurrently might be bad.
There was a problem hiding this comment.
Well AFAIK RandomNumberGenerator.Create does some dictionary lookups inorder to figure out the default cryptographic random number generator ( and returns with RNGCryptoProvider instance), however actual perf would need to be measured. But as @pranavkm pointed out that GetBytes is not thread safe ( even though create is ) I think I will move out the cached and create a RandomNumberGenerator everytime a token is generated ( and cache it later if we see it to be the bottleneck).
(Unfortunately we don't have RNGCryptoProviderService in core clr, as that is thread safe).
There was a problem hiding this comment.
All implementations of RandomNumberGenerator are required to be thread-safe, so it's safe to use a cached instance.
|
Please get signoff from @GrabYourPitchforks, then |
|
|
1 similar comment
|
|
#847