From d92def91b0ed450ebb0e1d6665b14d64d7d566ed Mon Sep 17 00:00:00 2001 From: Adam Hathcock Date: Sat, 31 Jan 2026 10:59:30 +0000 Subject: [PATCH] Opus 4.5 did this fix, need to understand it --- ...taDescriptorStream-RewindableStream-Fix.md | 129 ++++++++++++++++++ .../Zip/StreamingZipHeaderFactory.Async.cs | 35 +++-- .../Common/Zip/StreamingZipHeaderFactory.cs | 29 +++- 3 files changed, 177 insertions(+), 16 deletions(-) create mode 100644 docs/DataDescriptorStream-RewindableStream-Fix.md diff --git a/docs/DataDescriptorStream-RewindableStream-Fix.md b/docs/DataDescriptorStream-RewindableStream-Fix.md new file mode 100644 index 00000000..eb31de14 --- /dev/null +++ b/docs/DataDescriptorStream-RewindableStream-Fix.md @@ -0,0 +1,129 @@ +# DataDescriptorStream and RewindableStream Fix + +## Summary + +Fixed the `Zip_Uncompressed_Read_All` test failure caused by incompatibility between `DataDescriptorStream` seeking requirements and the new `RewindableStream` wrapper used in `StreamingZipHeaderFactory`. + +## Problem Description + +### Symptom +The test `Zip_Uncompressed_Read_All` was failing with: +``` +System.NotSupportedException : Cannot seek outside buffered region. +``` + +### Root Cause + +The issue had two related aspects: + +#### 1. Double-Wrapping of RewindableStream + +`StreamingZipHeaderFactory.ReadStreamHeader()` was creating a new `RewindableStream` wrapper: +```csharp +var rewindableStream = new RewindableStream(stream); +``` + +When `ReaderFactory.OpenReader()` already wraps the input stream with `SeekableRewindableStream` (for seekable streams), this resulted in double-wrapping: +``` +DataDescriptorStream + -> NonDisposingStream + -> RewindableStream (new, plain) <-- created by ReadStreamHeader + -> SeekableRewindableStream <-- created by ReaderFactory + -> FileStream +``` + +The inner plain `RewindableStream` lost the seeking capability of `SeekableRewindableStream`. + +#### 2. Recording State Interference + +Even after fixing the double-wrapping using `RewindableStream.EnsureSeekable()`, there was another issue: + +`StreamingZipHeaderFactory.ReadStreamHeader()` contains code to peek ahead when checking for zero-length files with `UsePostDataDescriptor`: + +```csharp +rewindableStream.StartRecording(); +var nextHeaderBytes = reader.ReadUInt32(); +rewindableStream.Rewind(true); +``` + +This code was interfering with the recording state that `ReaderFactory.OpenReader()` had set up: + +1. `ReaderFactory.OpenReader()` calls `bStream.StartRecording()` at position 0 +2. Factory detection calls `StreamingZipHeaderFactory.ReadStreamHeader()` via `IsZipFile()` +3. Inside `ReadStreamHeader`, the above code overwrites the recorded position +4. `Rewind(true)` stops recording and seeks to the wrong position +5. When control returns to `Factory.TryOpenReader()`, it calls `stream.Rewind(true)`, but recording is already stopped, so nothing happens +6. The stream position is not at the beginning, causing subsequent reads to fail + +## Solution + +### Fix 1: Use EnsureSeekable instead of new RewindableStream + +Changed `StreamingZipHeaderFactory.ReadStreamHeader()` to use: +```csharp +var rewindableStream = RewindableStream.EnsureSeekable(stream); +``` + +This method: +- Returns the existing `RewindableStream` if the stream is already one (avoids double-wrapping) +- Creates a `SeekableRewindableStream` if the underlying stream is seekable +- Creates a plain `RewindableStream` only for non-seekable streams + +### Fix 2: Use direct position save/restore for SeekableRewindableStream + +For the peek-ahead logic, changed the code to check for `SeekableRewindableStream` specifically and use direct position manipulation: + +```csharp +if (rewindableStream is SeekableRewindableStream) +{ + // Direct position save/restore avoids interfering with caller's recording state + var savedPosition = rewindableStream.Position; + var nextHeaderBytes = reader.ReadUInt32(); + rewindableStream.Position = savedPosition; + header.HasData = !IsHeader(nextHeaderBytes); +} +else +{ + // Plain RewindableStream was created fresh by EnsureSeekable, safe to use recording + rewindableStream.StartRecording(); + var nextHeaderBytes = reader.ReadUInt32(); + rewindableStream.Rewind(true); + header.HasData = !IsHeader(nextHeaderBytes); +} +``` + +This approach: +- For `SeekableRewindableStream` (reused from caller): Uses direct position save/restore to avoid clobbering the caller's recording state +- For plain `RewindableStream` (freshly created): Uses the recording mechanism which is safe since the stream isn't shared + +## Files Changed + +- `src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.cs` +- `src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.Async.cs` + +## Design Notes + +### Why not fix RewindableStream.CanSeek? + +`RewindableStream.CanSeek` returns `true` even though it can only seek within its buffered region. We considered changing this to `false`, but: +1. It would be a breaking change for existing code that relies on `CanSeek` +2. The `RewindableStream` does provide limited seeking capability (within buffer) +3. Checking for `SeekableRewindableStream` specifically is more precise + +### Stream Wrapper Hierarchy + +Understanding the stream wrapper hierarchy is crucial: + +**For seekable source streams (e.g., FileStream):** +``` +SeekableRewindableStream (full seeking via underlying stream) + -> FileStream +``` + +**For non-seekable source streams (e.g., decompression streams):** +``` +RewindableStream (limited seeking via buffer) + -> DecompressionStream +``` + +`DataDescriptorStream` needs backward seeking to position the stream correctly after finding the data descriptor marker. This is why proper stream wrapper selection matters. diff --git a/src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.Async.cs b/src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.Async.cs index 126e855a..bdaf8817 100644 --- a/src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.Async.cs +++ b/src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.Async.cs @@ -72,7 +72,9 @@ internal sealed partial class StreamingZipHeaderFactory ) { _headerFactory = headerFactory; - _rewindableStream = new RewindableStream(stream); + // Use EnsureSeekable to avoid double-wrapping if stream is already a RewindableStream, + // and to preserve seekability for DataDescriptorStream which needs to seek backward + _rewindableStream = RewindableStream.EnsureSeekable(stream); _reader = new AsyncBinaryReader(_rewindableStream, leaveOpen: true); _cancellationToken = cancellationToken; } @@ -281,14 +283,29 @@ internal sealed partial class StreamingZipHeaderFactory } // Check if zip is streaming ( Length is 0 and is declared in PostDataDescriptor ) else if (localHeader.Flags.HasFlag(HeaderFlags.UsePostDataDescriptor)) { - _rewindableStream.StartRecording(); - var nextHeaderBytes = await _reader - .ReadUInt32Async(_cancellationToken) - .ConfigureAwait(false); - _rewindableStream.Rewind(true); - - // Check if next data is PostDataDescriptor, streamed file with 0 length - header.HasData = !IsHeader(nextHeaderBytes); + // Peek ahead to check if next data is a header or file data. + // For SeekableRewindableStream, use direct position save/restore to avoid + // interfering with any recording state set by the caller (e.g., ReaderFactory). + // Plain RewindableStream can use StartRecording/Rewind safely since it was + // created fresh by EnsureSeekable and isn't shared with the caller. + if (_rewindableStream is SeekableRewindableStream) + { + var savedPosition = _rewindableStream.Position; + var nextHeaderBytes = await _reader + .ReadUInt32Async(_cancellationToken) + .ConfigureAwait(false); + _rewindableStream.Position = savedPosition; + header.HasData = !IsHeader(nextHeaderBytes); + } + else + { + _rewindableStream.StartRecording(); + var nextHeaderBytes = await _reader + .ReadUInt32Async(_cancellationToken) + .ConfigureAwait(false); + _rewindableStream.Rewind(true); + header.HasData = !IsHeader(nextHeaderBytes); + } } else // We are not streaming and compressed size is 0, we have no data { diff --git a/src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.cs b/src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.cs index 95f95c49..3498e481 100644 --- a/src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.cs +++ b/src/SharpCompress/Common/Zip/StreamingZipHeaderFactory.cs @@ -23,7 +23,9 @@ internal sealed partial class StreamingZipHeaderFactory : ZipHeaderFactory internal IEnumerable ReadStreamHeader(Stream stream) { - var rewindableStream = new RewindableStream(stream); + // Use EnsureSeekable to avoid double-wrapping if stream is already a RewindableStream, + // and to preserve seekability for DataDescriptorStream which needs to seek backward + var rewindableStream = RewindableStream.EnsureSeekable(stream); while (true) { var reader = new BinaryReader(rewindableStream); @@ -174,12 +176,25 @@ internal sealed partial class StreamingZipHeaderFactory : ZipHeaderFactory } // Check if zip is streaming ( Length is 0 and is declared in PostDataDescriptor ) else if (local_header.Flags.HasFlag(HeaderFlags.UsePostDataDescriptor)) { - rewindableStream.StartRecording(); - var nextHeaderBytes = reader.ReadUInt32(); - rewindableStream.Rewind(true); - - // Check if next data is PostDataDescriptor, streamed file with 0 length - header.HasData = !IsHeader(nextHeaderBytes); + // Peek ahead to check if next data is a header or file data. + // For SeekableRewindableStream, use direct position save/restore to avoid + // interfering with any recording state set by the caller (e.g., ReaderFactory). + // Plain RewindableStream can use StartRecording/Rewind safely since it was + // created fresh by EnsureSeekable and isn't shared with the caller. + if (rewindableStream is SeekableRewindableStream) + { + var savedPosition = rewindableStream.Position; + var nextHeaderBytes = reader.ReadUInt32(); + rewindableStream.Position = savedPosition; + header.HasData = !IsHeader(nextHeaderBytes); + } + else + { + rewindableStream.StartRecording(); + var nextHeaderBytes = reader.ReadUInt32(); + rewindableStream.Rewind(true); + header.HasData = !IsHeader(nextHeaderBytes); + } } else // We are not streaming and compressed size is 0, we have no data {