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

Fix overflow check for parsing format strings #72647

Merged
merged 5 commits into from
Aug 9, 2022

Conversation

dakersnar
Copy link
Contributor

Fixes #67891

Limiting the precision to int.MaxValue ended up causing issues, so we are going to limit to 999_999_999 (9 digits) instead. As discussed in the above issue, that should be more than enough precision for every type we support and plan to support.

Do we need to update this article, or any other documentation? https://docs.microsoft.com/en-us/dotnet/standard/base-types/standard-numeric-format-strings

@ghost
Copy link

ghost commented Jul 21, 2022

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

Issue Details

Fixes #67891

Limiting the precision to int.MaxValue ended up causing issues, so we are going to limit to 999_999_999 (9 digits) instead. As discussed in the above issue, that should be more than enough precision for every type we support and plan to support.

Do we need to update this article, or any other documentation? https://docs.microsoft.com/en-us/dotnet/standard/base-types/standard-numeric-format-strings

Author: dakersnar
Assignees: -
Labels:

area-System.Numerics

Milestone: -

@stephentoub
Copy link
Member

Do we need to update this article, or any other documentation?

Yes, we should submit PRs to update the docs appropriately.

@stephentoub
Copy link
Member

Does

// Fallback for symbol and any length digits. The digits value must be >= 0 && <= 99,
// but it can begin with any number of 0s, and thus we may need to check more than two
// digits. Further, for compat, we need to stop when we hit a null char.
int n = 0;
int i = 1;
while ((uint)i < (uint)format.Length && char.IsAsciiDigit(format[i]))
{
int temp = (n * 10) + format[i++] - '0';
if (temp < n)
{
throw new FormatException(SR.Argument_BadFormatSpecifier);
}
n = temp;
}
need to be updated, too?

@dakersnar
Copy link
Contributor Author

@stephentoub yes, and probably this too https://source.dot.net/#System.Runtime.Numerics/System/Numerics/BigNumber.cs,858. I'll bundle those in.

@dakersnar dakersnar added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jul 26, 2022
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 26, 2022
@ghost
Copy link

ghost commented Jul 26, 2022

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@dakersnar dakersnar removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 27, 2022
@dakersnar dakersnar merged commit 22ba36d into dotnet:main Aug 9, 2022
@dakersnar dakersnar deleted the fix-67891 branch August 9, 2022 22:13
@ghost ghost locked as resolved and limited conversation to collaborators Sep 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Numerics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsing format string precision intends to validate for overflow but the check does not work
3 participants