From 3df1c27eb48fda9b746a5029bc7e28d1dd568d26 Mon Sep 17 00:00:00 2001 From: marcnet80 <46241368+marcnet80@users.noreply.github.com> Date: Sat, 14 Sep 2019 22:36:07 +0300 Subject: [PATCH] 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 --- .../Collections/Concurrent/PartitionerStatic.cs | 28 +++++++++++----------- .../tests/IntRangePartitionerTests.cs | 24 +++++++++++++++++++ .../tests/LongRangePartitionerTests.cs | 24 +++++++++++++++++++ 3 files changed, 62 insertions(+), 14 deletions(-) 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 --- 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 /// 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; + /// /// Creates an orderable partitioner from an /// instance. @@ -181,16 +186,13 @@ namespace System.Collections.Concurrent /// A partitioner. /// The argument is /// less than or equal to the argument. + /// if ProccessorCount == 1, for correct rangeSize calculation the const CoreOversubscriptionRate must be > 1 (avoid division by 1) public static OrderablePartitioner> 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 /// A partitioner. /// The argument is /// less than or equal to the argument. + /// 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. public static OrderablePartitioner> 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 --- 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); } + + /// + /// Ensure that the range partitioner doesn't exceed the exclusive bound + /// + /// + /// + [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> 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 --- 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); } + + /// + /// Ensure that the range partitioner doesn't exceed the exclusive bound + /// + /// + /// + [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> 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); + } } } -- 2.7.4