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?
| long currentPosition = fs.Position; | ||
|
|
||
| // Confirm it's initially set to the correct value | ||
| using (BinaryReader reader = new(fs, Encoding.UTF8, leaveOpen: true)) |
There was a problem hiding this comment.
You can open the reader and writer at the same time, and just move the position back 2 bytes after you read it. That makes this more reabable and should reduce the code a bit.
There was a problem hiding this comment.
I think it is more important to be able to close the ZipArchive, dispose it, then reopen it from the same stream, and confirm that the ZipArchive can be opened, the ZipArchiveEntry can be created without unexpected errors, and finally confirm the zip64 value was unmodified.
If I reduce the code as suggested, that would mean using the exact same ZipArchive object, without really verifying that the ZipArchive and the ZipArchiveEntry instances could still be created successfully.
|
|
||
| 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.
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: