-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[.NET 10 Preview 1] System.IO.Compression.ZipArchive produces subtly incorrect zip headers #112017
Comments
Tagging subscribers to this area: @dotnet/area-system-io-compression |
@edwardneal seems there's a bug from the latest changes. |
It could be related to us using this file as input: packaged_resources.zip A tool named Then we add additional files and save it to disk. |
Actually, it might not be a bug. We did change the logic that writes the zip headers, @jonathanpeppers . If the files are not malformed and can be read without issues, then maybe we need to let the tool know that the mismatch is expected this time. |
Some We are trying a workaround to see if this is blocking, thanks. |
If you open the archive with different tools, can you open the individual files and read their contents normally? |
The full log prints the local header and the central header, and indeed there is only one field that does not match between the two, the
vs
|
The erroneous value seems to be 0. The possible values for runtime/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipVersion.cs Lines 6 to 13 in 0530b55
@jonathanpeppers you mention the zip file is created with the external tool Just so we have these links available for quick reach if needed, I collected the related lines of code that were changed recently: Expand meThe length of the runtime/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.FieldLengths.cs Line 98 in 0530b55
The location of the field is specified here: runtime/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.FieldLocations.cs Line 102 in 0530b55
The central directory value is read here: runtime/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs Line 641 in 0530b55
The central directory value is written here: runtime/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs Line 587 in 0530b55
Also, the shared apk.zip file weighs 6.75 MB, meaning we're not using zip64 (that code kicks in at 4GB and above). I mention this because there's a method called |
The entries that Appear to have been in the input archive created by But not all of them are incorrect, only 5 seem to be wrong? |
Okay, that might be good news from the runtime perspective, then. The recent changes may have not introduce the bug.
You're right, that's weird. The 5 files reported with warnings had a problem with the spec field. Bot for most of the entries, that field is correct. Example of an entry without issues:
@jonathanpeppers did the |
@jonathanpeppers There is another case we need to verify: Do you have access to the original archive, before it is updated by System.IO.Compression? Can you run that original file with the zipdetails tool? I ask because the new ZipArchive code will reorder and rewrite any existing entries that were modified and will also rewrite all the entries that succeed the first one that was modified. And then after that it will write the new entries at the end. I'd like to confirm the file was malformed before it was updated. |
Thanks @carlossanlop. I've checked the original and updated ZIP files. It's the combination of not rewriting the archive, and an unusual input file. The original archive has entries which are written with an Extract Zip Spec ("version needed to extract" in the spec.) field of 0x00. In the five cases we see, this isn't valid - the entry's compression methods are Deflate, and the specification says that the minimum Extract Zip Spec for this is 2.0 ( ZipArchiveEntry notes this when it's constructed and sets the Extract Zip Spec field correctly. When the Central Directory Header is written, this correct value is then written out as part of that process. The entries with incorrect Extract Zip Spec fields haven't had their contents or changed though, so they (and their local file headers containing the field value of 0x00) aren't rewritten. This is where the mismatch comes from - the CD header has been corrected, but the local file headers haven't been. I'm fairly sure that we can modify I can submit a PR fixing this tomorrow, but if that's too close to the release and I'm not sure why |
We are past code complete for Preview1. If there's no workaround for this, @jonathanpeppers , we can bring this up to Tactics and determine if we should revert the changes or take the fix or do nothing. Thanks for the confirmation @edwardneal . Please send the PR with the fix to have it ready. |
Good news: We got green light to merge a fix, @edwardneal . We still have some runway. You'd send it to |
@carlossanlop we have a valid workaround, we switched to using our old library for handling zip files: The only drawback, it has worse performance than System.IO.Compression. But I think that is ok for a preview. |
@jonathanpeppers I am ready to send an email to Tactics requesting backport approval for the fix #112032 . Do you prefer to use the workaround for preview1 (no backport) and then switch back to aapt2 + the ziparchive fix for preview2? |
It can be up to you, we are OK to leave the workaround in for preview 1. And then main/preview 2, we can verify the fix. |
Ok thanks. Let's follow the cautious approach and make the fix available in preview 2 instead. |
Changes: dotnet/sdk@aca4b81...ee4ea82 Updates: * Microsoft.NET.Sdk: from 10.0.100-alpha.1.25069.2 to 10.0.100-preview.1.25080.14 * Microsoft.NETCore.App.Ref: from 10.0.0-alpha.1.25067.10 to 10.0.0-preview.1.25078.5 (parent: Microsoft.NET.Sdk) * Microsoft.NET.ILLink.Tasks: from 10.0.0-alpha.1.25067.10 to 10.0.0-preview.1.25078.5 (parent: Microsoft.NET.Sdk) Other changes: * Default app project builds to `$(_AndroidUseLibZipSharp)=true` to avoid: error ANDZA0000: 01-30 21:38:27.669 38611 159726 W zip : WARNING: header mismatch Reported: dotnet/runtime#112017 * Update `<GitBranch/>` MSBuild task to substring long `darc-` branch names to avoid: NuGet.Build.Tasks.Pack.targets(221,5): error NU5123: Warning As Error: The file 'package/services/metadata/core-properties/027a96e260344b159133798c830dab61.psmdcp' path, name, or both are too long. Your package might not work without long file path support. Please shorten the file path or file name. Co-authored-by: Jonathan Peppers <[email protected]>
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]>
@jonathanpeppers the fix is part of preview2. Are you able to see the failure gone? Can we close this? |
Context: dotnet/runtime#112017 In .NET 10 Preview 1, when using System.IO.Compression to create `.apk` files, `zipalign` was giving the error: 01-30 21:38:27.669 38611 159726 W zip : WARNING: header mismatch To workaround, we temporarily set `$(_AndroidUseLibZipSharp)=true`. We think this is fixed now, so partially revert f3ef4fe.
I think the results now might be worse... The new error acts like the file is missing (but it is present at that path):
I think they might just print this error if unable to open the file: When I download the file and inspect,
Here is the file, I renamed the extension to |
Okay, I think we will have to revert the last fix. |
I agree - the current behaviour leaves us in a worse position than before, can we revert it from preview2? I'll try to smoke-test the Android build with my local runtime in case there are any other issues, then reattempt the fix. |
Maybe. I need to check if we have runway. I'll submit the revert to main for now, and if I get a green light, will submit the backport too.
Do you mean try again this fix to mitigate the aapt2/zipalign problem? I don't think it's worth it. We aren't really at fault here. |
@edwardneal But what about reverting to the previous behavior before the refactoring? Looks like we tried to be more correct with the refactoring but maybe we should keep the handling of the "version needed to extract" exactly the same as it was before the refactoring. |
Thanks @carlossanlop. We could do that - we'd need to revert #102704, maintaining the fix from #11802. We'd lose the memory usage improvements, but the BinaryReader/BinaryWriter removals would remain in situ (and so the API proposal for async ZipFile APIs would remain unblocked.) At the moment though, I think it'd be better to get more feedback from preview1. As of the current release, the issue is that ZipArchive doesn't gracefully handle files which were originally slightly malformed, not that it's corrupting well-formed inputs. If we see any instances of the latter, then I'm in favour of reverting. Separately: although we're not strictly at fault for the malformed ZIP file, the step to correct the header is also fairly trivial. I'd personally prefer to make ZipArchive more forgiving in this case, so that dotnet/android can use the library for .NET 10 and doesn't need to account for the version of aapt2 in use. What do you think? |
Sorry if I wasn't clear, I don't think we need to revert the memory improvements you already merged. They are all good. I agree with you that we should make the behavior more forgiving. I also agree that the change to allow that is probably trivial. So what I actually meant is that specifically for the header bug, we should try to find a way to treat the version needed to extract exactly like we used to before all these improvements were merged (and only that field, everything else is good and should stay merged). |
Thanks carlossanlop. I've spent a little time with the Android build, and can reproduce the problem with preview1 if I set When I substitute my local Release build of System.IO.Compression, I can successfully build a previously-failing Android project. All headers match, the output passes a test in 7-Zip. Besides testing the change when building an Android project, I've also created a new .apk file with The second problem we're dealing with is related to Previously, ZipArchive would have silently ignored this malformed data; only well-formed ZipExtraField structures would have been written out to the backing stream upon disposal. For unmodified entries, we now try to seek past the local file header and file data. We work out how many bytes forward to seek by summing up the lengths of all well-formed ZipExtraField structures - and this is where the problem lies. The X bytes of malformed extra data aren't accounted for. As a result, we find that the field in the central directory header which stores the offset of the local file header falls slightly out of sync with the stream's position. This shows up in #113306 will reintroduce the previously-reverted change and fix the first problem. I think a fix for the second problem is a few days away though. |
Thanks @edwardneal. I'll review your PR. |
Description
We noticed this here:
We use System.IO.Compression.ZipArchive to create Android
.apk
files (and other archives).When signing an
.apk
with Android'szipalign
tool, it reports:However, if I inspect the actual file, some tools are ok with it:
So, then we tried:
Full output: https://gist.github.com/grendello/3007335dba59383ac7782311d262e844
Reproduction Steps
dotnet new android
dotnet build
Expected behavior
ZipArchive
produces files that do not cause Android tooling to produce errors.Actual behavior
ZipArchive
produces files that cause Android tooling to produce errors.Regression?
Yes
Known Workarounds
We are attempting a flag we have
$(_AndroidUseLibZipSharp)=true
, that will fall back to an open-source zip library we used to use.Configuration
.NET SDK: 10.0.100-preview.1.25079.13
.NET runtime: 10.0.0-preview.1.25078.5
Archive: com.companyname.DotNetNewandroid.apk.zip
Other information
No response
The text was updated successfully, but these errors were encountered: