Fix BigInteger.Parse returns bogus value for exponents above 1000

[Maximys]

Fixes issue 17296

[ghost]

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixing invalid behaviour of BigInteger.Parse and BigInteger.ToString.

Author:	Maximys
Assignees:	-
Labels:	area-System.Numerics
Milestone:	-

[jeffhandley]

@pgovind This PR is assigned to you for follow-up/decision before the RC1 snap.

[pgovind]

Thanks for the contribution here @Maximys. Due to it being so late in the cycle, this is going to miss out on the .NET 6 release. We'll instead be able to review and merge it after the RC1 snap when we begin accepting more broad changes into main again (ETA: 3-4 weeks)

[Maximys]

@pgovind , ok, thank you so much, I'll wait

[jeffhandley]

@Maximys We're still trying to finish up a few things that are part of .NET 6.0 RC2. It'll be another week or two before we can review this thoroughly. I've set the milestone to 7.0.0 to indicate our intention of having this in that release though. Thanks for your contribution and patience!

[Maximys]

@pgovind , I had just commit fixes by review. Can you check it?

[tannergooding]

Is there a smaller fix which only addresses the bogus values for exponents above 1000 issue without also refactoring all the parsing logic simultaneously?

This feels like it should be two separate PRs.

[Maximys]

@tannergooding , I had just remove additional commits and changes from my branch.

[jeffhandley]

Thanks for minimizing the changes, @Maximys.

@tannergooding - how does this look to you now? I honestly don't understand what the previous logic was trying to accomplish.

[tannergooding]

Sorry for the delay in review here.

This isn't quite the right fix and likely introduces a different bug. For example, if you have an exponent of 21474836492 it will succesfully parse the first 9 digits giving 214748364. Multiply by 10 gives 2147483640, add 9 overflows to -2147483647. Then further multiplied by 10, gives exactly 10, adding 2 gives 12.

Both of these last two digits are problematic because of this and so we need something that accounts for overflow here and surfaces it as invalid input/failure to parse.

[Maximys]

@tannergooding , I agree with you in this case. I have idea about processing this cases

[Maximys]

@tannergooding , I had just repeat my summer investigation about algorithms of BigInteger, and I remember, that there is BigNumber class with BigNumberBuffer inside it and this buffer contains scale with int type.
BigNumber provide some functionality to BigInteger and if int type with it's MaxValue is invalid, then .NET Command should create separate task for change it.
But firstly, lets make little calculation:
itemSize * arraySize = 32bit * 2147483647 = 68719476704 bit = 8589934588 byte ≈ 8388608 kbyte = 8192 mb = 8 gb
thus, theoretically we can provide 8 Gb for store one BigInteger by current architecture! I think, this is more than enough

[tannergooding]

I think we can simply make this be:

int exp = 0;
do
{
    exp = exp * 10 + (ch - '0');
    ch = ++p < strEnd ? *p : '\0';
} while ((ch >= '0') && (ch <= '9') && (exp < (int.MaxValue / 10))); // exp < 214748364

if ((ch >= '0') || (ch <= '9'))
{
    // We still had remaining characters but bailed early because
    // the exponent was going to overflow. If exp is exactly 214748364
    // then we can technically handle one more character being 0-7
    // but the additional complexity might not be worthwhile.

    Debug.Assert(exp >= 214748364);
    return false;
}

[tannergooding]

The point is to account for overflow and ensure its handled so we can throw for Parse and return false for TryParse.

This trivially handles it by accounting for both cases of overflow:

1. due to exp * 10 which can only overflow for exp > (int.MaxValue / 10)
2. due to the subsequent add which can only overflow for exp == (int.MaxValue / 10) and ch > '7'

[tannergooding]

Suggested change

	// then we can technically handle one more character being 0-9
	// then we can technically handle one more character being 0-7

2147483647 is int.MaxValue so the last digit, if we handled it, could only be 0-7

[tannergooding]

There should also be a test covering 2147483639, 2147483647, and a couple arbitrary strings bigger than that (such as 21474836492).

The latter two should end up returning false for TryParse and throwing an OverflowException to parity what int.TryParse does.
The first case will probably result in an OutOfMemoryException but otherwise should be parsed as a valid set of integer digits.

[tannergooding]

The latter two should end up returning false for TryParse and throwing an OverflowException to parity what int.TryParse does.

Actually after thinking it over and talking with @bartonjs and @GrabYourPitchforks, I think that we should always throw PlatformNotSupportedException in this case as well. It is more consistent and allows us to expand in the future without an "additional" breaking change and avoids false being returned for a "technically valid" big integer that otherwise we simply don't support.

[tannergooding]

@Maximys are you still working on this?

No particular rush, just wanted to ensure that was the case since many people took a break over the holidays. There are still several pending comments above that would need to be resolved.

[ghost]

This pull request has been automatically marked no recent activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no recent activity.

[ghost]

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

[dakersnar]

@tannergooding Should I try to take this over and get it across the finish line? Do we still want this for 7.0?

[tannergooding]

It's definitely something worth fixing for early .NET 8 at the very least. We can always bar check for .NET 7

[dakersnar]

Sounds good, I'm going to cherry pick these commits and make a new PR that includes the requested tweaks.