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/8.0-staging] UInt64.CreateSaturating<Int128> truncates instead of saturates #97047

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jan 16, 2024

Backport of #96369 to release/8.0-staging

/cc @tannergooding @pedrobsaila

Customer Impact

As reported by a customer, attempting to use generic math to convert an Int128 to a UInt64 using saturation may see incorrect results. This conversion uses truncation behavior instead of saturation. For example, ulong.CreateSaturating((Int128)ulong.MaxValue + (Int128)10L)) returns 9 instead of ulong.MaxValue (18446744073709551615).

The customer found and illustrated scenarios/types where the correct behavior exists, comparing this to types where the incorrect behavior is present. This bug results in unexpected arithmetic results, which can cause data correctness issues in applications. More details can be found in #94523, where this was reported.

Regression

No. The bug exists back to .NET 7.

Testing

Additional tests covering the edge case were added.

Risk

Low. The general pattern is already established and this was using the wrong one (conversion to a smaller type use one pattern, conversion to a larger type use a different pattern), likely due to a copy/paste error when covering the full set of integral types that needed the code.

@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 Jan 16, 2024
@tannergooding tannergooding added Servicing-consider Issue for next servicing release review area-System.Numerics and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jan 16, 2024
@ghost
Copy link

ghost commented Jan 16, 2024

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

Issue Details

Backport of #96369 to release/8.0-staging

/cc @tannergooding @pedrobsaila

Customer Impact

Developers attempting to use generic math to convert an Int128 to a UInt64 using saturation may see incorrect results.

Testing

Additional tests covering the edge case was added.

Risk

Low. The general pattern is already established and this was using the wrong one (conversion to a smaller type use one pattern, conversion to a larger type use a different pattern), likely due to a copy/paste error when covering the full set of integral types that needed the code.

Author: github-actions[bot]
Assignees: -
Labels:

Servicing-consider, area-System.Numerics

Milestone: -

@carlossanlop
Copy link
Member

carlossanlop commented Jan 17, 2024

FYI - This will have to wait until the March release. Code Complete for the February Release is today and I'm already starting the flow to internal.

@jeffhandley
Copy link
Member

I missed you by a few minutes, @carlossanlop -- but understood. Thanks for the heads-up.

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 17, 2024
@carlossanlop
Copy link
Member

Approved by Tactics via email. We can get this into Feb.

@carlossanlop carlossanlop merged commit 6d35e39 into release/8.0-staging Jan 17, 2024
174 of 180 checks passed
@carlossanlop carlossanlop deleted the backport/pr-96369-to-release/8.0-staging branch January 17, 2024 01:15
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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