-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 slightly incorrect Zip files #68106
Conversation
Tagging subscribers to this area: @dotnet/area-system-io-compression Issue DetailsFix #49580
|
Also fix new SC test that will only pass elevated. cc @deeprobin -- our PR validation runs elevated, I believe. |
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Show resolved
Hide resolved
Updated with full annotations added for the test zip, making clear what's going on. |
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Show resolved
Hide resolved
failures are #67349 |
BTW @dotnet/area-system-io-compression are we aware of any public corpus of test zip files? Clearly, there is some variation in spec interpretations. If there was such a corpus, I could imagine we write a script to unzip them all with "main" and also with 6.0 and verify the same outputs/errors. That would reduce the risk of the #1094 experience we had. |
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.
Some nits.
Looks overall good to me
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Robin Lindner <[email protected]>
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.
Overall LGTM, I've left some comments, but all of them are just nits.
@danmoseley thanks a lot!
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Outdated
Show resolved
Hide resolved
Assert.Equal(6, entry.CompressedLength); // it should have used 32-bit size | ||
} | ||
|
||
private static readonly byte[] s_tinyZip64 = |
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.
nit: based on the description provided for the tests above, I think that "slightlyIncorrect" would be a better name than "tiny" here
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Adam Sitnik <[email protected]>
@dotnet/dnceng seems an issue in dotnet-install.sh?
|
@danmoseley. It does look like there is a bug with it, but that tool isn't written/maintained by dnceng. If you click the "report infrastructure issue" we could try and get it assigned to the right people. |
My bad, thanks @ChadNedzlek and @missymessa |
No worries, thanks for using the new features :) |
* Add test that fails * fix * Fix test failing unelevated * annotate * overflow * Apply suggestions from code review Co-authored-by: Robin Lindner <[email protected]> * Apply suggestions from code review Co-authored-by: Adam Sitnik <[email protected]> Co-authored-by: Robin Lindner <[email protected]> Co-authored-by: Adam Sitnik <[email protected]>
Fix #49580