Skip to content

Commit

Permalink
Fix incorrect results of right shifting huge BigIntegers error report…
Browse files Browse the repository at this point in the history
…ed in Issue #65633 (#67867)

* Fix #65633, multiplication overflow causing incorrect right shift results

* Add unit test reproducing incorrect right shift results for BigInteger (#65633)

* Fix inconsistent formatting

* Expand LargeNegativeBigIntegerShiftTest to validate values, expose private BigInteger members to tests

* Apply suggestions from code review

Co-authored-by: Tanner Gooding <[email protected]>

* Restrict unit test to 64-bit processors

* Document reasoning for restricting the test to 64-bit

Co-authored-by: Dan Moseley <[email protected]>

Co-authored-by: Tanner Gooding <[email protected]>
Co-authored-by: Dan Moseley <[email protected]>
  • Loading branch information
3 people authored Apr 19, 2022
1 parent 400d171 commit f5455d3
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;

[assembly: InternalsVisibleTo("System.Runtime.Numerics.Tests, PublicKey=00240000048000009400000006020000002400005253413100040000010001004b86c4cb78549b34bab61a3b1800e23bfeb5b3ec390074041536a7e3cbd97f5f04cf0f857155a8928eaa29ebfd11cfbbad3ba70efea7bda3226c6a8d370a4cd303f714486b6ebc225985a638471e6ef571cc92a4613c00b8fa65d61ccee0cbe5f36330c9a01f4183559f1bef24cc2917c6d913e3a541333a1d05d9bed22b38cb")]
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
Link="CoreLib\System\Text\ValueStringBuilder.cs" />
<Compile Include="$(CommonPath)System\HexConverter.cs"
Link="Common\System\HexConverter.cs" />
<Compile Include="Properties\InternalsVisibleTo.cs" />
</ItemGroup>
<ItemGroup>
<Reference Include="System.Memory" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2211,7 +2211,7 @@ stackalloc uint[BigIntegerCalculator.StackAllocThreshold]

if (negx)
{
if (shift >= (kcbitUint * xd.Length))
if (shift >= ((long)kcbitUint * xd.Length))
{
result = MinusOne;
goto exit;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,54 @@ public static void BigShiftsTest()
}
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))] // May fail on 32-bit due to a large memory requirement
public static void LargeNegativeBigIntegerShiftTest()
{
// Create a very large negative BigInteger
BigInteger bigInt = new BigInteger(-1) << int.MaxValue;
Assert.Equal(2147483647, bigInt.GetBitLength());
Assert.Equal(-1, bigInt.Sign);

// Validate internal representation.
// At this point, bigInt should be a 1 followed by int.MaxValue zeros.
// Given this, bigInt._bits is expected to be structured as follows:
// - _bits.Length == (int.MaxValue + 1) / (8 * sizeof(uint))
// - First (_bits.Length - 1) elements: 0x00000000
// - Last element: 0x80000000
// ^------ (There's the leading '1')

Assert.Equal(((uint)int.MaxValue + 1) / (8 * sizeof(uint)), (uint)bigInt._bits.Length);

uint i = 0;
for (; i < (bigInt._bits.Length - 1); i++) {
Assert.Equal(0x00000000u, bigInt._bits[i]);
}

Assert.Equal(0x80000000u, bigInt._bits[i]);

// Right shift the BigInteger
BigInteger shiftedBigInt = bigInt >> 1;
Assert.Equal(2147483646, shiftedBigInt.GetBitLength());
Assert.Equal(-1, shiftedBigInt.Sign);

// Validate internal representation.
// At this point, shiftedBigInt should be a 1 followed by int.MaxValue - 1 zeros.
// Given this, shiftedBigInt._bits is expected to be structured as follows:
// - _bits.Length == (int.MaxValue + 1) / (8 * sizeof(uint))
// - First (_bits.Length - 1) elements: 0x00000000
// - Last element: 0x40000000
// ^------ (the '1' is now one position to the right)

Assert.Equal(((uint)int.MaxValue + 1) / (8 * sizeof(uint)), (uint)shiftedBigInt._bits.Length);

i = 0;
for (; i < (shiftedBigInt._bits.Length - 1); i++) {
Assert.Equal(0x00000000u, shiftedBigInt._bits[i]);
}

Assert.Equal(0x40000000u, shiftedBigInt._bits[i]);
}

[Fact]
public static void RunRightShiftTests()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,8 @@
<Compile Include="BigInteger\TryWriteBytes.cs" />
<Compile Include="ComplexTests.cs" />
</ItemGroup>
<ItemGroup>
<!-- Some internal types are needed, so we reference the implementation assembly, rather than the reference assembly. -->
<ProjectReference Include="..\src\System.Runtime.Numerics.csproj" SkipUseReferenceAssembly="true" />
</ItemGroup>
</Project>

0 comments on commit f5455d3

Please sign in to comment.