Merge pull request #1160 from adamhathcock/adam/check-if-seek

add check to see if we need to seek before hand

Author: adamhathcock

src/SharpCompress/IO/BufferedSubStream.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Buffers;
 using System.IO;
 using System.Threading;
 using System.Threading.Tasks;
@@ -28,14 +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<byte>.Shared.Return(_cache);
+            _cache = null;
+        }
         base.Dispose(disposing);
     }
 
     private int _cacheOffset;
     private int _cacheLength;
-    private readonly byte[] _cache = new byte[32 << 10];
+    private byte[]? _cache = ArrayPool<byte>.Shared.Rent(81920);
     private long origin;
+    private bool _isDisposed;
 
     private long BytesLeftToRead { get; set; }
 
@@ -57,29 +69,51 @@ 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;
     }
 
     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);
@@ -102,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;
         }
 
@@ -120,7 +154,7 @@ public override int ReadByte()
             }
         }
 
-        return _cache[_cacheOffset++];
+        return _cache![_cacheOffset++];
     }
 
     public override async Task<int> ReadAsync(
@@ -143,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;
         }
 
@@ -170,7 +204,7 @@ public override async ValueTask<int> ReadAsync(
             }
 
             count = Math.Min(count, _cacheLength - _cacheOffset);
-            _cache.AsSpan(_cacheOffset, count).CopyTo(buffer.Span);
+            _cache!.AsSpan(_cacheOffset, count).CopyTo(buffer.Span);
             _cacheOffset += count;
         }
 

src/SharpCompress/IO/SharpCompressStream.cs

@@ -257,7 +257,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)

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);
+    }
 }