Implement Shuffle, GetString, and GetItems on Random{NumberGenerator} #78598

vcsjones · 2022-11-19T16:27:02Z

Implements:

Random.GetItems
Random.Shuffle
RandomNumberGenerator.GetString
RandomNumberGenerator.GetHexString
RandomNumberGenerator.Shuffle

This does both RandomNumberGenerator and Random in one go. If folks prefer I can split the PR.

ghost · 2022-11-19T16:27:08Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-11-19T16:27:14Z

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Implements:

Random.GetItems
Random.Shuffle
RandomNumberGenerator.GetString
RandomNumberGenerator.GetHexString
RandomNumberGenerator.Shuffle

This does both RandomNumberGenerator and Random in one go. If folks prefer I can split the PR.

Closes #73864.

Author:	vcsjones
Assignees:	vcsjones
Labels:	`area-System.Security`
Milestone:	-

stephentoub · 2022-11-19T16:40:02Z

Can you search around the repo and use these where it makes sense, e.g.

runtime/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/SymbolicRegexMatcher.Sample.cs

Line 179 in 6251d49

static T[] Shuffle<T>(Random random, T[] array)

runtime/src/libraries/Common/tests/System/RandomExtensions.cs

Line 10 in 57bfe47

public static void Shuffle<T>(this Random random, T[] array)

etc?

src/libraries/System.Private.CoreLib/src/System/Random.cs

stephentoub · 2022-11-20T00:31:14Z

Use where possible

A quick search suggests other opportunities, e.g.

d:\repos\runtime\src\libraries\System.Net.Sockets\tests\FunctionalTests\UnixDomainSocketTest.cs(545):result = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName() + new string('A', Random.Shared.Next(1, 32)));
d:\repos\runtime\src\libraries\System.Net.WebClient\tests\WebClientTest.cs(664):new string(Enumerable.Range(0, 512 * 1024).Select(_ => (char)('a' + Random.Shared.Next(0, 26))).ToArray());
d:\repos\runtime\src\libraries\Common\tests\System\Net\Http\PostScenarioTest.cs(151):var rand = new Random(42);
d:\repos\runtime\src\libraries\Common\tests\Tests\System\StringTests.cs(3776):Random rand = new Random(42);
d:\repos\runtime\src\libraries\Common\tests\Tests\System\StringTests.cs(3810):Random rand = new Random(42);
d:\repos\runtime\src\libraries\Common\tests\Tests\System\StringTests.cs(3832):Random rand = new Random(42);
d:\repos\runtime\src\libraries\Common\tests\TestUtilities\System\IO\FileCleanupTestBase.cs(178):Random random = new Random();
d:\repos\runtime\src\libraries\Common\tests\TestUtilities\System\IO\PathGenerator.cs(11):private static Random _rand = new Random();
d:\repos\runtime\src\libraries\Common\tests\TestUtilities\RandomTestCaseOrderer.cs(59):static List<T> Randomize(List<T> tests)
d:\repos\runtime\src\libraries\Common\tests\System\Net\Http\PostScenarioTest.cs(155):sb.Append((char)(rand.Next(0, 26) + 'a'));
d:\repos\runtime\src\libraries\Common\tests\TestUtilities\System\IO\FileCleanupTestBase.cs(181):sb.Append((char)('a' + random.Next(0, 26)));
d:\repos\runtime\src\libraries\System.IO\tests\StreamReader\StreamReaderTests.cs(407):data[i] = (char)('a' + r.Next(0, 26));
d:\repos\runtime\src\libraries\System.IO\tests\StreamReader\StreamReaderTests.cs(438):data[i] = (char)('a' + r.Next(0, 26));
d:\repos\runtime\src\libraries\System.IO\tests\StreamReader\StreamReaderTests.cs(469):data[i] = (char)('a' + r.Next(0, 26));
d:\repos\runtime\src\libraries\System.IO\tests\StreamReader\StreamReaderTests.cs(500):data[i] = (char)('a' + r.Next(0, 26));
d:\repos\runtime\src\libraries\System.IO\tests\StreamWriter\StreamWriter.WriteTests.cs(257):data[i] = (char)(rand.Next(0, 26) + 'a');
d:\repos\runtime\src\libraries\System.IO\tests\StreamWriter\StreamWriter.WriteTests.cs(290):data[i] = (char)(rand.Next(0, 26) + 'a');
d:\repos\runtime\src\libraries\System.IO\tests\StreamWriter\StreamWriter.WriteTests.cs(323):data[i] = (char)(rand.Next(0, 26) + 'a');
d:\repos\runtime\src\libraries\System.IO\tests\StreamWriter\StreamWriter.WriteTests.cs(356):data[i] = (char)(rand.Next(0, 26) + 'a');
d:\repos\runtime\src\libraries\System.Net.HttpListener\tests\HttpRequestStreamTests.cs(234):.Select(_ => (byte)('a' + rand.Next(0, 26)))
d:\repos\runtime\src\libraries\System.Net.HttpListener\tests\HttpRequestStreamTests.cs(274):.Select(_ => (byte)('a' + rand.Next(0, 26)))
d:\repos\runtime\src\libraries\System.Net.WebClient\tests\WebClientTest.cs(664):new string(Enumerable.Range(0, 512 * 1024).Select(_ => (char)('a' + Random.Shared.Next(0, 26))).ToArray());

vcsjones · 2022-11-20T01:57:39Z

A quick search suggests other opportunities, e.g.

I excluded searching in /tests/, but if we want to, that's fine. I'm generally risk-adverse touching tests purely for refactoring / using new APIs, but this seems simple enough.

vcsjones · 2022-11-20T03:28:42Z

A quick search suggests other opportunities, e.g.

I checked in a few more. Several of them I left unchanged because they have configurations that build for NetCoreAppMinimum or NetFramework. It didn't make sense to #ifdef in tests to me.

...aries/System.Security.Cryptography/src/System/Security/Cryptography/RandomNumberGenerator.cs

Co-authored-by: Benjamin Moir <[email protected]> Co-authored-by: Theodore Tsirpanis <[email protected]> Co-authored-by: Michał Petryka <[email protected]>

vcsjones · 2022-11-20T19:03:49Z

Sorry for the force push. I goofed on Co-authored-by and did not want to leave Michał out.

src/libraries/System.Private.CoreLib/src/System/Random.cs

src/libraries/Common/tests/Tests/System/StringTests.cs

...braries/System.DirectoryServices/src/System/DirectoryServices/ActiveDirectory/TrustHelper.cs

src/libraries/System.Linq/tests/Shuffler.cs

src/libraries/System.Net.WebClient/tests/WebClientTest.cs

...aries/System.Security.Cryptography/src/System/Security/Cryptography/RandomNumberGenerator.cs

Co-authored-by: Dan Moseley <[email protected]>

stephentoub · 2023-01-04T22:14:30Z

@vcsjones, when you get a chance, could you take a look at the pending comments/questions? Then we can get this merged. Thanks.

...aries/System.Security.Cryptography/src/System/Security/Cryptography/RandomNumberGenerator.cs

bartonjs · 2023-01-06T19:22:07Z

...aries/System.Security.Cryptography/src/System/Security/Cryptography/RandomNumberGenerator.cs

+            for (int i = 0; i < destination.Length; i++)
+            {
+                destination[i] = choices[GetInt32(choices.Length)];
+            }


Similar to the N virtual calls concern in System.Random, it might be useful to change this to do something like

Span<byte> randomBytes = stackalloc byte[128]; Span<int> randomIntStorage = stackalloc int[randomBytes.Length / sizeof(int)]; int randomInts = 0; for (int i = destination.Length - 1; i >= 0; i--) { while (randomInts <= 0) { if (i < randomIntStorage.Length) { randomBytes = randomBytes.Slice(0, i * sizeof(int)); } Fill(randomBytes); randomInts = FilterInt32(randomBytes, randomIntStorage); } randomInts--; destination[i] = choices[randomInts[randomInts]]; }

to reduce P/Invokes by a factor of randomIntStorage.Length.

Though that requires writing FilterInt32, and the complexity of above, so it should be checked to see if the perf justifies it.

(This is an idea for later, not a "right now")

…/Cryptography/RandomNumberGenerator.cs Co-authored-by: Jeremy Barton <[email protected]>

davidfowl · 2023-01-13T15:06:29Z

Very nice APIs! I wonder if we should have T PickRandom(this IReadOnlyList<T> source)

stephentoub · 2023-01-13T15:10:44Z

Very nice APIs! I wonder if we should have T PickRandom(this IReadOnlyList source)

Concept seems reasonable, though to nit pick, to match the other APIs we'd probably want it to be called GetItem rather than PickRandom. Can you open a separate issue? Are there places it would be used in ASP.NET? I assume this is essentially shorthand for: source[RandomNumberGenerator.GetInt32(source.Count)]?

davidfowl · 2023-01-13T15:14:24Z

Can you open a separate issue?

I can.

Are there places it would be used in ASP.NET?

Not sure, I was building something with YARP when this came up.

Also would be used here

I assume this is essentially shorthand for: source[RandomNumberGenerator.GetInt32(source.Count)]?

Yep!

davidfowl · 2023-01-14T06:47:20Z

Filed an issue here

Implement Shuffle, GetString, and GetItems.

2cb8813

vcsjones added the area-System.Security label Nov 19, 2022

vcsjones self-assigned this Nov 19, 2022

ghost added the new-api-needs-documentation label Nov 19, 2022

vcsjones requested review from bartonjs and stephentoub November 19, 2022 16:27

Cleanup and additional tests

30a07bf

stephentoub reviewed Nov 19, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Random.cs Outdated Show resolved Hide resolved

stephentoub reviewed Nov 19, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Random.cs Outdated Show resolved Hide resolved

stephentoub reviewed Nov 19, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Random.cs Outdated Show resolved Hide resolved

vcsjones added 2 commits November 19, 2022 12:05

Use where possible

570d879

Code review feedback

e3081bd

gfoidl reviewed Nov 19, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Random.cs Show resolved Hide resolved

Additional uses of Random throughout tests

516fb02

Feedback on documentation

ff3fbaa

MichalPetryka reviewed Nov 20, 2022

View reviewed changes

...aries/System.Security.Cryptography/src/System/Security/Cryptography/RandomNumberGenerator.cs Outdated Show resolved Hide resolved

Use ref to span to get span in string.Create.

d87b234

Co-authored-by: Benjamin Moir <[email protected]> Co-authored-by: Theodore Tsirpanis <[email protected]> Co-authored-by: Michał Petryka <[email protected]>

vcsjones force-pushed the random-things branch from 3c6aa37 to d87b234 Compare November 20, 2022 19:02

danmoseley reviewed Nov 20, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Random.cs Outdated Show resolved Hide resolved

stephentoub reviewed Nov 21, 2022

View reviewed changes

src/libraries/Common/tests/Tests/System/StringTests.cs Show resolved Hide resolved

stephentoub reviewed Nov 21, 2022

View reviewed changes

...braries/System.DirectoryServices/src/System/DirectoryServices/ActiveDirectory/TrustHelper.cs Show resolved Hide resolved

stephentoub reviewed Nov 21, 2022

View reviewed changes

src/libraries/System.Linq/tests/Shuffler.cs Outdated Show resolved Hide resolved

stephentoub reviewed Nov 21, 2022

View reviewed changes

src/libraries/System.Net.WebClient/tests/WebClientTest.cs Outdated Show resolved Hide resolved

danmoseley reviewed Dec 8, 2022

View reviewed changes

...aries/System.Security.Cryptography/src/System/Security/Cryptography/RandomNumberGenerator.cs Outdated Show resolved Hide resolved

danmoseley reviewed Dec 8, 2022

View reviewed changes

...aries/System.Security.Cryptography/src/System/Security/Cryptography/RandomNumberGenerator.cs Outdated Show resolved Hide resolved

danmoseley reviewed Dec 8, 2022

View reviewed changes

...aries/System.Security.Cryptography/src/System/Security/Cryptography/RandomNumberGenerator.cs Show resolved Hide resolved

Apply suggestions from code review

84b1988

Co-authored-by: Dan Moseley <[email protected]>

build-analysis bot mentioned this pull request Dec 9, 2022

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

5 tasks

vcsjones added 4 commits January 6, 2023 11:12

Merge remote-tracking branch 'ms/main' into random-things

96e1351

Use AssertExtensions over Assert

4c54ebe

Improve comment

5563e6b

No idea where AssertHexString came from

7fc84e4

bartonjs reviewed Jan 6, 2023

View reviewed changes

...aries/System.Security.Cryptography/src/System/Security/Cryptography/RandomNumberGenerator.cs Outdated Show resolved Hide resolved

bartonjs reviewed Jan 6, 2023

View reviewed changes

bartonjs approved these changes Jan 6, 2023

View reviewed changes

Update src/libraries/System.Security.Cryptography/src/System/Security…

e42a27a

…/Cryptography/RandomNumberGenerator.cs Co-authored-by: Jeremy Barton <[email protected]>

build-analysis bot mentioned this pull request Jan 12, 2023

Build fails with "eng/common/tools.sh: line 474: 537 Segmentation fault" #76759

Closed

stephentoub merged commit 05e1fe1 into dotnet:main Jan 12, 2023

vcsjones deleted the random-things branch January 12, 2023 23:04

gfoidl mentioned this pull request Jan 13, 2023

Add new "api" project template that supports native AOT dotnet/aspnetcore#46064

Merged

davidfowl mentioned this pull request Jan 16, 2023

[API Proposal]: Add Random.GetItem(IReadOnlyList<T> choices) #80651

Closed

This was referenced Jan 27, 2023

Update "api" project template to use new methods on Random dotnet/aspnetcore#46087

Closed

Update api template to use new Random APIs dotnet/aspnetcore#46298

Merged

ghost locked as resolved and limited conversation to collaborators Feb 13, 2023

bartonjs added cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. tracking This issue is tracking the completion of other related issues. labels Jul 29, 2024

bartonjs removed tracking This issue is tracking the completion of other related issues. cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. labels Aug 15, 2024

Implement Shuffle, GetString, and GetItems on Random{NumberGenerator} #78598

Implement Shuffle, GetString, and GetItems on Random{NumberGenerator} #78598

Uh oh!

Conversation

vcsjones commented Nov 19, 2022

Uh oh!

ghost commented Nov 19, 2022

Uh oh!

ghost commented Nov 19, 2022

Uh oh!

stephentoub commented Nov 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentoub commented Nov 20, 2022

Uh oh!

vcsjones commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vcsjones commented Nov 20, 2022

Uh oh!

Uh oh!

vcsjones commented Nov 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentoub commented Jan 4, 2023

Uh oh!

Uh oh!

bartonjs Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Jan 13, 2023

Uh oh!

stephentoub commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidfowl commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidfowl commented Jan 14, 2023

Uh oh!

Uh oh!

vcsjones commented Nov 20, 2022 •

edited

Loading

bartonjs Jan 6, 2023 •

edited

Loading

stephentoub commented Jan 13, 2023 •

edited

Loading

davidfowl commented Jan 13, 2023 •

edited

Loading