From e1f9d45bd7607dca6765683f5107c7dcf6f785a3 Mon Sep 17 00:00:00 2001 From: Sayantan Mandal Date: Tue, 23 Jun 2026 23:13:25 +0530 Subject: [PATCH] Fix integer overflow in ParallelHelper.For range computation The range size was computed with 'Math.Abs(start - end)' using 32-bit arithmetic. For ranges larger than int.MaxValue this wraps around (silently running single-threaded) or throws OverflowException (Math.Abs(int.MinValue)). Compute the range and batch sizes with 64-bit arithmetic instead. Closes #1188 --- .../Helpers/ParallelHelper.For.IAction.cs | 32 +++++--- .../Helpers/Test_ParallelHelper.For.cs | 82 +++++++++++++++++++ 2 files changed, 102 insertions(+), 12 deletions(-) diff --git a/src/CommunityToolkit.HighPerformance/Helpers/ParallelHelper.For.IAction.cs b/src/CommunityToolkit.HighPerformance/Helpers/ParallelHelper.For.IAction.cs index 4ea95a236..c006ad645 100644 --- a/src/CommunityToolkit.HighPerformance/Helpers/ParallelHelper.For.IAction.cs +++ b/src/CommunityToolkit.HighPerformance/Helpers/ParallelHelper.For.IAction.cs @@ -162,10 +162,15 @@ public static void For(int start, int end, in TAction action, int minim return; } - int count = Math.Abs(start - end); - int maxBatches = 1 + ((count - 1) / minimumActionsPerThread); + // The number of elements in [start, end) can exceed int.MaxValue (for example the + // full [int.MinValue, int.MaxValue) range), so the range size is computed using 64-bit + // arithmetic. Using int here would overflow: 'start - end' could wrap around to a small + // value (silently degrading to a single-threaded run), and 'Math.Abs(int.MinValue)' + // throws OverflowException. The earlier 'start > end' check guarantees count >= 1. + long count = (long)end - start; + long maxBatches = 1 + ((count - 1) / minimumActionsPerThread); int cores = Environment.ProcessorCount; - int numBatches = Math.Min(maxBatches, cores); + int numBatches = (int)Math.Min(maxBatches, (long)cores); // Skip the parallel invocation when a single batch is needed if (numBatches == 1) @@ -178,7 +183,7 @@ public static void For(int start, int end, in TAction action, int minim return; } - int batchSize = 1 + ((count - 1) / numBatches); + long batchSize = 1 + ((count - 1) / numBatches); ActionInvoker actionInvoker = new(start, end, batchSize, action); @@ -196,14 +201,14 @@ private readonly struct ActionInvoker { private readonly int start; private readonly int end; - private readonly int batchSize; + private readonly long batchSize; private readonly TAction action; [MethodImpl(MethodImplOptions.AggressiveInlining)] public ActionInvoker( int start, int end, - int batchSize, + long batchSize, in TAction action) { this.start = start; @@ -218,12 +223,15 @@ public ActionInvoker( /// The index of the batch to process public void Invoke(int i) { - int offset = i * this.batchSize; - int low = this.start + offset; - int high = low + this.batchSize; - int stop = Math.Min(high, this.end); - - for (int j = low; j < stop; j++) + // The batch offset is computed with 64-bit arithmetic so it doesn't overflow for + // large ranges. 'low' and 'stop' are always within [start, end], which fits in an + // int, so the narrowing casts below are safe. + long offset = (long)i * this.batchSize; + long low = this.start + offset; + long high = low + this.batchSize; + int stop = (int)Math.Min(high, (long)this.end); + + for (int j = (int)low; j < stop; j++) { Unsafe.AsRef(in this.action).Invoke(j); } diff --git a/tests/CommunityToolkit.HighPerformance.UnitTests/Helpers/Test_ParallelHelper.For.cs b/tests/CommunityToolkit.HighPerformance.UnitTests/Helpers/Test_ParallelHelper.For.cs index 0bf56560a..3fb08105f 100644 --- a/tests/CommunityToolkit.HighPerformance.UnitTests/Helpers/Test_ParallelHelper.For.cs +++ b/tests/CommunityToolkit.HighPerformance.UnitTests/Helpers/Test_ParallelHelper.For.cs @@ -3,6 +3,8 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Generic; +using System.Linq; using CommunityToolkit.HighPerformance.Helpers; using Microsoft.VisualStudio.TestTools.UnitTesting; using CommunityToolkit.HighPerformance.UnitTests.Buffers.Internals; @@ -73,6 +75,86 @@ public unsafe void Test_ParallelHelper_ForWithRanges() } #endif + [TestMethod] + public void Test_ParallelHelper_ForLargeNegativeRange_DoesNotOverflow() + { + // Regression test for https://github.com/CommunityToolkit/dotnet/issues/1188. + // [int.MinValue, 0) spans 2^31 elements. The range size used to be computed as + // 'Math.Abs(start - end)', and 'Math.Abs(int.MinValue)' throws OverflowException + // before any work runs. The action throws on its first invocation so the loop + // exits immediately instead of iterating two billion times. + Exception? caught = null; + + try + { + ParallelHelper.For(int.MinValue, 0, default(ThrowingAction), 1); + } + catch (Exception e) + { + caught = e; + } + + Assert.IsNotNull(caught, "Expected the action to run and throw."); + + IEnumerable leaves = caught is AggregateException aggregate + ? aggregate.Flatten().InnerExceptions + : new[] { caught }; + + Assert.IsFalse(leaves.Any(static e => e is OverflowException), "The range size overflowed."); + Assert.IsTrue(leaves.Any(static e => e is ThrowingActionException), "The action did not run."); + } + + [TestMethod] + public void Test_ParallelHelper_ForFullRange_IsParallelized() + { + // Regression test for https://github.com/CommunityToolkit/dotnet/issues/1188. + // For [int.MinValue, int.MaxValue) the range size used to overflow to 1 ('start - end' + // wraps around), so the helper silently ran the whole loop on the calling thread + // instead of parallelizing it. When the work is parallelized, Parallel.For surfaces a + // failing action as an AggregateException; the buggy single-threaded path would throw + // the action's exception directly. The action throws on its first invocation so each + // batch exits immediately instead of iterating billions of times. + if (Environment.ProcessorCount < 2) + { + Assert.Inconclusive("Parallel dispatch requires more than one processor."); + } + + Exception? caught = null; + + try + { + ParallelHelper.For(int.MinValue, int.MaxValue, default(ThrowingAction), 1); + } + catch (Exception e) + { + caught = e; + } + + Assert.IsInstanceOfType(caught, typeof(AggregateException), "The work was not parallelized."); + Assert.IsTrue( + ((AggregateException)caught!).Flatten().InnerExceptions.All(static e => e is ThrowingActionException), + "Unexpected exception type."); + } + + /// + /// An exception type thrown by . + /// + private sealed class ThrowingActionException : Exception + { + } + + /// + /// A type implementing that throws on its first invocation. + /// + private readonly struct ThrowingAction : IAction + { + /// + public void Invoke(int i) + { + throw new ThrowingActionException(); + } + } + /// /// A type implementing to initialize an array ///