Possible overflow in Partitioner dotnet/corefx#40201 (dotnet/corefx#41066)

* Possible overflow in Partitioner dotnet/corefx#40201

https://github.com/dotnet/corefx/issues/40201

* Possible overflow in Partitioner dotnet/corefx#40201

https://github.com/dotnet/corefx/issues/40201

* Possible overflow in Partitioner dotnet/corefx#40201

https://github.com/dotnet/corefx/issues/40201

* Possible overflow in Partitioner dotnet/corefx#40201

https://github.com/dotnet/corefx/issues/40201

* Possible overflow in Partitioner dotnet/corefx#40201

https://github.com/dotnet/corefx/issues/40201

Commit migrated from https://github.com/dotnet/corefx/commit/36370ae660d385deef9c2dc25f72852bc4b9194e

diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/PartitionerStatic.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/PartitionerStatic.cs
index ef1095e..76c3304 100644 (file)
--- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/PartitionerStatic.cs
+++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/PartitionerStatic.cs
@@ -68,6 +68,11 @@ namespace System.Collections.Concurrent
     /// </remarks>
     public static class Partitioner
     {
+        // How many chunks do we want to divide the range into?  If this is 1, then the
+        // answer is "one chunk per core".  Generally, though, you'll achieve better
+        // load balancing on a busy system if you make it higher than 1.
+        private const int CoreOversubscriptionRate = 3;
+
         /// <summary>
         /// Creates an orderable partitioner from an <see cref="System.Collections.Generic.IList{T}"/>
         /// instance.
@@ -181,16 +186,13 @@ namespace System.Collections.Concurrent
         /// <returns>A partitioner.</returns>
         /// <exception cref="System.ArgumentOutOfRangeException"> The <paramref name="toExclusive"/> argument is
         /// less than or equal to the <paramref name="fromInclusive"/> argument.</exception>
+        /// <remarks>if ProccessorCount == 1, for correct rangeSize calculation the const CoreOversubscriptionRate must be > 1 (avoid division by 1)</remarks>
         public static OrderablePartitioner<Tuple<long, long>> Create(long fromInclusive, long toExclusive)
         {
-            // How many chunks do we want to divide the range into?  If this is 1, then the
-            // answer is "one chunk per core".  Generally, though, you'll achieve better
-            // load balancing on a busy system if you make it higher than 1.
-            int coreOversubscriptionRate = 3;
-
             if (toExclusive <= fromInclusive) throw new ArgumentOutOfRangeException(nameof(toExclusive));
-            long rangeSize = (toExclusive - fromInclusive) /
-                (PlatformHelper.ProcessorCount * coreOversubscriptionRate);
+            decimal range = (decimal)toExclusive - fromInclusive;
+            long rangeSize = (long)(range /
+                (PlatformHelper.ProcessorCount * CoreOversubscriptionRate));
             if (rangeSize == 0) rangeSize = 1;
             return Partitioner.Create(CreateRanges(fromInclusive, toExclusive, rangeSize), EnumerablePartitionerOptions.NoBuffering); // chunk one range at a time
         }
@@ -238,16 +240,14 @@ namespace System.Collections.Concurrent
         /// <returns>A partitioner.</returns>
         /// <exception cref="System.ArgumentOutOfRangeException"> The <paramref name="toExclusive"/> argument is
         /// less than or equal to the <paramref name="fromInclusive"/> argument.</exception>
+        /// <remarks>if ProccessorCount == 1, for correct rangeSize calculation the const CoreOversubscriptionRate must be > 1 (avoid division by 1),
+        /// and the same issue could occur with rangeSize == -1 when fromInclusive = int.MinValue and toExclusive = int.MaxValue.</remarks>
         public static OrderablePartitioner<Tuple<int, int>> Create(int fromInclusive, int toExclusive)
         {
-            // How many chunks do we want to divide the range into?  If this is 1, then the
-            // answer is "one chunk per core".  Generally, though, you'll achieve better
-            // load balancing on a busy system if you make it higher than 1.
-            int coreOversubscriptionRate = 3;
-
             if (toExclusive <= fromInclusive) throw new ArgumentOutOfRangeException(nameof(toExclusive));
-            int rangeSize = (toExclusive - fromInclusive) /
-                (PlatformHelper.ProcessorCount * coreOversubscriptionRate);
+            long range = (long)toExclusive - fromInclusive;
+            int rangeSize = (int)(range /
+                (PlatformHelper.ProcessorCount * CoreOversubscriptionRate));
             if (rangeSize == 0) rangeSize = 1;
             return Partitioner.Create(CreateRanges(fromInclusive, toExclusive, rangeSize), EnumerablePartitionerOptions.NoBuffering); // chunk one range at a time
         }

diff --git a/src/libraries/System.Collections.Concurrent/tests/IntRangePartitionerTests.cs b/src/libraries/System.Collections.Concurrent/tests/IntRangePartitionerTests.cs
index b5b462a..3680f2f 100644 (file)
--- a/src/libraries/System.Collections.Concurrent/tests/IntRangePartitionerTests.cs
+++ b/src/libraries/System.Collections.Concurrent/tests/IntRangePartitionerTests.cs
@@ -557,5 +557,29 @@ namespace System.Collections.Concurrent.Tests
             // Verifying that all items are there
             Assert.Equal(count, actualCount);
         }
+        
+        /// <summary>
+        /// Ensure that the range partitioner doesn't exceed the exclusive bound
+        /// </summary>
+        /// <param name="fromInclusive"></param>
+        /// <param name="toExclusive"></param>
+        [Theory]
+        [InlineData(-1, int.MaxValue)]
+        [InlineData(int.MinValue, int.MaxValue)]
+        [InlineData(int.MinValue, -1)]
+        [InlineData(int.MinValue / 2, int.MaxValue / 2)]
+        public void TestPartitionerCreate(int fromInclusive, int toExclusive)
+        {
+            OrderablePartitioner<Tuple<int, int>> op = Partitioner.Create(fromInclusive, toExclusive);
+            int start = fromInclusive;
+
+            foreach (var p in op.GetDynamicPartitions())
+            {
+                Assert.Equal(start, p.Item1);
+                start = p.Item2;
+            }
+
+            Assert.Equal(toExclusive, start);
+        }
     }
 }

diff --git a/src/libraries/System.Collections.Concurrent/tests/LongRangePartitionerTests.cs b/src/libraries/System.Collections.Concurrent/tests/LongRangePartitionerTests.cs
index f5d5545..98b4ecc 100644 (file)
--- a/src/libraries/System.Collections.Concurrent/tests/LongRangePartitionerTests.cs
+++ b/src/libraries/System.Collections.Concurrent/tests/LongRangePartitionerTests.cs
@@ -552,5 +552,29 @@ namespace System.Collections.Concurrent.Tests
             // Verifying that all items are there
             Assert.Equal(count, actualCount);
         }
+        
+        /// <summary>
+        /// Ensure that the range partitioner doesn't exceed the exclusive bound
+        /// </summary>
+        /// <param name="fromInclusive"></param>
+        /// <param name="toExclusive"></param>
+        [Theory]
+        [InlineData(-1, long.MaxValue)]
+        [InlineData(long.MinValue, long.MaxValue)]
+        [InlineData(long.MinValue, -1)]
+        [InlineData(long.MinValue / 2, long.MaxValue / 2)]
+        public void TestPartitionerCreate(long fromInclusive, long toExclusive)
+        {
+            OrderablePartitioner<Tuple<long, long>> op = Partitioner.Create(fromInclusive, toExclusive);
+            long start = fromInclusive;
+
+            foreach (var p in op.GetDynamicPartitions())
+            {
+                Assert.Equal(start, p.Item1);
+                start = p.Item2;
+            }
+
+            Assert.Equal(toExclusive, start);
+        }
     }
 }