Add more ZipArchive tests to confirm the Zip64 fix does not affect reading zip files generatd with the old bug #103152
Conversation
…y value, then confirms it can still be opened using the fixed ZipArchive code.
…ge zip archive with the zip64 fix, and another one that is run using .NET Framework which reads the buggy zip file and confirms it can be read without problems.
|
Tagging subscribers to this area: @dotnet/area-system-io-compression |
| using FileStream fs = File.OpenRead(zipArchivePath); | ||
|
|
||
| // Open archive to verify that we can still read an archive with the buggy version | ||
| using (ZipArchive zip = new ZipArchive(fs, ZipArchiveMode.Read)) |
There was a problem hiding this comment.
The bug in NETFX was actually with the validation in System.IO.Packaging. So this would pass even with the broken zip.
I'm not sure there's value in having this test committed even if we made it for IO.Packaging, since it has manual steps.
Maybe there's some value in having some suite of compatibility tests that produce different zips / IO.Packaging packages on one framework, and then consume them on the other framework without intervention? Perhaps those could be done in a way that they are a theory based on a number of different scenarios? I don't see that as a necessary thing for this particular fix.
There was a problem hiding this comment.
Do we have prior art running two different test processes for different frameworks?
There was a problem hiding this comment.
Not different processes, but we do have compatibility tests, where we check in data for use in testing. We also have other tests that can ensure checked-in data matches that which is currently generated. Doing so can make sure we keep the checked in data up to date on one framework and test with it on a different framework. Maybe that's infeasible here since these files are gigantic?
|
|
||
| fs.Position = 0; | ||
|
|
||
| // Open archive to modify the bit as it used to look before fix |
There was a problem hiding this comment.
I noticed the source has a define for DEBUG_FORCE_ZIP64. I don't see us ever use that, but I imagine the idea was we could use reflection to set that field and it would make a smaller zip use Zip64. That might be an interesting method to test this without forcing things to outerloop.
There was a problem hiding this comment.
Good to keep it in mind. I wasn't aware that it could be used for testing. At the moment I felt that it was more important to confirm that the real life scenario is working. I'll consider using the define in the future.
There was a problem hiding this comment.
Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.
Files not reviewed (2)
- src/libraries/System.IO.Compression/tests/Manual/System.IO.Compression.Manual.Net.Tests.csproj: Language not supported
- src/libraries/System.IO.Compression/tests/Manual/System.IO.Compression.Manual.NetFramework.Tests.csproj: Language not supported
Comments suppressed due to low confidence (1)
src/libraries/System.IO.Compression/tests/ZipArchive/zip_LargeFiles.cs:144
- [nitpick] Consider renaming 'buggyZip64Version' to 'legacyZip64Version' for better clarity.
ushort buggyZip64Version = 20;
|
@carlossanlop I'm not sure what you wanted to do with this PR. One piece of blocking feedback I had was that the manual regression test added here wouldn't have reproduced the bug on NetFx since it was testing the IO.Compression implementation of Zip and not the WPF implementation in IO.Packaging. |
|
Closing based on feedback. |
We recently fixed a bug in ZipArchive that prevented setting the zip64 bit corectly in the local header for very large files: #102053
This PR adds two more tests to confirm the above test works: