From d1e4526c3402e89588750dd8781dd35eabda126d Mon Sep 17 00:00:00 2001 From: Curt Hagenlocher Date: Wed, 19 Aug 2026 20:13:51 -0700 Subject: [PATCH 1/3] fix: Saturate NativeBuffer growth instead of overflowing past half of int.MaxValue Grow doubled the current length in a checked context without saturating, so once a buffer passed half of the addressable maximum its next grow threw OverflowException however small the requested increase and even though the requested size still fit. For a byte buffer that was a hard ceiling near 1 GiB. The TODO those lines carried described exactly this. Growth now saturates at the largest addressable element count, keeping it amortised right up to the ceiling. A request that genuinely cannot be addressed still fails at the byte-size calculation, as before, so behaviour is unchanged for anything that could not have worked. The count arithmetic is extracted so it can be tested at the boundary without allocating more than a gigabyte in a unit test, including the per-element-size ceiling: the limit is a byte count, so a wider element type saturates at proportionally fewer elements. Closes #418. --- src/Apache.Arrow/Memory/NativeBuffer.cs | 23 +++++-- test/Apache.Arrow.Tests/NativeBufferTests.cs | 63 ++++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/src/Apache.Arrow/Memory/NativeBuffer.cs b/src/Apache.Arrow/Memory/NativeBuffer.cs index f5b8f619..51a2843e 100644 --- a/src/Apache.Arrow/Memory/NativeBuffer.cs +++ b/src/Apache.Arrow/Memory/NativeBuffer.cs @@ -90,11 +90,8 @@ public void Grow(int newElementCount, bool zeroFill = true) if (newElementCount <= Length) return; - // Exponential growth (2x) to amortise repeated grows - // TODO: There might be a size that's big enough to work for this case but not too big to overflow. - // We could use that instead of blindly doubling. - int newCount = Math.Max(newElementCount, checked(Length * 2)); int elementSize = Unsafe.SizeOf(); + int newCount = ComputeGrowCount(Length, newElementCount, elementSize); int needed = checked(newCount * elementSize); var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer)); @@ -109,6 +106,24 @@ public void Grow(int newElementCount, bool zeroFill = true) Length = newCount; } + /// + /// The element count to grow to: double the current length to amortise repeated grows, but never + /// past the largest buffer that can be addressed, and never below what the caller asked for. + /// + /// + /// Doubling used to be unconditional and checked, so once the buffer passed half of the maximum + /// its next grow threw however little was asked for, even though + /// the requested size still fit. Saturating instead keeps growth amortised right up to the + /// ceiling; a request that genuinely cannot be addressed still overflows at the byte-size + /// calculation in , as before. + /// + internal static int ComputeGrowCount(int length, int newElementCount, int elementSize) + { + int maxCount = int.MaxValue / elementSize; + long doubled = (long)length * 2; + return (int)Math.Max(newElementCount, Math.Min(doubled, maxCount)); + } + public void Dispose() { IDisposable disposable = _owner; diff --git a/test/Apache.Arrow.Tests/NativeBufferTests.cs b/test/Apache.Arrow.Tests/NativeBufferTests.cs index 84d050cf..87ea4405 100644 --- a/test/Apache.Arrow.Tests/NativeBufferTests.cs +++ b/test/Apache.Arrow.Tests/NativeBufferTests.cs @@ -84,6 +84,69 @@ public void GrowWithSmallerOrEqualCountIsNoOp() Assert.Equal(42, buf.Span[0]); } + // Growth doubles to stay amortised, but must saturate rather than overflow. Doubling used to be + // unconditional and checked, so a buffer past half the maximum threw OverflowException on its + // next grow however little was asked for — a byte buffer could not grow beyond about 1 GiB. + // + // The arithmetic is tested directly: reproducing it through Grow would mean allocating more than + // a gigabyte, which is not something to put in a unit test. + [Theory] + // length, requested, elementSize, expected + [InlineData(0, 1, 1, 1)] // nothing to double yet + [InlineData(3, 10, 4, 10)] // request exceeds the doubling + [InlineData(8, 10, 4, 16)] // doubling exceeds the request + [InlineData(5, 5, 4, 10)] // equal: doubling still wins + public void ComputeGrowCountDoublesWhileItFits( + int length, int requested, int elementSize, int expected) + { + Assert.Equal( + expected, + NativeBuffer.ComputeGrowCount(length, requested, elementSize)); + } + + [Fact] + public void ComputeGrowCountSaturatesInsteadOfOverflowing() + { + // Past half the maximum, doubling would overflow. The result saturates at the largest + // addressable count and still covers the request. + const int elementSize = 1; + int overHalf = (int.MaxValue / 2) + 1000; + + int grown = NativeBuffer.ComputeGrowCount( + overHalf, overHalf + 1, elementSize); + + Assert.Equal(int.MaxValue, grown); + Assert.True(grown >= overHalf + 1); + } + + [Fact] + public void ComputeGrowCountSaturatesPerElementSize() + { + // The ceiling is a byte count, so a wider element saturates at proportionally fewer of them. + const int elementSize = 8; + int maxCount = int.MaxValue / elementSize; + int overHalf = (maxCount / 2) + 1000; + + int grown = NativeBuffer.ComputeGrowCount( + overHalf, overHalf + 1, elementSize); + + Assert.Equal(maxCount, grown); + Assert.True((long)grown * elementSize <= int.MaxValue); + } + + [Fact] + public void ComputeGrowCountNeverReturnsLessThanRequested() + { + // A request larger than the ceiling is not silently truncated; Grow still refuses it when it + // works out the byte size. + const int elementSize = 8; + int beyond = (int.MaxValue / elementSize) + 1; + + Assert.Equal( + beyond, + NativeBuffer.ComputeGrowCount(0, beyond, elementSize)); + } + [Fact] public void BuildTransfersOwnershipToArrowBuffer() { From ed366b1db9d9793291415f82ba819ec111dc16b5 Mon Sep 17 00:00:00 2001 From: Curt Hagenlocher Date: Sat, 22 Aug 2026 07:27:27 -0700 Subject: [PATCH 2/3] fix: Assert the element size invariant in ComputeGrowCount Addresses review feedback on #419. `elementSize` is only ever `Unsafe.SizeOf()` for an unmanaged `TItem`, so it cannot be zero or negative and the division cannot fault; the parameter exists so the boundary can be tested without allocating a buffer of that size. A `Debug.Assert` states that invariant for callers of the internal helper without adding a runtime check to the growth path. Co-Authored-By: Claude Opus 5 (1M context) --- src/Apache.Arrow/Memory/NativeBuffer.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Apache.Arrow/Memory/NativeBuffer.cs b/src/Apache.Arrow/Memory/NativeBuffer.cs index 51a2843e..4a1f6428 100644 --- a/src/Apache.Arrow/Memory/NativeBuffer.cs +++ b/src/Apache.Arrow/Memory/NativeBuffer.cs @@ -15,6 +15,7 @@ using System; using System.Buffers; +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Threading; @@ -119,6 +120,10 @@ public void Grow(int newElementCount, bool zeroFill = true) /// internal static int ComputeGrowCount(int length, int newElementCount, int elementSize) { + // Always Unsafe.SizeOf() for an unmanaged TItem, so never below one; the parameter + // exists so the boundary can be tested without allocating a buffer of that size. + Debug.Assert(elementSize > 0); + int maxCount = int.MaxValue / elementSize; long doubled = (long)length * 2; return (int)Math.Max(newElementCount, Math.Min(doubled, maxCount)); From bedad88507f4e9af0e1fe1e1fb2a2c9ce099bca3 Mon Sep 17 00:00:00 2001 From: Curt Hagenlocher Date: Sat, 22 Aug 2026 07:34:22 -0700 Subject: [PATCH 3/3] fix: Drop the redundant disposed check and trim the ComputeGrowCount remarks Grow already throws ObjectDisposedException on entry, so re-checking _owner before Reallocate was dead code. The block narrated the history of the bug being fixed, which belongs in the commit message and the pull request rather than in the API documentation. Co-Authored-By: Claude Opus 5 (1M context) --- src/Apache.Arrow/Memory/NativeBuffer.cs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/Apache.Arrow/Memory/NativeBuffer.cs b/src/Apache.Arrow/Memory/NativeBuffer.cs index 4a1f6428..b042999b 100644 --- a/src/Apache.Arrow/Memory/NativeBuffer.cs +++ b/src/Apache.Arrow/Memory/NativeBuffer.cs @@ -95,8 +95,7 @@ public void Grow(int newElementCount, bool zeroFill = true) int newCount = ComputeGrowCount(Length, newElementCount, elementSize); int needed = checked(newCount * elementSize); - var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer)); - owner.Reallocate(needed); + _owner.Reallocate(needed); if (zeroFill) { @@ -111,13 +110,6 @@ public void Grow(int newElementCount, bool zeroFill = true) /// The element count to grow to: double the current length to amortise repeated grows, but never /// past the largest buffer that can be addressed, and never below what the caller asked for. /// - /// - /// Doubling used to be unconditional and checked, so once the buffer passed half of the maximum - /// its next grow threw however little was asked for, even though - /// the requested size still fit. Saturating instead keeps growth amortised right up to the - /// ceiling; a request that genuinely cannot be addressed still overflows at the byte-size - /// calculation in , as before. - /// internal static int ComputeGrowCount(int length, int newElementCount, int elementSize) { // Always Unsafe.SizeOf() for an unmanaged TItem, so never below one; the parameter