Skip to content

Clear pool buffer array on disposal in ZCharArray #519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
axunonb wants to merge 4 commits into from
Closed

Clear pool buffer array on disposal in ZCharArray #519

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e6dd735
Refactor `ZCharArray` - clear array on disposal
axunonb Jan 29, 2026
c14283f
Merge branch 'main' into pr/zchararray
axunonb Jan 29, 2026
ebbe32b
Refactor `ZCharArray` - clear array on disposal
axunonb Jan 29, 2026
dbeab9e
Update ZCharArray.cs
axunonb Jan 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 15 additions & 13 deletions src/SmartFormat/ZString/ZCharArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ public struct ZCharArray : IDisposable
private static readonly ArrayPool<char>
Pool = ArrayPool<char>.Create(MaxBufferCapacity, 100);

private char[]? _bufferArray;
private char[] _bufferArray;
Comment thread
axunonb marked this conversation as resolved.
Outdated
private int _currentLength;
private bool _isDisposed;

/// <summary>
/// The default capacity of the array.
Expand All @@ -48,6 +49,7 @@ public ZCharArray(int length)
{
_bufferArray = Pool.Rent(length);
_currentLength = 0;
_isDisposed = false;
Comment thread
axunonb marked this conversation as resolved.
Outdated
}

/// <summary>
Expand Down Expand Up @@ -96,7 +98,7 @@ public int Capacity
get
{
ThrowIfDisposed();
return _bufferArray!.Length;
return _bufferArray.Length;
}
}

Expand All @@ -119,7 +121,7 @@ public void Reset()
private void Grow(int length)
{
var newArray = Pool.Rent(length);
Array.Copy(_bufferArray!, newArray, Math.Min(_bufferArray!.Length, length));
Array.Copy(_bufferArray, newArray, Math.Min(_bufferArray.Length, length));
Pool.Return(_bufferArray);
Comment thread
axunonb marked this conversation as resolved.
Outdated
_bufferArray = newArray;
}
Expand All @@ -133,7 +135,7 @@ public void Write(Span<char> data)
{
ThrowIfDisposed();
GrowBufferIfNeeded(data.Length);
data.CopyTo(_bufferArray!.AsSpan(_currentLength, data.Length));
data.CopyTo(_bufferArray.AsSpan(_currentLength, data.Length));
_currentLength += data.Length;
}

Expand All @@ -146,7 +148,7 @@ public void Write(ReadOnlySpan<char> data)
{
ThrowIfDisposed();
GrowBufferIfNeeded(data.Length);
data.CopyTo(_bufferArray!.AsSpan(_currentLength, data.Length));
data.CopyTo(_bufferArray.AsSpan(_currentLength, data.Length));
_currentLength += data.Length;
}

Expand All @@ -159,7 +161,7 @@ public void Write(string data)
{
ThrowIfDisposed();
GrowBufferIfNeeded(data.Length);
data.AsSpan().CopyTo(_bufferArray!.AsSpan(_currentLength, data.Length));
data.AsSpan().CopyTo(_bufferArray.AsSpan(_currentLength, data.Length));
_currentLength += data.Length;
}

Expand All @@ -172,7 +174,7 @@ public void Write(char c)
{
ThrowIfDisposed();
GrowBufferIfNeeded(1);
_bufferArray![_currentLength++] = c;
_bufferArray[_currentLength++] = c;
}

/// <summary>
Expand All @@ -188,7 +190,7 @@ public void Write(char c, int count)

for (var i = 0; i < count; i++)
{
_bufferArray![_currentLength++] = c;
_bufferArray[_currentLength++] = c;
}
}

Expand Down Expand Up @@ -246,7 +248,7 @@ private void GrowBufferIfNeeded(int dataLength)
/// <summary>
/// Returns <see langword="true"/> if the array has been disposed.
/// </summary>
public bool IsDisposed => _bufferArray is null;
public bool IsDisposed => _isDisposed;

private void ThrowIfDisposed()
{
Expand All @@ -261,17 +263,17 @@ private void ThrowIfDisposed()
public override string ToString()
{
ThrowIfDisposed();
return new string(_bufferArray!, 0, _currentLength);
return new string(_bufferArray, 0, _currentLength);
}

/// <summary>
/// Disposes the array, returning it to the <see cref="ArrayPool{T}"/>.
/// </summary>
public void Dispose()
{
if (IsDisposed) return;
if (_isDisposed) return;

Pool.Return(_bufferArray!);
_bufferArray = null;
Pool.Return(_bufferArray, clearArray: true);

Copilot AI Jan 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting clearArray: true when returning the buffer to the pool will clear all characters in the rented array before returning it. While this can be beneficial for security (preventing data leakage), it has performance implications for large buffers.

Given that ZCharArray uses buffers up to MaxBufferCapacity (10,000,000 characters = 20MB), clearing the entire buffer on every disposal could add significant overhead, especially in high-frequency scenarios.

Consider whether the security benefit of clearing the array outweighs the performance cost for this use case. The PR description mentions this is for security/correctness, but it would be worth:

  1. Measuring the performance impact in realistic scenarios
  2. Documenting why this trade-off was made
  3. Potentially making it configurable if different use cases have different security/performance requirements

Note: The ArrayPool implementation may already zero out arrays in some cases, so this might be redundant depending on the pool's configuration.

Copilot uses AI. Check for mistakes.
_isDisposed = true;
}
}
Loading