Skip to content

Commit 62effc1

Browse files
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.
1 parent c5a82e5 commit 62effc1

2 files changed

Lines changed: 82 additions & 4 deletions

File tree

src/Apache.Arrow/Memory/NativeBuffer.cs

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,8 @@ public void Grow(int newElementCount, bool zeroFill = true)
9090
if (newElementCount <= Length)
9191
return;
9292

93-
// Exponential growth (2x) to amortise repeated grows
94-
// TODO: There might be a size that's big enough to work for this case but not too big to overflow.
95-
// We could use that instead of blindly doubling.
96-
int newCount = Math.Max(newElementCount, checked(Length * 2));
9793
int elementSize = Unsafe.SizeOf<TItem>();
94+
int newCount = ComputeGrowCount(Length, newElementCount, elementSize);
9895
int needed = checked(newCount * elementSize);
9996

10097
var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer<TItem, TTracker>));
@@ -109,6 +106,24 @@ public void Grow(int newElementCount, bool zeroFill = true)
109106
Length = newCount;
110107
}
111108

109+
/// <summary>
110+
/// The element count to grow to: double the current length to amortise repeated grows, but never
111+
/// past the largest buffer that can be addressed, and never below what the caller asked for.
112+
/// </summary>
113+
/// <remarks>
114+
/// Doubling used to be unconditional and checked, so once the buffer passed half of the maximum
115+
/// its next grow threw <see cref="OverflowException"/> however little was asked for, even though
116+
/// the requested size still fit. Saturating instead keeps growth amortised right up to the
117+
/// ceiling; a request that genuinely cannot be addressed still overflows at the byte-size
118+
/// calculation in <see cref="Grow"/>, as before.
119+
/// </remarks>
120+
internal static int ComputeGrowCount(int length, int newElementCount, int elementSize)
121+
{
122+
int maxCount = int.MaxValue / elementSize;
123+
long doubled = (long)length * 2;
124+
return (int)Math.Max(newElementCount, Math.Min(doubled, maxCount));
125+
}
126+
112127
public void Dispose()
113128
{
114129
IDisposable disposable = _owner;

test/Apache.Arrow.Tests/NativeBufferTests.cs

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,69 @@ public void GrowWithSmallerOrEqualCountIsNoOp()
8484
Assert.Equal(42, buf.Span[0]);
8585
}
8686

87+
// Growth doubles to stay amortised, but must saturate rather than overflow. Doubling used to be
88+
// unconditional and checked, so a buffer past half the maximum threw OverflowException on its
89+
// next grow however little was asked for — a byte buffer could not grow beyond about 1 GiB.
90+
//
91+
// The arithmetic is tested directly: reproducing it through Grow would mean allocating more than
92+
// a gigabyte, which is not something to put in a unit test.
93+
[Theory]
94+
// length, requested, elementSize, expected
95+
[InlineData(0, 1, 1, 1)] // nothing to double yet
96+
[InlineData(3, 10, 4, 10)] // request exceeds the doubling
97+
[InlineData(8, 10, 4, 16)] // doubling exceeds the request
98+
[InlineData(5, 5, 4, 10)] // equal: doubling still wins
99+
public void ComputeGrowCountDoublesWhileItFits(
100+
int length, int requested, int elementSize, int expected)
101+
{
102+
Assert.Equal(
103+
expected,
104+
NativeBuffer<byte, NoOpAllocationTracker>.ComputeGrowCount(length, requested, elementSize));
105+
}
106+
107+
[Fact]
108+
public void ComputeGrowCountSaturatesInsteadOfOverflowing()
109+
{
110+
// Past half the maximum, doubling would overflow. The result saturates at the largest
111+
// addressable count and still covers the request.
112+
const int elementSize = 1;
113+
int overHalf = (int.MaxValue / 2) + 1000;
114+
115+
int grown = NativeBuffer<byte, NoOpAllocationTracker>.ComputeGrowCount(
116+
overHalf, overHalf + 1, elementSize);
117+
118+
Assert.Equal(int.MaxValue, grown);
119+
Assert.True(grown >= overHalf + 1);
120+
}
121+
122+
[Fact]
123+
public void ComputeGrowCountSaturatesPerElementSize()
124+
{
125+
// The ceiling is a byte count, so a wider element saturates at proportionally fewer of them.
126+
const int elementSize = 8;
127+
int maxCount = int.MaxValue / elementSize;
128+
int overHalf = (maxCount / 2) + 1000;
129+
130+
int grown = NativeBuffer<long, NoOpAllocationTracker>.ComputeGrowCount(
131+
overHalf, overHalf + 1, elementSize);
132+
133+
Assert.Equal(maxCount, grown);
134+
Assert.True((long)grown * elementSize <= int.MaxValue);
135+
}
136+
137+
[Fact]
138+
public void ComputeGrowCountNeverReturnsLessThanRequested()
139+
{
140+
// A request larger than the ceiling is not silently truncated; Grow still refuses it when it
141+
// works out the byte size.
142+
const int elementSize = 8;
143+
int beyond = (int.MaxValue / elementSize) + 1;
144+
145+
Assert.Equal(
146+
beyond,
147+
NativeBuffer<long, NoOpAllocationTracker>.ComputeGrowCount(0, beyond, elementSize));
148+
}
149+
87150
[Fact]
88151
public void BuildTransfersOwnershipToArrowBuffer()
89152
{

0 commit comments

Comments
 (0)