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.

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

Jump to bottom

Fix BigInteger.Parse returning incorrect values for exponents above 1000 #73643

Merged
merged 8 commits into from
Aug 12, 2022
Merged

Fix BigInteger.Parse returning incorrect values for exponents above 1000 #73643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3de6672
Fixing problem with invalid parse scientific form of numbers. (#17296)
Maximys Jul 9, 2021
614fbce
Preventing OverflowException when parsing scientific form of numbers.…
Maximys Nov 6, 2021
318fa97
Suggestions from review
dakersnar Aug 9, 2022
9401318
Added bigger test cases
dakersnar Aug 9, 2022
f5a15da
Merge branch 'main' into fix-17296
dakersnar Aug 9, 2022
4264827
Adjusted cutoff to 9 digits
dakersnar Aug 12, 2022
16c134b
Merge branch 'fix-17296' of https://github.com/dakersnar/runtime into…
dakersnar Aug 12, 2022
0221eec
Accidental commit
dakersnar Aug 12, 2022
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
17 changes: 13 additions & 4 deletions src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,17 +515,26 @@ private static unsafe bool ParseNumber(ref char* str, char* strEnd, NumberStyles
int exp = 0;
do
{
exp = exp * 10 + (ch - '0');
ch = ++p < strEnd ? *p : '\0';
if (exp > 1000)
// Check if we are about to overflow past our limit of 9 digits
if (exp >= 100_000_000)
{
exp = 9999;
// Set exp to Int.MaxValue to signify the requested exponent is too large. This will lead to an OverflowException later.
exp = int.MaxValue;
number.scale = 0;
Comment on lines +521 to +523
Copy link
Contributor Author

@dakersnar dakersnar Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@tannergooding negative exponents already had another in-use code path, so we have to use int.MaxValue as our Overflow signifier instead of -1. Additonally, number.scale has to be set to 0 because sometimes it's non-zero at this point and the addition on line 533 will overflow.


// Finish parsing the number, a FormatException could still occur later on.
while (char.IsAsciiDigit(ch))
{
ch = ++p < strEnd ? *p : '\0';
}
break;
}

exp = exp * 10 + (ch - '0');
ch = ++p < strEnd ? *p : '\0';

} while (char.IsAsciiDigit(ch));

if (negExp)
{
exp = -exp;
Expand Down
4 changes: 2 additions & 2 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 @@ -684,7 +684,7 @@ public static bool TryParse([NotNullWhen(true)] string? value, out BigInteger re

public static bool TryParse([NotNullWhen(true)] string? value, NumberStyles style, IFormatProvider? provider, out BigInteger result)
{
return BigNumber.TryParseBigInteger(value, style, NumberFormatInfo.GetInstance(provider), out result);
return BigNumber.TryParseBigInteger(value, style, NumberFormatInfo.GetInstance(provider), out result) == BigNumber.ParsingStatus.OK;
}

public static BigInteger Parse(ReadOnlySpan<char> value, NumberStyles style = NumberStyles.Integer, IFormatProvider? provider = null)
Expand All @@ -699,7 +699,7 @@ public static bool TryParse(ReadOnlySpan<char> value, out BigInteger result)

public static bool TryParse(ReadOnlySpan<char> value, NumberStyles style, IFormatProvider? provider, out BigInteger result)
{
return BigNumber.TryParseBigInteger(value, style, NumberFormatInfo.GetInstance(provider), out result);
return BigNumber.TryParseBigInteger(value, style, NumberFormatInfo.GetInstance(provider), out result) == BigNumber.ParsingStatus.OK;
}

public static int Compare(BigInteger left, BigInteger right)
Expand Down
60 changes: 43 additions & 17 deletions src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,22 @@ internal static class BigNumber
| NumberStyles.AllowCurrencySymbol | NumberStyles.AllowHexSpecifier);

private static readonly uint[] s_uint32PowersOfTen = { 1, 10, 100, 1000, 10000, 100000, 1000000, 10000000, 100000000, 1000000000 };
internal enum ParsingStatus
{
OK,
Failed,
Overflow
}

[DoesNotReturn]
internal static void ThrowOverflowOrFormatException(ParsingStatus status) => throw GetException(status);

private static Exception GetException(ParsingStatus status)
{
return status == ParsingStatus.Failed
? new FormatException(SR.Overflow_ParseBigInteger)
: new OverflowException(SR.Overflow_ParseBigInteger);
}

private struct BigNumberBuffer
{
Expand Down Expand Up @@ -325,18 +341,18 @@ internal static bool TryValidateParseStyleInteger(NumberStyles style, [NotNullWh
return true;
}

internal static bool TryParseBigInteger(string? value, NumberStyles style, NumberFormatInfo info, out BigInteger result)
internal static ParsingStatus TryParseBigInteger(string? value, NumberStyles style, NumberFormatInfo info, out BigInteger result)
{
if (value == null)
{
result = default;
return false;
return ParsingStatus.Failed;
}

return TryParseBigInteger(value.AsSpan(), style, info, out result);
}

internal static bool TryParseBigInteger(ReadOnlySpan<char> value, NumberStyles style, NumberFormatInfo info, out BigInteger result)
internal static ParsingStatus TryParseBigInteger(ReadOnlySpan<char> value, NumberStyles style, NumberFormatInfo info, out BigInteger result)
{
if (!TryValidateParseStyleInteger(style, out ArgumentException? e))
{
Expand All @@ -347,7 +363,7 @@ internal static bool TryParseBigInteger(ReadOnlySpan<char> value, NumberStyles s
if (!FormatProvider.TryStringToBigInteger(value, style, info, bigNumber.digits, out bigNumber.precision, out bigNumber.scale, out bigNumber.sign))
{
result = default;
return false;
return ParsingStatus.Failed;
}

if ((style & NumberStyles.AllowHexSpecifier) != 0)
Expand All @@ -373,19 +389,22 @@ internal static BigInteger ParseBigInteger(ReadOnlySpan<char> value, NumberStyle
{
throw e;
}
if (!TryParseBigInteger(value, style, info, out BigInteger result))

ParsingStatus status = TryParseBigInteger(value, style, info, out BigInteger result);
if (status != ParsingStatus.OK)
{
throw new FormatException(SR.Overflow_ParseBigInteger);
ThrowOverflowOrFormatException(status);
}

return result;
}

private static bool HexNumberToBigInteger(ref BigNumberBuffer number, out BigInteger result)
private static ParsingStatus HexNumberToBigInteger(ref BigNumberBuffer number, out BigInteger result)
{
if (number.digits == null || number.digits.Length == 0)
{
result = default;
return false;
return ParsingStatus.Failed;
}

const int DigitsPerBlock = 8;
Expand Down Expand Up @@ -480,7 +499,7 @@ private static bool HexNumberToBigInteger(ref BigNumberBuffer number, out BigInt
}

result = new BigInteger(sign, bits);
return true;
return ParsingStatus.OK;
}
finally
{
Expand All @@ -499,7 +518,7 @@ private static bool HexNumberToBigInteger(ref BigNumberBuffer number, out BigInt
// a divide-and-conquer algorithm with a running time of O(NlogN).
//
private static int s_naiveThreshold = 20000;
private static bool NumberToBigInteger(ref BigNumberBuffer number, out BigInteger result)
private static ParsingStatus NumberToBigInteger(ref BigNumberBuffer number, out BigInteger result)
{
int currentBufferSize = 0;

Expand All @@ -510,10 +529,17 @@ private static bool NumberToBigInteger(ref BigNumberBuffer number, out BigIntege
const uint TenPowMaxPartial = 1000000000;

int[]? arrayFromPoolForResultBuffer = null;

if (numberScale == int.MaxValue)
{
result = default;
return ParsingStatus.Overflow;
}

if (numberScale < 0)
{
result = default;
return false;
return ParsingStatus.Failed;
}

try
Expand All @@ -535,7 +561,7 @@ private static bool NumberToBigInteger(ref BigNumberBuffer number, out BigIntege
}
}

bool Naive(ref BigNumberBuffer number, out BigInteger result)
ParsingStatus Naive(ref BigNumberBuffer number, out BigInteger result)
{
Span<uint> stackBuffer = stackalloc uint[BigIntegerCalculator.StackAllocThreshold];
Span<uint> currentBuffer = stackBuffer;
Expand All @@ -547,7 +573,7 @@ bool Naive(ref BigNumberBuffer number, out BigInteger result)
if (!ProcessChunk(digitsChunk.Span, ref currentBuffer))
{
result = default;
return false;
return ParsingStatus.Failed;
}
}

Expand All @@ -557,7 +583,7 @@ bool Naive(ref BigNumberBuffer number, out BigInteger result)
}

result = NumberBufferToBigInteger(currentBuffer, number.sign);
return true;
return ParsingStatus.OK;

bool ProcessChunk(ReadOnlySpan<char> chunkDigits, ref Span<uint> currentBuffer)
{
Expand Down Expand Up @@ -619,7 +645,7 @@ bool ProcessChunk(ReadOnlySpan<char> chunkDigits, ref Span<uint> currentBuffer)
}
}

bool DivideAndConquer(ref BigNumberBuffer number, out BigInteger result)
ParsingStatus DivideAndConquer(ref BigNumberBuffer number, out BigInteger result)
{
Span<uint> currentBuffer;
int[]? arrayFromPoolForMultiplier = null;
Expand Down Expand Up @@ -674,7 +700,7 @@ bool DivideAndConquer(ref BigNumberBuffer number, out BigInteger result)
if (digitChar != '0')
{
result = default;
return false;
return ParsingStatus.Failed;
}
}
}
Expand Down Expand Up @@ -776,7 +802,7 @@ bool DivideAndConquer(ref BigNumberBuffer number, out BigInteger result)
ArrayPool<int>.Shared.Return(arrayFromPoolForMultiplier);
}
}
return true;
return ParsingStatus.OK;
}

BigInteger NumberBufferToBigInteger(Span<uint> currentBuffer, bool signa)
Expand Down
36 changes: 36 additions & 0 deletions src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,42 @@ public static void CustomFormatPerMille()
RunCustomFormatToStringTests(s_random, "#\u2030000000", CultureInfo.CurrentCulture.NumberFormat.NegativeSign, 6, PerMilleSymbolFormatter);
}

public static IEnumerable<object[]> RunFormatScientificNotationToBigIntegerAndViceVersaData()
{
yield return new object[] { "1E+1000", "1E+1000" };
yield return new object[] { "1E+1001", "1E+1001" };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tannergooding I was not able to get a test with "1E+2147483639" to work locally, I think it ran for too long.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine, as long as we have some basic tests that values over 1E+1000 work it should be good enough

Testing the extreme edge cases is going to run into lots of issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a couple bigger test cases.

yield return new object[] { "1E+10001", "1E+10001" };
yield return new object[] { "1E+100001", "1E+100001" };
yield return new object[] { "1E+99999", "1E+99999" };
}

[Theory]
[MemberData(nameof(RunFormatScientificNotationToBigIntegerAndViceVersaData))]
public static void RunFormatScientificNotationToBigIntegerAndViceVersa(string testingValue, string expectedResult)
{
BigInteger parsedValue;
string actualResult;

parsedValue = BigInteger.Parse(testingValue, NumberStyles.AllowExponent);
actualResult = parsedValue.ToString("E0");

Assert.Equal(expectedResult, actualResult);
}

public static IEnumerable<object[]> RunFormatScientificNotationToBigIntegerThrowsExceptionData()
{
yield return new object[] { "1E+1000000000" };
yield return new object[] { "1E+2147483647" };
yield return new object[] { "1E+21474836492" };
}

[Theory]
[MemberData(nameof(RunFormatScientificNotationToBigIntegerThrowsExceptionData))]
public static void RunFormatScientificNotationToBigIntegerThrowsException(string testingValue)
{
Assert.Throws<OverflowException>(() => BigInteger.Parse(testingValue, NumberStyles.AllowExponent));
}

[Fact]
public static void ToString_InvalidFormat_ThrowsFormatException()
{
Expand Down