Fix edge cases with 1-byte reads#1282
Conversation
…y to return incorrect results when reading a stream with `ReadByte()`. As the use of `ReadByte()` on a compressed stream is arguably seldom used, I opted for a simple "read into 1-byte array" solution that is not terribly resource efficient, but prevents having code replicated from the main `Read()` method into `ReadByte()`. Unfortunately, the main `Read()` method also had issues when reading into a 1-byte array, so that was fixed as well. Added a set of tests that explain the issue and fail before the fix, but succeeds after. These are clearly edge case, but if anyone passes a compressed stream to another library, event these edge cases should work correctly.
There was a problem hiding this comment.
Pull request overview
Fixes incorrect behavior when consuming deflated ZIP entry streams via Stream.ReadByte() / 1-byte Read() calls by correcting end-of-stream handling in ZlibBaseStream, and adds regression tests to cover byte-by-byte and buffered read roundtrips across both Archive and Reader APIs.
Changes:
- Add
ZlibBaseStream.ReadByte()override that delegates toRead()and returns-1on EOF. - Fix
ZlibBaseStream.Read(...)/ReadAsync(...)logic to avoid misinterpreting a 1-byte read result asZ_STREAM_END, preventing spurious rewinds. - Add ZIP deflate roundtrip tests validating both buffered (
CopyTo) and byte-by-byte (ReadByte) extraction paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/SharpCompress.Test/Zip/ZipCompressionRoundtripTests.cs | Adds regression coverage for buffered vs. byte-by-byte reads through Archive and Reader APIs. |
| src/SharpCompress/Compressors/Deflate/ZlibBaseStream.cs | Fixes 1-byte read edge cases by overriding ReadByte() and preserving the zlib result code before converting rc to byte count. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…re related to the fixes though.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for finding this. I'm mainly nervous around its usage and the using Read() on an asynchronous stream. I have tests that only use the async version to prove things. I'm wondering if it's best to just have the ReadByte throw an exception and have the Read and ReadAsync methods work with single bytes |
|
@adamhathcock Not sure I understand the concern. The |
|
but there isn't a ReadByte for the async path. Since ReadByte reuses the sync path, it would fail on asynchronous operations/tests. |
|
Added the bug here: #1346 Tempted to just make ZLibStream.ReadByte return -1 |
This PR fixes a missing overload for
ReadByte()that caused the library to return incorrect results when reading a stream withReadByte().As the use of
ReadByte()on a compressed stream is arguably seldom used, I opted for a simple "read into 1-byte array" solution that is not terribly resource efficient, but prevents having code replicated from the mainRead()method intoReadByte().Unfortunately, the main
Read()method also had issues when reading into a 1-byte array, so that was fixed as well.Added a set of tests that explain the issue and fail before the fix, but succeeds after.
These are clearly edge case, but if anyone passes a compressed stream to another library, even these edge cases should work correctly.