-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Make DeflateStream write out headers and footers on empty streams. #118570
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.
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
base: main
Are you sure you want to change the base?
Make DeflateStream write out headers and footers on empty streams. #118570
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures that DeflateStream writes necessary headers and footers even for empty input streams, addressing issue #66449. The change modifies the behavior where previously empty DeflateStreams would produce zero output, which could cause compatibility issues with compression standards.
Key changes:
- Modified DeflateStream to write output for empty streams by default
- Added internal constructor and logic to maintain backward compatibility with ZipArchive
- Updated test cases to verify the new behavior
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
DeflateStream.cs |
Added internal constructor with noFlushOnEmpty parameter and modified flush logic to write output for empty streams unless explicitly suppressed |
ZipArchiveEntry.cs |
Updated to use new DeflateStream constructor that suppresses empty output for backward compatibility |
CompressionStreamUnitTests.Deflate.cs |
Added test to verify DeflateStream writes output for empty streams |
CompressionStreamUnitTests.Gzip.cs |
Added test to verify GZipStream writes headers/footers for empty streams, plus minor formatting fixes |
zip_UpdateTests.cs |
Refactored test to use helper method for better code organization |
zip_InvalidParametersAndStrangeFiles.cs |
Fixed array reference bug in test |
src/libraries/System.IO.Compression/tests/CompressionStreamUnitTests.Gzip.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/CompressionStreamUnitTests.Gzip.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/CompressionStreamUnitTests.Deflate.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/CompressionStreamUnitTests.Deflate.cs
Outdated
Show resolved
Hide resolved
Is this functionally necessary? What would someone else implementing their own ZipArchive-equivalent do? |
It isn't but there have been some failing tests which I found very hard to fix. Specifically, if there was an existing archive with deflated 0-bytes entry, then opening the entry stream in Update mode, and disposing everything without making any changes would corrupt the archive on the disk. the current code keeps track whether there have been any writes to the entry stream (so that it knows to write the file header record before that), there is a lot of juggling with stream positions and seeking, and the code does not expect the first write could be done in a (Deflate)Stream.Dispose(). to fix this "properly" would probably mean significant changes to ZipArchive which I wanted to avoid until I get to know the codebase better. |
Converting to draft for now as this will need to wait until .NET 11 |
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs
Outdated
Show resolved
Hide resolved
I got an idea how to preserve ZipArchive's current behavior without internal-only constructor on DeflateStream. The root of the issue is that ZipArchive attempts to store 0-length files as Stored (i.e. uncompressed), which saves a few bytes by not including header/footer of an empty deflated file. This behavior is consistent across multiple zip archivers I have tested, but is not trivial to implement if we give user a plain stream for writing. When user opens the entry stream via Open(), we can't know if the file will be empty or not and if we create a DeflateStream at that point, it would eventually write something during Dispose(). To preserve the 0-length files behavior, this PR makes instantiation of the compression streams lazily on the first write to the entry stream. If there are no writes, logic to detect 0-length files correctly detects this case and changes the compression method to Stored. If there are writes, the DeflatStream is created and the code behaves as before. |
Fixes #66449.
Ensure that DeflateStream writes necessary headers and footers even for empty input streams.
To preserve ZipArchive's functionality of storing 0-length bytes as Stored, we will now instantiate compression streams on the first write to the ZipArchiveEntry's stream, that way we avoid writing empty Deflate envelopes.