-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix reading Zip64 end of central directory locator #118239
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
Fix reading Zip64 end of central directory locator #118239
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 fixes a bug in reading Zip64 end of central directory locator by adding proper stream length validation and correcting internal signature seeking methods. The changes address issue #117147 where malformed ZIP archives with insufficient data could trigger assertions.
Key changes:
- Adds stream length check before attempting to read Zip64 end of central directory locator
- Fixes SeekBackwardsToSignature methods to respect maxBytesToRead parameter properly
- Corrects calculation of total bytes read and overlap handling in signature seeking
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
ZipHelper.cs | Fixed SeekBackwardsToSignature to properly respect maxBytesToRead and calculate overlap correctly |
ZipHelper.Async.cs | Applied same fixes to async version of SeekBackwardsToSignature method |
ZipArchive.cs | Added stream length validation and simplified EOCD position tracking |
ZipArchive.Async.cs | Applied same validation and position tracking fixes to async version |
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.Async.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.Async.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.Async.cs
Outdated
Show resolved
Hide resolved
Tagging subscribers to this area: @dotnet/area-system-io-compression |
…ZipHelper.Async.cs Co-authored-by: Copilot <[email protected]>
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs
Show resolved
Hide resolved
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
@MihuBot fuzz ZipArchive |
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs
Show resolved
Hide resolved
I think we are ready for review. |
Fixes #117147.
Replaces #118230.
The structure of Zip64 ZIP file is as follows:
The applications reading the ZIP archive are expected to parse it from the end, where presence of ZIP64 fields is indicated by special values in the eocd record.
the problematic input is too short to contain eocd locator, which triggers the assert, this PR adds a stream length check for that.
This PR also fixes some internal methods (SeekBackwardsToSignature(Async)) to now behave as expected. Previous implementation always read at least 4kb even if maxBytesToRead was less. This in turn uncovered a different bug where expected offset of eocd locator was not calculated correctly.
Review of the fuzzing code (ZipArchiveFuzzer) revealed some issues (like disposing the stream between runs and ignoring all exceptions), so this PR fixes improves the fuzzer az well.