Runtime: Possible overflow in Partitioner.Create(int fromInclusive, int toExclusive)?

Created on 10 Aug 2019 · 11Comments · Source: dotnet/runtime

This issue raised in stackoverflow

```C#
var partitioner = Partitioner.Create(-1, Int32.MaxValue);
var partitions = partitioner.GetPartitions(1);
foreach (var partition in partitions)
{
while (partition.MoveNext())
{
var range = partition.Current;
Console.WriteLine($"Range: {range.Item1,11} => {range.Item2,11}");
}
}


output

Range: -1 => -178956971
Range: -178956971 => -357913941
Range: -357913941 => -536870911
Range: -536870911 => -715827881
Range: -715827881 => -894784851
Range: -894784851 => -1073741821
Range: -1073741821 => -1252698791
Range: -1252698791 => -1431655761
Range: -1431655761 => -1610612731
Range: -1610612731 => -1789569701
Range: -1789569701 => -1968526671
Range: -1968526671 => -2147483641
Range: -2147483641 => 2147483647
```

So looping throw these ranges with for (int i = range.Item1; i < range.Item2; i++) will result in zero loops for all but the last range, which will loop effectively the full range of the Int32 type.
It seems that an integer overflow occurs on the subtraction toExclusive - fromInclusive.

area-System.Collections bug

Source

tarekgh

Most helpful comment

@marcnet80

I want to clarify all current methods which have int parameters should return long:

No, we cannot change the already exposed public methods. What I meant the implementation of these APIs will have a line like

https://github.com/dotnet/corefx/blob/6ff7587b9a0a55d1acd6e137b54c918bb4dd23a9/src/System.Collections.Concurrent/src/System/Collections/Concurrent/PartitionerStatic.cs#L249

which is subtracting 2 int values and causing the overflow issue. If we can try to be smart and make this operation done as long instead of int, this can avoid the overflow and make the API behave as expected in same time. we just want to ensure the ranges are in the expect int ranges.

Call like:

C# Partitioner.Create(-1, Int32.MaxValue);
should work just fine.

tarekgh on 11 Sep 2019

👍3

All 11 comments

Hello, @tarekgh. Yes, you are right, need add checked for subtraction, something like this:

 
int subtraction = 0;
try
{
   subtraction = checked(toExclusive - fromInclusive);
} catch (OverflowException)
{
    throw new OverflowException(nameof(subtraction));
}

Could I take this issue?

marcnet80 on 10 Sep 2019

@marcnet80 feel free to work fixing this issue. I think the fix is not just adding the checked and throw here. I believe we can just change the implementation to use long in the calculation instead of int and make the operation succeed.

tarekgh on 10 Sep 2019

👍1

@tarekgh, I want to clarify all current methods which have int parameters should return long:
https://github.com/dotnet/corefx/blob/master/src/System.Collections.Concurrent/src/System/Collections/Concurrent/PartitionerStatic.cs#L241
https://github.com/dotnet/corefx/blob/master/src/System.Collections.Concurrent/src/System/Collections/Concurrent/PartitionerStatic.cs#L264
Please correct me, if I wrong.

marcnet80 on 11 Sep 2019

@marcnet80