Skip to content

Restrict the maximum length of BigInteger #102874

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
tannergooding merged 3 commits into from
May 31, 2024
Merged

Restrict the maximum length of BigInteger #102874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db7c685
Restrict the maximum length of BigInteger
tannergooding May 30, 2024
f320969
Fixing the op_rightshift test to use the new limit
tannergooding May 30, 2024
8a1ebd8
Don't throw an overflow exception unnecessarily
tannergooding May 30, 2024
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
138 changes: 73 additions & 65 deletions src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@ public readonly struct BigInteger
internal const int kcbitUlong = 64;
internal const int DecimalScaleFactorMask = 0x00FF0000;

// Various APIs only allow up to int.MaxValue bits, so we will restrict ourselves
// to fit within this given our underlying storage representation and the maximum
// array length. This gives us just shy of 256MB as the largest allocation size.
//
// Such a value allows for almost 646,456,974 digits, which is more than large enough
// for typical scenarios. If user code requires more than this, they should likely
// roll their own type that utilizes native memory and other specialized techniques.
internal static int MaxLength => Array.MaxLength / kcbitUint;

// For values int.MinValue < n <= int.MaxValue, the value is stored in sign
// and _bits is null. For all other values, sign is +1 or -1 and the bits are in _bits
internal readonly int _sign; // Do not rename (binary serialization)
Expand Down Expand Up @@ -487,22 +496,22 @@ internal BigInteger(int n, uint[]? rgu)
/// <param name="negative">The bool indicating the sign of the value.</param>
private BigInteger(ReadOnlySpan<uint> value, bool negative)
{
// Try to conserve space as much as possible by checking for wasted leading span entries
// sometimes the span has leading zeros from bit manipulation operations & and ^

int length = value.LastIndexOfAnyExcept(0u) + 1;
value = value[..length];

if (value.Length > MaxLength)
{
ThrowHelper.ThrowOverflowException();
}

int len;

// Try to conserve space as much as possible by checking for wasted leading span entries
// sometimes the span has leading zeros from bit manipulation operations & and ^
for (len = value.Length; len > 0 && value[len - 1] == 0; len--);

if (len == 0)
if (value.Length == 0)
{
this = s_bnZeroInt;
this = default;
}
else if (len == 1 && value[0] < kuMaskHighBit)
else if (value.Length == 1 && value[0] < kuMaskHighBit)
{
// Values like (Int32.MaxValue+1) are stored as "0x80000000" and as such cannot be packed into _sign
_sign = negative ? -(int)value[0] : (int)value[0];
Expand All @@ -516,7 +525,7 @@ private BigInteger(ReadOnlySpan<uint> value, bool negative)
else
{
_sign = negative ? -1 : +1;
_bits = value.Slice(0, len).ToArray();
_bits = value.ToArray();
}
AssertValid();
}
Expand All @@ -527,87 +536,88 @@ private BigInteger(ReadOnlySpan<uint> value, bool negative)
/// <param name="value"></param>
private BigInteger(Span<uint> value)
Copy link
Copy Markdown
Member

@stephentoub stephentoub May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why we have two separate implementations one for Span<uint> and one for ReadOnlySpan<uint> + isNegative. Could this ctor just do the checks to compute isNegative and then delegate to the other in order to avoid duplication? Or are they dealing with completely different formats?

Copy link
Copy Markdown
Member Author

@tannergooding tannergooding May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly because BigInteger is an old type in desperate need of a rewrite 😆. I imagine we could, with a bit more refactoring, share some or most of the implementation here.

private BigInteger(ReadOnlySpan<uint> value, bool negative) is an optimized implementation that assumes we are already in almost the right shape. So value is the unsigned storage data (the absolute value) and we're really just trimming unnecessary leading zeros and deciding whether to store it in _bits or compress it into _sign.

private BigInteger(Span<uint> value) on the other hand is taking a two's complement little-endian span. So it has to detect if its positive or negative and if its negative fix it up to be the absolute value for storage purposes. So I expect we could implement it as:

  • determine if negative, trimming leading sign data (0xFF if negative, 0x00 if positive)
  • if negative, compute the two's complement (absolute value)
  • call private BigInteger(ReadOnlySpan<uint> value, bool negative)

{
bool isNegative;
int length;

if ((value.Length > 0) && ((int)value[^1] < 0))
{
isNegative = true;
length = value.LastIndexOfAnyExcept(uint.MaxValue) + 1;

if ((length == 0) || ((int)value[length - 1] > 0))
{
// We ne need to preserve the sign bit
length++;
}
Debug.Assert((int)value[length - 1] < 0);
}
else
{
isNegative = false;
length = value.LastIndexOfAnyExcept(0u) + 1;
}
value = value[..length];

if (value.Length > MaxLength)
{
ThrowHelper.ThrowOverflowException();
}

int dwordCount = value.Length;
bool isNegative = dwordCount > 0 && ((value[dwordCount - 1] & kuMaskHighBit) == kuMaskHighBit);

// Try to conserve space as much as possible by checking for wasted leading span entries
while (dwordCount > 0 && value[dwordCount - 1] == 0) dwordCount--;

if (dwordCount == 0)
if (value.Length == 0)
{
// BigInteger.Zero
// 0
this = s_bnZeroInt;
AssertValid();
return;
}
if (dwordCount == 1)
else if (value.Length == 1)
{
if (unchecked((int)value[0]) < 0 && !isNegative)
if (isNegative)
{
_bits = new uint[1];
_bits[0] = value[0];
_sign = +1;
if (value[0] == uint.MaxValue)
{
// -1
this = s_bnMinusOneInt;
}
else if (value[0] == kuMaskHighBit)
{
// int.MinValue
this = s_bnMinInt;
}
else
{
_sign = unchecked((int)value[0]);
_bits = null;
}
}
// Handle the special cases where the BigInteger likely fits into _sign
else if (int.MinValue == unchecked((int)value[0]))
else if (unchecked((int)value[0]) < 0)
{
this = s_bnMinInt;
_sign = +1;
_bits = [value[0]];
}
else
{
_sign = unchecked((int)value[0]);
_bits = null;
}
AssertValid();
return;
}

if (!isNegative)
else
{
// Handle the simple positive value cases where the input is already in sign magnitude
_sign = +1;
value = value.Slice(0, dwordCount);
_bits = value.ToArray();
AssertValid();
return;
}

// Finally handle the more complex cases where we must transform the input into sign magnitude
NumericsHelpers.DangerousMakeTwosComplement(value); // mutates val
if (isNegative)
{
NumericsHelpers.DangerousMakeTwosComplement(value);

// Pack _bits to remove any wasted space after the twos complement
int len = value.Length;
while (len > 0 && value[len - 1] == 0) len--;
// Retrim any leading zeros carried from the sign
length = value.LastIndexOfAnyExcept(0u) + 1;
value = value[..length];

// The number is represented by a single dword
if (len == 1 && unchecked((int)(value[0])) > 0)
{
if (value[0] == 1 /* abs(-1) */)
{
this = s_bnMinusOneInt;
}
else if (value[0] == kuMaskHighBit /* abs(Int32.MinValue) */)
{
this = s_bnMinInt;
_sign = -1;
}
else
{
_sign = (-1) * ((int)value[0]);
_bits = null;
_sign = +1;
}
}
else
{
_sign = -1;
_bits = value.Slice(0, len).ToArray();
_bits = value.ToArray();
}
AssertValid();
return;
}

public static BigInteger Zero { get { return s_bnZeroInt; } }
Expand All @@ -616,8 +626,6 @@ private BigInteger(Span<uint> value)

public static BigInteger MinusOne { get { return s_bnMinusOneInt; } }

internal static int MaxLength => Array.MaxLength / sizeof(uint);

public bool IsPowerOfTwo
{
get
Expand Down
20 changes: 11 additions & 9 deletions src/libraries/System.Runtime.Numerics/tests/BigInteger/op_rightshift.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,21 @@ public static void BigShiftsTest()
public static void LargeNegativeBigIntegerShiftTest()
{
// Create a very large negative BigInteger
BigInteger bigInt = new BigInteger(-1) << int.MaxValue;
Assert.Equal(2147483647, bigInt.GetBitLength());
Copy link
Copy Markdown
Member

@stephentoub stephentoub May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this become a test that validates an exception is thrown?

Copy link
Copy Markdown
Member Author

@tannergooding tannergooding May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially. There's some general cleanup and additional validation I'd like to do here long term, but I'll get this back up to validate it throws in a follow up PR.

int bitsPerElement = 8 * sizeof(uint);
int maxBitLength = ((Array.MaxLength / bitsPerElement) * bitsPerElement);
BigInteger bigInt = new BigInteger(-1) << (maxBitLength - 1);
Assert.Equal(maxBitLength - 1, bigInt.GetBitLength());
Assert.Equal(-1, bigInt.Sign);

// Validate internal representation.
// At this point, bigInt should be a 1 followed by int.MaxValue zeros.
// At this point, bigInt should be a 1 followed by maxBitLength - 1 zeros.
// Given this, bigInt._bits is expected to be structured as follows:
// - _bits.Length == (int.MaxValue + 1) / (8 * sizeof(uint))
// - _bits.Length == ceil(maxBitLength / bitsPerElement)
// - First (_bits.Length - 1) elements: 0x00000000
// - Last element: 0x80000000
// ^------ (There's the leading '1')

Assert.Equal(((uint)int.MaxValue + 1) / (8 * sizeof(uint)), (uint)bigInt._bits.Length);
Assert.Equal((maxBitLength + (bitsPerElement - 1)) / bitsPerElement, bigInt._bits.Length);

uint i = 0;
for (; i < (bigInt._bits.Length - 1); i++) {
Expand All @@ -52,18 +54,18 @@ public static void LargeNegativeBigIntegerShiftTest()

// Right shift the BigInteger
BigInteger shiftedBigInt = bigInt >> 1;
Assert.Equal(2147483646, shiftedBigInt.GetBitLength());
Assert.Equal(maxBitLength - 2, shiftedBigInt.GetBitLength());
Assert.Equal(-1, shiftedBigInt.Sign);

// Validate internal representation.
// At this point, shiftedBigInt should be a 1 followed by int.MaxValue - 1 zeros.
// At this point, shiftedBigInt should be a 1 followed by maxBitLength - 2 zeros.
// Given this, shiftedBigInt._bits is expected to be structured as follows:
// - _bits.Length == (int.MaxValue + 1) / (8 * sizeof(uint))
// - _bits.Length == ceil((maxBitLength - 1) / bitsPerElement)
// - First (_bits.Length - 1) elements: 0x00000000
// - Last element: 0x40000000
// ^------ (the '1' is now one position to the right)

Assert.Equal(((uint)int.MaxValue + 1) / (8 * sizeof(uint)), (uint)shiftedBigInt._bits.Length);
Assert.Equal(((maxBitLength - 1) + (bitsPerElement - 1)) / bitsPerElement, shiftedBigInt._bits.Length);

i = 0;
for (; i < (shiftedBigInt._bits.Length - 1); i++) {
Expand Down