Skip to content
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

OverflowException for random ulong #581

Open
reuterma24 opened this issue Dec 20, 2024 · 0 comments
Open

OverflowException for random ulong #581

reuterma24 opened this issue Dec 20, 2024 · 0 comments

Comments

@reuterma24
Copy link

reuterma24 commented Dec 20, 2024

Bogus NuGet Package

v35.6.1

.NET Version

.NET 8

Visual Studio Version

No response

What operating system are you using?

MacOS

What locale are you using with Bogus?

en

Problem Description

Hi everbody! 😃

I just stumbled across an issue with the ULong method.
According to the documentation, it generates a random ulong between ulong.MinValue and ulong.MaxValue.
Unfortunately the method crashes due to an arithmetic overflow if the min parameter is very large (relatively close to MaxValue).

LINQPad Example or Reproduction Steps

The following test case will fail and trigger the overflow:

[Fact]
public void overflow_test()
{
    var randomizer = new Randomizer();
    ulong max = ulong.MaxValue;
    ulong min = max - 10; // I could go up to -3000 on my machine
    
    ulong result = randomizer.ULong(min, max);
  
    result.Should().BeInRange(min, max);
}

Expected Behavior

Expected would be a random ulong in the desired range instead of a crash.

Actual Behavior

Actual implementation:

public ulong ULong(ulong min = ulong.MinValue, ulong max = ulong.MaxValue)
{
    return Convert.ToUInt64(Double() * (max - min) + min);
}
Stacktrace:
Failed Bogus.Tests.RandomizerTest.overflow_test [5 ms]
  Error Message:
   System.OverflowException : Arithmetic operation resulted in an overflow.
  Stack Trace:
     at System.Convert.ToUInt64(Double value)
   at Bogus.Randomizer.ULong(UInt64 min, UInt64 max)

Altough I am not an expert on this, I believe it happens because of floating-point rounding.

  • The Double() method generates a random value between 0 and 1.
  • When multiplied by (max - min) — in this case, 10 — it produces a small double with many decimal places
    (like 6.5668499183687 for example).
  • Adding the very large min value (Double() * (max - min) + min) results in a value where the mantissa cannot accurately represent the least significant bits, causing it to round up and exceed ulong.MaxValue.

Known Workarounds

I think a very simple way of fixing this and avoiding the rounding error is to convert the randomized double to a ulong before adding min, just like this:

public ulong ULong(ulong min = ulong.MinValue, ulong max = ulong.MaxValue)
{
    return Convert.ToUInt64(Double() * (max - min)) + min;
}

This implementation also passes all test cases inside RandomizerTest.cs.

Could you help with a pull-request?

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant