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] Fix BigInteger.Rotate{Left,Right} for backport #112991

Open
wants to merge 5 commits into
base: release/9.0-staging
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 27, 2025

Backport of #112878 and #112879 to release/9.0

/cc @tannergooding @kzrnm

Customer Impact

  • Customer reported
  • Found internally

This was customer reported via #112564. The customer contributed the fix and validated it for their scenarios.

Shifting and bit-rotation have defined behaviors for fixed sized integers and while the behaviors are harder to map to the concept of a BigInteger which can have an arbitrary number of bits, there is still an expected behavior that maps to those semantics. In particular it's expected to behave similarly to as if the BigInteger was represented as the shortest two's complement representation rounded up to the nearest multiple of 32-bits.

Customers were getting incorrect results for certain inputs due to the internal representation of BigInteger causing the sign bit to be lost after the value was converted to its two's complement representation.

Regression

  • Yes
  • No

The addition of unsigned right shift and bitwise rotatation APIs is recent to .NET and was done as part of the push to support Generic Math.

Testing

Some comprehensive tests were added that test the various edge case bit patterns that could be prone to triggering such issues.

Risk

Medium

The APIs impacted in BigInteger are relatively new (.NET 7) and so the risk of negatively impacting existing customers is relatively low. The largest risk effectively being that some customer is accidentally depending on the broken behavior without realizing it.

However, these breaks are also additional examples of some of the problems caused by the current internal representation of BigInteger (which has been that way since it was introduced in 2010). The current representation is effectively that it is "ones complement + sign" and while that can make some basic arithmetic operations (such as addition, subtraction, multiplication, and division) simpler, it makes many other operations more complex or slower than necessary (particularly where they involve considering the bits of the two's complement representation). This in turn means that the confidence bar in such changes tends to be lower and needs more comprehensive testing (as was added in this fix).

The required confidence bar that the fix is correct has been met here, but the representation and additional testing nuance for edge cases make all such changes riskier, hence the categorization as medium.

Copy link
Contributor

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

* Add tests for the shift operator of BigInteger

* Fix the unsigned right shift operator of BigInteger

* avoid stackalloc

* external sign element
@tannergooding tannergooding changed the base branch from release/9.0 to release/9.0-staging February 27, 2025 18:11
@tannergooding
Copy link
Member

CC. @jeffhandley, @artl93

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Numerics Servicing-consider Issue for next servicing release review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants