-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Reimplement PR 112032, preserve trailing extra data when updating ZIP files #113306
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
Conversation
Tagging subscribers to this area: @dotnet/area-system-io-compression |
This data is generated by zipalign. It needs to be accounted for and preserved.
Feel free to mark it as ready to review when you're ready and ping me to give it a full review. |
Thanks @carlossanlop - I can see enough tests passing on enough platforms for review. Besides the tests, I've rolled the latest build of System.IO.Compression into a local .NET 10 preview installation and published a simple .NET for Android project. I was able to reproduce the problem resolved by PR 112032 with this, but not the issue with ZIP alignment. To reproduce that, I ran |
@carlossanlop -- seems like 44c08b8 created some conflicts here that were a bit too involved for me to easily resolve -- can you work with @edwardneal to get those resolved and review this change, please? |
Thanks both. I've merged and verified that all's well in CI, and have managed to successfully build an Android .apk and .aab file using the new version. This is ready to review. |
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.
Thanks for the PR @edwardneal . Left some comments/questions for you to consider.
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs
Show resolved
Hide resolved
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
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
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
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Outdated
Show resolved
Hide resolved
Prove functionality where there are 3, 4, 7, 8 and 15 bytes of padding. This covers zero, one and multiple well-formed extra fields, with and without subsequent trailing data.
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Just in case, I ran the outerloop tests. If nothing concerning shows up, I'll approve and merge this. |
Converted to a clearer member which generates the combinations dynamically.
cc @jonathanpeppers this is the fix for the zipalign issue. |
I will test this here: The most recent Maestro PR didn't have this one yet: I can try it on the next Maestro PR. |
Thanks @jonathanpeppers. I tested this PR earlier, and re-tested it with the most recent preview, using a new Android project with the settings: <AndroidUseAapt2>True</AndroidUseAapt2>
<AndroidPackageFormat>apk</AndroidPackageFormat>
<AndroidSigningKeyStore>[keystore]</AndroidSigningKeyStore>
<AndroidSigningStorePass>[password]</AndroidSigningStorePass>
<AndroidSigningKeyPass>[password]</AndroidSigningKeyPass>
<AndroidKeyStore>true</AndroidKeyStore>
<AndroidSigningKeyAlias>apk</AndroidSigningKeyAlias>
<_AndroidUseLibZipSharp>false</_AndroidUseLibZipSharp> I cleaned the project, then ran
I've replaced .NET 10 Preview 3's System.IO.Compression.dll with the output from building dotnet/runtime, then cleaned the Android project again and re-ran I couldn't force the zipalign step to fail, so had to test that independently - I just ran zipalign against an appropriate ZIP file, verified that it had the unusual padding in the extra data field, then used that as the initial baseline data for the new unit tests. Are there any other tests I can run locally prior to the next Maestro PR? |
I would have thought a Your description sounds like you tested it, but I can check a |
This change hasn't shown up yet this morning: I'll try again later, this is currently dotnet/runtime/main from ~April 11. |
I just rebased this one, to test out this change: |
Contributes to #112017. Reimplements #112032. From the PR description:
The second commit enables ZipArchive to perform in-place updates of ZIP entries with slightly malformed "extra data" fields. This field must consist of [header, data] pairs, where the header is four bytes long. This isn't always adhered to though - zipalign will pad the extra data field with zeroes to make sure that the compressed data which follows is aligned to an X byte boundary.
To tolerate this, we introduce the concept of "trailing extra data" - the part of the extra data field which can't be successfully parsed as a [header, data] pair. This is detected when parsing the central directory header (and the local file header), used when calculating the "extra data length" part of the relevant header, then written out after the parsed extra data.