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

[release/9.0] Ensure that integer parsing correctly handles non-zero fractional data #106700

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 20, 2024

Backport of #106506 to release/9.0

/cc @tannergooding

Customer Impact

  • Customer reported
  • Found internally

Reported via #94971

Consumers of these parsing APIs for any built-in integer type and using NumberStyles.Number may see invalid inputs parsed without exception when they should have instead thrown an OverflowException or returned false from, the TryParse methods.

  • The built in Integer types are byte, sbyte, short, ushort, int, uint, long, ulong, nint, nuint, Int128, and UInt128 and all use this one shared generic code path
bool parsed = short.TryParse("1234.000001", NumberStyles.Number, CultureInfo.InvariantCulture, out short number);
Console.Write($"TryParse: {parsed}. Value = {number}. ");

try
{
    short value = short.Parse("1234.000001", NumberStyles.Number);
    Console.WriteLine($"Parse: Value = {value}.");
}
catch (Exception ex)
{
    Console.WriteLine($"Parse: {ex.GetType().Name} thrown.");
}

// .NET 6: TryParse: False. Value = 0. Parse: OverflowException thrown.
// .NET 7: TryParse: False. Value = 0. Parse: OverflowException thrown.
// .NET 8: TryParse: True. Value = 1234. Parse: Value = 1234.
// .NET 9: TryParse: True. Value = 1234. Parse: Value = 1234.
// Fixed:  TryParse: False. Value = 0. Parse: OverflowException thrown.

Because the buffer size and positioning of non-zero trailing digits both have an effect on the broken logic, customers currently experience inconsistent behavior from these APIs for differerent values. Some invalid strings are parsed with success (erroneously) while others are rejected expectedly. With this fix, we return to a consistent behavior regardless of the positioning of the non-zero trailing digits relative to the string length and buffer size for the target type.

Regression

  • Yes
  • No

When using the NumberStyles.Number parsing option, .NET allows for integer types to parse strings that contain fractional data provided that fractional data is all zero. For example, 1.0 is valid, as is 1.00000, but 1.01 is invalid.

In .NET 8, part of the parsing logic was refactored to allow better sharing of the logic across multiple types and so that UTF-8 and UTF-16 could be supported via the same algorithm without needing to duplicate several thousand lines of code. As part of this, an edge case was missed where an input string that had non-zero fractional data would not be reported as an error if said invalid fractional data appeared after the end of the NumberBuffer.

To elaborate, Int32 has a maximum number of 10 significant digit that can be represented, as such we allocate an 11 digit buffer to represent this and the null terminator. We parse the input string and track any significant digits in this buffer. Any significant digits beyond the end of this buffer continue scaling the exponent, allowing us to catch numbers that are "too large". We also track a property HasNonZeroTail that allows us to determine if any fractional data existed that was not 0. Doing this keeps the memory footprint low, while improving performance.

The issue was then that the refactoring failed to check number.HasNonZeroTail for the integer case. What this meant is that we would continue failing if there were fewer than 10 significant digits and non-zero fractional data. For example, 123.4 would fail because that has a total of 4 significant digits. However, 123456789.01 would incorrectly succeed because the first 10 significant digits (123456789.0) were valid, being whole integer and a fractional portion that is zero. The invalid data appeared after the end of the explicitly tracked buffer.

Testing

Explicit tests covering various edge cases for all the built-in integer types were added. This ensures that various inputs that are both within and outside the buffer range, particularly in relation to the respective Min/MaxValue for a given type are covered.

Risk

Medium.

The actual fix here is relatively simple, well understood, and directly impacts a less common code path (parsing using an explicit number style to allow integer inputs to specify fractional data that is 0).

However, this does touch core shared code that most applications in existence use (parsing for the primitive integer types such as int or long) and which has been known to be somewhat prone to user error in the face of globalization due to some cultures using . as the decimal separator and , as the number group separator; while others inverse this and use , as the decimal separator. The fix remains correct and does the right thing in the face of such culture specific differences. However, due to the semi-regular user error around this space it has a higher chance to cause friction when the patch does go out and is worth taking into consideration as part of the risk to benefit analysis.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 20, 2024
@jeffhandley jeffhandley added area-System.Numerics and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 20, 2024
Copy link
Contributor

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

@ericstj ericstj merged commit 7b81dd2 into release/9.0 Sep 16, 2024
149 of 150 checks passed
@jkotas jkotas deleted the backport/pr-106506-to-release/9.0 branch September 18, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Numerics Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants