Skip to content

Fix Random.Next(int, int) on 64-bit when max-min overflows int.MaxValue #50922

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 11, 2021
Merged

Fix Random.Next(int, int) on 64-bit when max-min overflows int.MaxValue #50922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e55c7db
Fix Random.Next(int, int) on 64-bit when max-min overflow int
stephentoub Apr 8, 2021
806e2a0
Update Random.cs
stephentoub Apr 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libraries/System.Private.CoreLib/src/System/Random.Xoshiro256StarStarImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ public override int Next(int maxValue)

public override int Next(int minValue, int maxValue)
{
ulong exclusiveRange = (ulong)(maxValue - minValue);
ulong exclusiveRange = (ulong)((long)maxValue - minValue);

if (exclusiveRange > 1)
{
Expand Down
112 changes: 95 additions & 17 deletions src/libraries/System.Private.CoreLib/src/System/Random.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,12 @@ protected private Random(bool isThreadSafeRandom)

/// <summary>Returns a non-negative random integer.</summary>
/// <returns>A 32-bit signed integer that is greater than or equal to 0 and less than <see cref="int.MaxValue"/>.</returns>
public virtual int Next() => _impl.Next();
public virtual int Next()
{
int result = _impl.Next();
AssertInRange(result, 0, int.MaxValue);
return result;
}

/// <summary>Returns a non-negative random integer that is less than the specified maximum.</summary>
/// <param name="maxValue">The exclusive upper bound of the random number to be generated. <paramref name="maxValue"/> must be greater than or equal to 0.</param>
Expand All @@ -68,7 +73,9 @@ public virtual int Next(int maxValue)
ThrowMaxValueMustBeNonNegative();
}

return _impl.Next(maxValue);
int result = _impl.Next(maxValue);
AssertInRange(result, 0, maxValue);
return result;
}

/// <summary>Returns a random integer that is within a specified range.</summary>
Expand All @@ -86,12 +93,19 @@ public virtual int Next(int minValue, int maxValue)
ThrowMinMaxValueSwapped();
}

return _impl.Next(minValue, maxValue);
int result = _impl.Next(minValue, maxValue);
AssertInRange(result, minValue, maxValue);
return result;
}

/// <summary>Returns a non-negative random integer.</summary>
/// <returns>A 64-bit signed integer that is greater than or equal to 0 and less than <see cref="long.MaxValue"/>.</returns>
public virtual long NextInt64() => _impl.NextInt64();
public virtual long NextInt64()
{
long result = _impl.NextInt64();
AssertInRange(result, 0, long.MaxValue);
return result;
}

/// <summary>Returns a non-negative random integer that is less than the specified maximum.</summary>
/// <param name="maxValue">The exclusive upper bound of the random number to be generated. <paramref name="maxValue"/> must be greater than or equal to 0.</param>
Expand All @@ -107,7 +121,9 @@ public virtual long NextInt64(long maxValue)
ThrowMaxValueMustBeNonNegative();
}

return _impl.NextInt64(maxValue);
long result = _impl.NextInt64(maxValue);
AssertInRange(result, 0, maxValue);
return result;
}

/// <summary>Returns a random integer that is within a specified range.</summary>
Expand All @@ -125,16 +141,28 @@ public virtual long NextInt64(long minValue, long maxValue)
ThrowMinMaxValueSwapped();
}

return _impl.NextInt64(minValue, maxValue);
long result = _impl.NextInt64(minValue, maxValue);
AssertInRange(result, minValue, maxValue);
return result;
}

/// <summary>Returns a random floating-point number that is greater than or equal to 0.0, and less than 1.0.</summary>
/// <returns>A single-precision floating point number that is greater than or equal to 0.0, and less than 1.0.</returns>
public virtual float NextSingle() => _impl.NextSingle();
public virtual float NextSingle()
{
float result = _impl.NextSingle();
AssertInRange(result);
return result;
}

/// <summary>Returns a random floating-point number that is greater than or equal to 0.0, and less than 1.0.</summary>
/// <returns>A double-precision floating point number that is greater than or equal to 0.0, and less than 1.0.</returns>
public virtual double NextDouble() => _impl.NextDouble();
public virtual double NextDouble()
{
double result = _impl.NextDouble();
AssertInRange(result);
return result;
}

/// <summary>Fills the elements of a specified array of bytes with random numbers.</summary>
/// <param name="buffer">The array to be filled with random numbers.</param>
Expand All @@ -155,14 +183,36 @@ public virtual void NextBytes(byte[] buffer)

/// <summary>Returns a random floating-point number between 0.0 and 1.0.</summary>
/// <returns>A double-precision floating point number that is greater than or equal to 0.0, and less than 1.0.</returns>
protected virtual double Sample() => _impl.Sample();
protected virtual double Sample()
{
double result = _impl.Sample();
AssertInRange(result);
return result;
}

private static void ThrowMaxValueMustBeNonNegative() =>
throw new ArgumentOutOfRangeException("maxValue", SR.Format(SR.ArgumentOutOfRange_NeedNonNegNum, "maxValue"));

private static void ThrowMinMaxValueSwapped() =>
throw new ArgumentOutOfRangeException("minValue", SR.Format(SR.Argument_MinMaxValue, "minValue", "maxValue"));

[Conditional("DEBUG")]
private static void AssertInRange(long result, long minInclusive, long maxExclusive)
{
if (maxExclusive > minInclusive)
{
Debug.Assert(result >= minInclusive && result < maxExclusive, $"Expected {minInclusive} <= {result} < {maxExclusive}");
}
else
{
Debug.Assert(result == minInclusive, $"Expected {minInclusive} == {result}");
}
}

[Conditional("DEBUG")]
private static void AssertInRange(double result) =>
Debug.Assert(result >= 0.0 && result < 1.0f, $"Expected 0.0 <= {result} < 1.0");

/// <summary>Random implementation that delegates all calls to a ThreadStatic Impl instance.</summary>
private sealed class ThreadSafeRandom : Random
{
Expand All @@ -189,7 +239,12 @@ public ThreadSafeRandom() : base(isThreadSafeRandom: true) { }
[MethodImpl(MethodImplOptions.NoInlining)]
private static XoshiroImpl Create() => t_random = new();

public override int Next() => LocalRandom.Next();
public override int Next()
{
int result = LocalRandom.Next();
AssertInRange(result, 0, int.MaxValue);
return result;
}

public override int Next(int maxValue)
{
Expand All @@ -198,7 +253,9 @@ public override int Next(int maxValue)
ThrowMaxValueMustBeNonNegative();
}

return LocalRandom.Next(maxValue);
int result = LocalRandom.Next(maxValue);
AssertInRange(result, 0, maxValue);
return result;
}

public override int Next(int minValue, int maxValue)
Expand All @@ -208,10 +265,17 @@ public override int Next(int minValue, int maxValue)
ThrowMinMaxValueSwapped();
}

return LocalRandom.Next(minValue, maxValue);
int result = LocalRandom.Next(minValue, maxValue);
AssertInRange(result, minValue, maxValue);
return result;
}

public override long NextInt64() => LocalRandom.NextInt64();
public override long NextInt64()
{
long result = LocalRandom.NextInt64();
AssertInRange(result, 0, long.MaxValue);
return result;
}

public override long NextInt64(long maxValue)
{
Expand All @@ -220,7 +284,9 @@ public override long NextInt64(long maxValue)
ThrowMaxValueMustBeNonNegative();
}

return LocalRandom.NextInt64(maxValue);
long result = LocalRandom.NextInt64(maxValue);
AssertInRange(result, 0, maxValue);
return result;
}

public override long NextInt64(long minValue, long maxValue)
Expand All @@ -230,12 +296,24 @@ public override long NextInt64(long minValue, long maxValue)
ThrowMinMaxValueSwapped();
}

return LocalRandom.NextInt64(minValue, maxValue);
long result = LocalRandom.NextInt64(minValue, maxValue);
AssertInRange(result, minValue, maxValue);
return result;
}

public override float NextSingle() => LocalRandom.NextSingle();
public override float NextSingle()
{
float result = LocalRandom.NextSingle();
AssertInRange(result);
return result;
}

public override double NextDouble() => LocalRandom.NextDouble();
public override double NextDouble()
{
double result = LocalRandom.NextDouble();
AssertInRange(result);
return result;
}

public override void NextBytes(byte[] buffer)
{
Expand Down
52 changes: 51 additions & 1 deletion src/libraries/System.Runtime.Extensions/tests/System/Random.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,31 @@ public void Next_IntInt_AllValuesWithinSmallRangeHit(bool derived, bool seeded)
Assert.DoesNotContain(44, hs);
}

public static IEnumerable<object[]> Next_IntInt_Next_IntInt_AllValuesAreWithinRange_MemberData() =>
from derived in new[] { false, true }
from seeded in new[] { false, true }
from (int min, int max) pair in new[]
{
(1, 2),
(-10, -3),
(0, int.MaxValue),
(-1, int.MaxValue),
(int.MinValue, 0),
(int.MinValue, int.MaxValue),
}
select new object[] { derived, seeded, pair.min, pair.max };

[Theory]
[MemberData(nameof(Next_IntInt_Next_IntInt_AllValuesAreWithinRange_MemberData))]
public void Next_IntInt_Next_IntInt_AllValuesAreWithinRange(bool derived, bool seeded, int min, int max)
{
Random r = Create(derived, seeded);
for (int i = 0; i < 100; i++)
{
Assert.InRange(r.Next(min, max), min, max - 1);
}
}

[Theory]
[InlineData(false, false)]
[InlineData(false, true)]
Expand Down Expand Up @@ -178,6 +203,31 @@ public void Next_LongLong_AllValuesWithinSmallRangeHit(bool derived, bool seeded
Assert.DoesNotContain(44L, hs);
}

public static IEnumerable<object[]> Next_LongLong_Next_IntInt_AllValuesAreWithinRange_MemberData() =>
from derived in new[] { false, true }
from seeded in new[] { false, true }
from (long min, long max) pair in new[]
{
(1L, 2L),
(0L, long.MaxValue),
(2147483648, 2147483658),
(-1L, long.MaxValue),
(long.MinValue, 0L),
(long.MinValue, long.MaxValue),
}
select new object[] { derived, seeded, pair.min, pair.max };

[Theory]
[MemberData(nameof(Next_LongLong_Next_IntInt_AllValuesAreWithinRange_MemberData))]
public void Next_LongLong_Next_IntInt_AllValuesAreWithinRange(bool derived, bool seeded, long min, long max)
{
Random r = Create(derived, seeded);
for (int i = 0; i < 100; i++)
{
Assert.InRange(r.NextInt64(min, max), min, max - 1);
}
}

[Theory]
[InlineData(false)]
[InlineData(true)]
Expand Down Expand Up @@ -502,7 +552,7 @@ public void Xoshiro_AlgorithmBehavesAsExpected()
Assert.Equal(12, randOuter.Next(0, 42));
Assert.Equal(7234, randOuter.Next(42, 12345));
Assert.Equal(2147483642, randOuter.Next(int.MaxValue - 5, int.MaxValue));
Assert.Equal(1981894504, randOuter.Next(int.MinValue, int.MaxValue));
Assert.Equal(-1236260882, randOuter.Next(int.MinValue, int.MaxValue));

Assert.Equal(3644728249650840822, randOuter.NextInt64());
Assert.Equal(2809750975933744783, randOuter.NextInt64());
Expand Down