-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Make DeflateStream write out headers and footers #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 #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 |
Fixes #66449.
Ensure that DeflateStream writes necessary headers and footers even for empty input streams.
For compatibility with existing ZipArchive code, internal uses in ZipArchive still suppress headers and footers on empty content, this should be fine as ZipArchive logic changes the compression method to Stored on empty files.