-
Notifications
You must be signed in to change notification settings - Fork 5
Add polyfills for RandomNumberGenerator class and Fill(Span<byte>) method
#76
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
Changes from 7 commits
dc0f73a
b26a09a
bce162b
df32acb
d44533d
981b9ca
1bf1f0c
5c023b4
0d9a36a
1207292
46d1155
a0ca669
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| using System; | ||
| using System.Security.Cryptography; | ||
| using FluentAssertions; | ||
| using Xunit; | ||
|
|
||
| namespace PolyShim.Tests.NetCore21; | ||
|
|
||
| public class RandomNumberGeneratorTests | ||
| { | ||
| [Fact] | ||
| public void Fill_Span_Test() | ||
| { | ||
| // Arrange | ||
| var span = new Span<byte>(new byte[16]); | ||
|
|
||
| // Act | ||
| RandomNumberGenerator.Fill(span); | ||
|
|
||
| // Assert | ||
| // Verify that not all bytes are zero (cryptographic RNG should produce non-zero data). | ||
| // Note: Theoretically, a crypto RNG could produce all zeros (probability: 2^-128), | ||
| // but this is astronomically unlikely in practice. | ||
| span.ToArray().Should().Contain(b => b != 0); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Fill_EmptySpan_Test() | ||
| { | ||
| // Arrange | ||
| var span = new Span<byte>(); | ||
|
|
||
| // Act & Assert | ||
| // Should not throw and should return immediately | ||
| RandomNumberGenerator.Fill(span); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| #if (NETCOREAPP && !NETCOREAPP2_1_OR_GREATER) || (NETFRAMEWORK) || (NETSTANDARD && !NETSTANDARD2_1_OR_GREATER) | ||
| #nullable enable | ||
| // ReSharper disable RedundantUsingDirective | ||
| // ReSharper disable CheckNamespace | ||
| // ReSharper disable InconsistentNaming | ||
| // ReSharper disable PartialTypeWithSinglePart | ||
|
|
||
| using System; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| #if !NETSTANDARD || NETSTANDARD1_3_OR_GREATER | ||
| using System.Security.Cryptography; | ||
|
|
||
| [ExcludeFromCodeCoverage] | ||
| internal static class MemberPolyfills_NetCore21_RandomNumberGenerator | ||
| { | ||
| extension(RandomNumberGenerator) | ||
| { | ||
| // https://learn.microsoft.com/dotnet/api/system.security.cryptography.randomnumbergenerator.fill | ||
| public static void Fill(Span<byte> data) | ||
| { | ||
| // Fast-path for empty spans to avoid unnecessary allocations | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment can be removed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed in 0d9a36a |
||
| if (data.Length == 0) | ||
| return; | ||
|
|
||
| var buffer = new byte[data.Length]; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this inside try
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved inside try in a0ca669 |
||
| #if NET35 | ||
| var rng = RandomNumberGenerator.Create(); | ||
| try | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I meant a using statement that ends with a semicolon and doesn't use curly braces.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed to
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the net35 exclusion? It should work the same everywhere @copilot
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the NET35 conditional by explicitly casting to IDisposable in the using statement, which works across all frameworks in 5c023b4 |
||
| { | ||
| rng.GetBytes(buffer); | ||
| buffer.CopyTo(data); | ||
| } | ||
| finally | ||
| { | ||
| ((IDisposable)rng).Dispose(); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explain in a comment why this cast is necessary
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added comment explaining it's needed for .NET Framework 3.5 compatibility in 1207292 |
||
| } | ||
| #else | ||
| using var rng = RandomNumberGenerator.Create(); | ||
| rng.GetBytes(buffer); | ||
| buffer.CopyTo(data); | ||
| #endif | ||
| } | ||
| } | ||
| } | ||
| #endif | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| # Signatures | ||
|
|
||
| - **Total:** 340 | ||
| - **Total:** 341 | ||
| - **Types:** 74 | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did this not increase?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The RandomNumberGenerator class polyfill is only active for netstandard < 1.3 (conditional compilation), so the signature generation script doesn't detect it when running on netstandard2.0 target where it generates the Signatures.md file.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes no sense. The script scans files as text it doesn't take conditional compilation into account. Are you sure you re-ran the build? Please investigate
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes no sense. The script scans files as text it doesn't take conditional compilation into account. Are you sure you re-ran the build? Please investigate
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You were right - the script does scan as text. The issue was the regex pattern
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You were right - the script does scan as text. The issue was the regex pattern was missing |
||
| - **Members:** 266 | ||
| - **Members:** 267 | ||
|
|
||
| ___ | ||
|
|
||
|
|
@@ -275,6 +275,8 @@ ___ | |
| - [`T[] GetItems<T>(T[], int)`](https://learn.microsoft.com/dotnet/api/system.random.getitems#system-random-getitems-1(-0()-system-int32)) <sup><sub>.NET 8.0</sub></sup> | ||
| - [`void NextBytes(Span<byte>)`](https://learn.microsoft.com/dotnet/api/system.random.nextbytes#system-random-nextbytes(system-span((system-byte)))) <sup><sub>.NET Core 2.1</sub></sup> | ||
| - [`void Shuffle<T>(T[])`](https://learn.microsoft.com/dotnet/api/system.random.shuffle#system-random-shuffle-1(-0())) <sup><sub>.NET 8.0</sub></sup> | ||
| - `RandomNumberGenerator` | ||
| - [`void Fill(Span<byte>)`](https://learn.microsoft.com/dotnet/api/system.security.cryptography.randomnumbergenerator.fill) <sup><sub>.NET Core 2.1</sub></sup> | ||
| - `Range` | ||
| - [**[struct]**](https://learn.microsoft.com/dotnet/api/system.range) <sup><sub>.NET Core 3.0</sub></sup> | ||
| - `ReadOnlyMemory<T>` | ||
|
|
||
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.
How do we get away with these boundaries if the class doesn't exist on some older targets?
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.
The nested
#if !NETSTANDARD || NETSTANDARD1_3_OR_GREATERon line 10 excludes the entire class definition for netstandard < 1.3 where RandomNumberGenerator doesn't exist. For those targets, this file compiles to empty content, so the polyfill simply isn't available (which is correct since both the class and Span don't exist there).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.
We provide Span for all targets, even net std 1.0. can we just create a polyfill for the Random number generator class for net std <1.3? It can just contain the bare minimum required functionality to work with our Fill method.
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.
Added RandomNumberGenerator polyfill for netstandard < 1.3 that wraps RNGCryptoServiceProvider using reflection in 0d9a36a