Force write of local file header when "version needed to extract" changes #112032
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-io-compression |
There was a problem hiding this comment.
Thanks for the fix, @edwardneal! Left some nit comments and a non blocking question, but otherwise the fix LGTM.
aapt2 creates ZIP entries with a "version needed to extract" of 0.0, but sometimes writes these entries with deflate compression (which requires a "version needed to extract" of 2.0.)
This sentence in the description makes it sound like the bug is in aapt2. My understanding from your explanation in the original issue was that this is a bug in ZipArchive. Or is it a bug in both?
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.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
Show resolved
Hide resolved
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). |
My understanding is that the bug is in both. AAPT2 uses the Deflate algorithm and doesn't correctly set the "version to extract" field. ZipArchive corrects the "version to extract" to a valid value and updates the central directory, but fails to update the locally prefixed header for the corrected entries. |
Adding/correcting comments in test, and slightly reducing the diff in ZipArchiveEntry.
|
Thanks @carlossanlop, I've addressed your review comments.
filipnavara is correct, it's a bug in both. The ZIP file being generated in aapt2 is malformed, but ZipArchive has a bug because it's correcting the central directory header without doing the same for the local file header. |
|
Lot's of unrelated failures. We rollbacked some helix machine updates due to reported generalized failures. I'll update the branch so the CI runs again. |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Changes: dotnet/sdk@aca4b81...d6bc791 Changes: dotnet/runtime@6c58f79...e51af40 Updates: * Microsoft.NET.Sdk: from 10.0.100-alpha.1.25069.2 to 10.0.100-preview.2.25102.3 * Microsoft.NETCore.App.Ref: from 10.0.0-alpha.1.25067.10 to 10.0.0-preview.2.25101.4 * Microsoft.NET.ILLink.Tasks: from 10.0.0-alpha.1.25067.10 to 10.0.0-preview.2.25101.4 Other changes: Context: dotnet/runtime#112017 Context: dotnet/runtime#112032 * Default to `$(_AndroidUseLibZipSharp)=true` to temporarily workaround a `System.IO.Compression.ZipArchive` issue. In a future PR, we'll revert this change to use System.IO.Compression where appropriate. Co-authored-by: Jonathan Peppers <[email protected]>
Contributes to #112017.
aapt2creates ZIP entries with a "version needed to extract" of 0.0, but sometimes writes these entries with deflate compression (which requires a "version needed to extract" of 2.0.)When a ZipArchive is opened in Update mode and a new item is added, the central directory is written with the correct value for this field. However, the local file header for existing entries isn't rewritten (because it hasn't changed.) This leads to a mismatch between the local file header values and the central directory header values, which causes Android app builds to fail.
When an existing entry's "version needed to extract" is changed, we force its local file header to be rewritten.
There's a test to cover this. I've also added an entry to the packaged_resources.zip file from the issue and confirmed that the field matches between the local file header and the CD header for all compressed entries in the resultant file.
@carlossanlop @jonathanpeppers