Add polyfills for RandomNumberGenerator.Fill(...)#73
Add polyfills for RandomNumberGenerator.Fill(...)#73GerardSmit wants to merge 1 commit intoTyrrrz:masterfrom
RandomNumberGenerator.Fill(...)#73Conversation
| @@ -0,0 +1,25 @@ | |||
| #if (NETCOREAPP && !NETCOREAPP2_1_OR_GREATER) || (NETFRAMEWORK && NET45_OR_GREATER) | |||
There was a problem hiding this comment.
This is missing net standard boundaries (up to 2.1)
There was a problem hiding this comment.
Pull request overview
Adds a PolyShim polyfill for System.Security.Cryptography.RandomNumberGenerator.Fill(Span<byte>) and introduces a corresponding unit test to validate the API surface/behavior for the NetCore21 polyfill set.
Changes:
- Add
RandomNumberGenerator.Fill(Span<byte>)polyfill implementation. - Add an xUnit test covering
RandomNumberGenerator.Fill(Span<byte>).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| PolyShim/NetCore21/RandomNumberGenerator.cs | Introduces the RandomNumberGenerator.Fill(Span<byte>) polyfill implementation behind conditional compilation. |
| PolyShim.Tests/NetCore21/RandomNumberGeneratorTests.cs | Adds a test invoking RandomNumberGenerator.Fill(...) and asserting the buffer contents changed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,25 @@ | |||
| #if (NETCOREAPP && !NETCOREAPP2_1_OR_GREATER) || (NETFRAMEWORK && NET45_OR_GREATER) | |||
There was a problem hiding this comment.
The compilation guard here is inconsistent with the rest of the NetCore21/* polyfills (which generally include NETSTANDARD && !NETSTANDARD2_1_OR_GREATER and don’t special-case NETFRAMEWORK && NET45_OR_GREATER). As written, this polyfill won’t be included for netstandard2.0 consumers, and it’s unclear why it should be excluded for net35/net40 but not other NetCore21 polyfills. Consider aligning the #if condition with the pattern used in PolyShim/NetCore21/DateTime.cs / Random.cs so the API is polyfilled for the intended TFMs.
| public static void Fill(Span<byte> data) | ||
| { | ||
| var buffer = new byte[data.Length]; | ||
| using var rng = RandomNumberGenerator.Create(); | ||
| rng.GetBytes(buffer); | ||
| buffer.CopyTo(data); |
There was a problem hiding this comment.
Fill(Span<byte>) always allocates a managed array and creates/disposes a RandomNumberGenerator, even when data.Length == 0. Adding a fast-path return for empty spans would avoid unnecessary allocations/crypto provider creation while preserving behavior.
| Span<byte> buffer = stackalloc byte[16]; | ||
|
|
||
| // Act | ||
| RandomNumberGenerator.Fill(buffer); | ||
|
|
||
| // Assert | ||
| // Since cryptographic random data is non-deterministic, | ||
| // we can only verify that the buffer contains some non-zero bytes | ||
| buffer.ToArray().Should().Contain(b => b != 0); |
There was a problem hiding this comment.
The test uses stackalloc byte[16], which is not guaranteed to be zero-initialized; the assertion can pass even if RandomNumberGenerator.Fill(...) is a no-op because the stack buffer may already contain non-zero bytes. Initialize the span (e.g., call buffer.Clear() or use a new byte[16]) before invoking Fill so the test actually validates that bytes were written.
|
Superseded by #76 |
Tyrrrz
added
duplicate
Adds https://learn.microsoft.com/dotnet/api/system.security.cryptography.randomnumbergenerator.fill