diff --git a/src/SharpCompress/IO/BufferedSubStream.cs b/src/SharpCompress/IO/BufferedSubStream.cs index 7a51bb0ff..1cfc1867b 100755 --- a/src/SharpCompress/IO/BufferedSubStream.cs +++ b/src/SharpCompress/IO/BufferedSubStream.cs @@ -29,17 +29,25 @@ protected override void Dispose(bool disposing) #if DEBUG_STREAMS this.DebugDispose(typeof(BufferedSubStream)); #endif - if (disposing) + if (_isDisposed) + { + return; + } + _isDisposed = true; + + if (disposing && _cache is not null) { ArrayPool.Shared.Return(_cache); + _cache = null; } base.Dispose(disposing); } private int _cacheOffset; private int _cacheLength; - private readonly byte[] _cache = ArrayPool.Shared.Rent(32 << 10); + private byte[]? _cache = ArrayPool.Shared.Rent(81920); private long origin; + private bool _isDisposed; private long BytesLeftToRead { get; set; } @@ -61,14 +69,26 @@ public override long Position private void RefillCache() { - var count = (int)Math.Min(BytesLeftToRead, _cache.Length); + if (_isDisposed) + { + throw new ObjectDisposedException(nameof(BufferedSubStream)); + } + + var count = (int)Math.Min(BytesLeftToRead, _cache!.Length); _cacheOffset = 0; if (count == 0) { _cacheLength = 0; return; } - Stream.Position = origin; + + // Only seek if we're not already at the correct position + // This avoids expensive seek operations when reading sequentially + if (Stream.CanSeek && Stream.Position != origin) + { + Stream.Position = origin; + } + _cacheLength = Stream.Read(_cache, 0, count); origin += _cacheLength; BytesLeftToRead -= _cacheLength; @@ -76,14 +96,24 @@ private void RefillCache() private async ValueTask RefillCacheAsync(CancellationToken cancellationToken) { - var count = (int)Math.Min(BytesLeftToRead, _cache.Length); + if (_isDisposed) + { + throw new ObjectDisposedException(nameof(BufferedSubStream)); + } + + var count = (int)Math.Min(BytesLeftToRead, _cache!.Length); _cacheOffset = 0; if (count == 0) { _cacheLength = 0; return; } - Stream.Position = origin; + // Only seek if we're not already at the correct position + // This avoids expensive seek operations when reading sequentially + if (Stream.CanSeek && Stream.Position != origin) + { + Stream.Position = origin; + } _cacheLength = await Stream .ReadAsync(_cache, 0, count, cancellationToken) .ConfigureAwait(false); @@ -106,7 +136,7 @@ public override int Read(byte[] buffer, int offset, int count) } count = Math.Min(count, _cacheLength - _cacheOffset); - Buffer.BlockCopy(_cache, _cacheOffset, buffer, offset, count); + Buffer.BlockCopy(_cache!, _cacheOffset, buffer, offset, count); _cacheOffset += count; } @@ -124,7 +154,7 @@ public override int ReadByte() } } - return _cache[_cacheOffset++]; + return _cache![_cacheOffset++]; } public override async Task ReadAsync( @@ -147,7 +177,7 @@ CancellationToken cancellationToken } count = Math.Min(count, _cacheLength - _cacheOffset); - Buffer.BlockCopy(_cache, _cacheOffset, buffer, offset, count); + Buffer.BlockCopy(_cache!, _cacheOffset, buffer, offset, count); _cacheOffset += count; } @@ -174,7 +204,7 @@ public override async ValueTask ReadAsync( } count = Math.Min(count, _cacheLength - _cacheOffset); - _cache.AsSpan(_cacheOffset, count).CopyTo(buffer.Span); + _cache!.AsSpan(_cacheOffset, count).CopyTo(buffer.Span); _cacheOffset += count; } diff --git a/src/SharpCompress/IO/SharpCompressStream.cs b/src/SharpCompress/IO/SharpCompressStream.cs index ad4c6e351..cc64a0c2f 100644 --- a/src/SharpCompress/IO/SharpCompressStream.cs +++ b/src/SharpCompress/IO/SharpCompressStream.cs @@ -255,7 +255,6 @@ public override long Seek(long offset, SeekOrigin origin) ValidateBufferState(); } - long orig = _internalPosition; long targetPos; // Calculate the absolute target position based on origin switch (origin) diff --git a/tests/SharpCompress.Test/Streams/SharpCompressStreamTest.cs b/tests/SharpCompress.Test/Streams/SharpCompressStreamTest.cs index 1c4a8b571..f74a6670e 100644 --- a/tests/SharpCompress.Test/Streams/SharpCompressStreamTest.cs +++ b/tests/SharpCompress.Test/Streams/SharpCompressStreamTest.cs @@ -5,6 +5,7 @@ using System.Text; using SharpCompress.Compressors.LZMA; using SharpCompress.IO; +using SharpCompress.Test.Mocks; using Xunit; namespace SharpCompress.Test.Streams; @@ -64,7 +65,14 @@ public void BufferReadAndSeekTest() { createData(ms); - using (SharpCompressStream scs = new SharpCompressStream(ms, true, false, 0x10000)) + using ( + SharpCompressStream scs = new SharpCompressStream( + new ForwardOnlyStream(ms), + true, + false, + 0x10000 + ) + ) { IStreamStack stack = (IStreamStack)scs; @@ -89,4 +97,25 @@ public void BufferReadAndSeekTest() } } } + + [Fact] + public void BufferedSubStream_DoubleDispose_DoesNotCorruptArrayPool() + { + // This test verifies that calling Dispose multiple times on BufferedSubStream + // doesn't return the same array to the pool twice, which would cause pool corruption + byte[] data = new byte[0x10000]; + using (MemoryStream ms = new MemoryStream(data)) + { + var stream = new BufferedSubStream(ms, 0, data.Length); + + // First disposal + stream.Dispose(); + + // Second disposal should not throw or corrupt the pool + stream.Dispose(); + } + + // If we got here without an exception, the test passed + Assert.True(true); + } }