Skip to content

Commit 5e4e64e

Browse files
Copilotphilstopford
andcommitted
Fix CryptoRNG birthday paradox and Math.Abs overflow issues
Co-authored-by: philstopford <1983851+philstopford@users.noreply.github.com>
1 parent dd9ce40 commit 5e4e64e

File tree

2 files changed

+14
-7
lines changed

2 files changed

+14
-7
lines changed

Common/entropyRNG/crypto.cs

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ public static class Crypto_RNG
1919
new(() => RandomNumberGenerator.Create());
2020

2121
private static readonly ThreadLocal<byte[]> _threadLocalBuffer =
22-
new(() => new byte[sizeof(long)]); // Use long buffer for efficiency
22+
new(() => new byte[sizeof(uint)]); // Buffer for uint conversion
2323

2424
// Performance optimization: Box-Muller cached value for better efficiency
2525
[ThreadStatic] private static double _cachedGaussian;
@@ -60,16 +60,19 @@ private static double[] GenerateGaussianPair()
6060
double U1, U2;
6161

6262
// Performance optimization: Use do-while for better branch prediction
63+
// Use uint to avoid Math.Abs overflow issue with int.MinValue
6364
do
6465
{
65-
random.GetBytes(tmp.AsSpan(0, sizeof(int)));
66-
U1 = Math.Abs(BitConverter.ToInt32(tmp, 0)) / maxInt;
66+
random.GetBytes(tmp.AsSpan(0, sizeof(uint)));
67+
uint value1 = BitConverter.ToUInt32(tmp, 0);
68+
U1 = value1 / (double)uint.MaxValue;
6769
} while (U1 < MIN_RANDOM_VALUE);
6870

6971
do
7072
{
71-
random.GetBytes(tmp.AsSpan(0, sizeof(int)));
72-
U2 = Math.Abs(BitConverter.ToInt32(tmp, 0)) / maxInt;
73+
random.GetBytes(tmp.AsSpan(0, sizeof(uint)));
74+
uint value2 = BitConverter.ToUInt32(tmp, 0);
75+
U2 = value2 / (double)uint.MaxValue;
7376
} while (U2 < MIN_RANDOM_VALUE);
7477

7578
// Performance optimization: Pre-calculate common sub-expressions
@@ -90,7 +93,9 @@ public static double nextdouble()
9093
var tmp = _threadLocalBuffer.Value!;
9194

9295
random.GetBytes(tmp.AsSpan(0, sizeof(int)));
93-
return Math.Abs(BitConverter.ToInt32(tmp, 0)) / maxInt;
96+
// Use uint to avoid Math.Abs overflow issue with int.MinValue
97+
uint value = BitConverter.ToUInt32(tmp, 0);
98+
return value / (double)uint.MaxValue;
9499
}
95100

96101
[MethodImpl(MethodImplOptions.AggressiveInlining)]

UnitTests/RNGTests.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,9 @@ public static void CryptoRNGTest()
6464
.GroupBy(x => x) // group matching items
6565
.Where(g => g.Skip(1).Any()) // where the group contains more than one item
6666
.SelectMany(g => g).ToArray(); // re-expand the groups with more than one item
67-
Assert.That(values3.Distinct().Count(), Is.EqualTo(sampleCount));
67+
// Allow for occasional duplicates due to birthday paradox - with 25000 samples from uint.MaxValue space,
68+
// probability of collision is ~7%. Require at least 99.5% distinct values (at most 125 duplicates).
69+
Assert.That(values3.Distinct().Count(), Is.GreaterThanOrEqualTo((int)(sampleCount * 0.995)));
6870
}
6971

7072
[Test]

0 commit comments

Comments
 (0)