Skip to content

[Draft] Limit length of System.Runtime.BigInteger to int.MaxValue-32 bits - #84901

Closed
speshuric wants to merge 2 commits into
dotnet:mainfrom
speshuric:dotnet84670
Closed

[Draft] Limit length of System.Runtime.BigInteger to int.MaxValue-32 bits#84901
speshuric wants to merge 2 commits into
dotnet:mainfrom
speshuric:dotnet84670

Conversation

@speshuric

Copy link
Copy Markdown
Contributor

See discussion in #84670

  • Limit BigInteger.MaxLength to int.MaxValue/32 uints. This is equal to limit length in bits to int.MaxValue-32 bits
  • Fix ctor BigInteger(ReadOnlySpan<byte>, bool, bool) to check MaxLength limit
  • Fix private ctor BigInteger(ReadOnlySpan<uint>, bool) to check MaxLength limit after trying to conserve space, not before
  • Fix private ctor BigInteger(Span<uint>) to check MaxLength limit after trying to conserve space, not before
  • Fix obsolete comment in AssertValid()
  • Fix test LargeNegativeBigIntegerShiftTest to respect new limit.

fix#84670

- Limit `BigInteger.MaxLength` to `int.MaxValue/32` uints. This is equal
to limit length in bits to `int.MaxValue-32 bits`
- Fix ctor `BigInteger(ReadOnlySpan<byte>, bool, bool)` to check MaxLength
limit
- Fix private ctor `BigInteger(ReadOnlySpan<uint>, bool)` to check
`MaxLength` limit after trying to conserve space, not before
- Fix private ctor `BigInteger(Span<uint>)` to check MaxLength limit after
trying to conserve space, not before
- Fix obsolete comment in AssertValid()
- Fix test `LargeNegativeBigIntegerShiftTest` to respect new limit.
fix#84670
@ghostghost added community-contribution Indicates that the PR has been added by a community member area-System.Numerics labels Apr 16, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

See discussion in #84670

  • Limit BigInteger.MaxLength to int.MaxValue/32 uints. This is equal to limit length in bits to int.MaxValue-32 bits
  • Fix ctor BigInteger(ReadOnlySpan<byte>, bool, bool) to check MaxLength limit
  • Fix private ctor BigInteger(ReadOnlySpan<uint>, bool) to check MaxLength limit after trying to conserve space, not before
  • Fix private ctor BigInteger(Span<uint>) to check MaxLength limit after trying to conserve space, not before
  • Fix obsolete comment in AssertValid()
  • Fix test LargeNegativeBigIntegerShiftTest to respect new limit.

fix #84670

Author:speshuric
Assignees:-
Labels:

area-System.Numerics, community-contribution

Milestone:-

@huoyaoyuan

Copy link
Copy Markdown
Member

The overflow guard in DebuggerDisplay can also be updated.

@speshuric

Copy link
Copy Markdown
ContributorAuthor

The overflow guard in DebuggerDisplay can also be updated.

Thank you for point that! I'll check it.

@tannergooding

Copy link
Copy Markdown
Member

Is this still being worked on? I'd ideally like to get this one merged prior to #84733 or #83951

nit: Keep the braces please
Co-authored-by: Tanner Gooding <tagoo@outlook.com>
@speshuric

speshuric commented May 16, 2023

Copy link
Copy Markdown
ContributorAuthor

Is this still being worked on? I'd ideally like to get this one merged prior to #84733 or #83951

Sorry, I have been busy a bit. I'll commit the rest of changes this week.

@adamsitnik

adamsitnik commented Jul 7, 2023

Copy link
Copy Markdown
Member

I'll commit the rest of changes this week.

Thank you for your help, there is absolutely no rush from our side.

I am going to apply "need author action" label so this PR is not listed on our internal list of PRs awaiting for review.

@adamsitnikadamsitnik added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 7, 2023
@ghost

Copy link
Copy Markdown

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.

@speshuric

Copy link
Copy Markdown
ContributorAuthor

I still plan to complete this.

@ghostghost removed needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity labels Aug 4, 2023
@adamsitnikadamsitnik added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 7, 2023
@adamsitnikadamsitnik added this to the Future milestone Aug 7, 2023
@ghost

Copy link
Copy Markdown

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

ghost commented Sep 4, 2023

Copy link
Copy Markdown

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.

@ghostghost closed this Sep 4, 2023
@ghostghost locked as resolved and limited conversation to collaborators Oct 4, 2023
This pull request was closed.
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Numericscommunity-contributionIndicates that the PR has been added by a community memberneeds-author-actionAn issue or pull request that requires more info or actions from the author.no-recent-activity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IBinaryInteger<BigInteger>.GetShortestBitLength() - int overflow is not handled

4 participants

@speshuric@huoyaoyuan@tannergooding@adamsitnik