Set version in ZIP local header to ZIP64, when the file offset is >4GB

[karakasa]

Fix #94899. Analysis in the comments. This PR fixes some compatibility issues of ZIP64 archives with certain decompressors (such as .NET Framework).

The spec is unclear whether the versionNeeded should be same in the CD and LH, and decompressors like 7-Zip & Windows don't complain about the discrepancy. Nevertheless I found 7-Zip set the LH to version ZIP64 under such circumstance.

The test is put in the outer loop because it will create a large 5GB file.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fix #94899. Analysis in the comments.

The spec is unclear whether the versionNeeded should be same in the CD and LH, and decompressors such as 7-Zip & Windows don't complain about the discrepancy. Nevertheless I found 7-Zip set the LH to version ZIP64 under such circumstance.

Author:	karakasa
Assignees:	-
Labels:	area-System.IO.Compression
Milestone:	-

[bryanhunwardsen]

Is there a line of sight to a resolution here?

[danmoseley]

I think we thought we were waiting on @karakasa but I guess we're not. @carlossanlop does this change look reasonable to you - as you are best versed in the code/ the format?

[ericstj]

@carlossanlop - can you please review this?

[carlossanlop]

Thanks for your patience. Here's my review so far:

• I see that setting the value of _offsetOfLocalHeader a few lines earlier in WriteLocalFileHeader is safe as it is only used in the new invocation of ShouldUseZIP64().
• I can see the new ShouldUseZIP64() method has the right scope for this specific issue.
• Left some suggestions for you to consider.

Please consider:

• Addressing @danmoseley 's suggestion of adding a test case that uses compression.
• Adding an extra test to System.IO.Packaging. We should test the original reported issue as close as possible. In the src/libraries/System.IO.Packaging/tests/Tests.cs file, there is a test called VeryLargePart, which can be used as inspiration for yours. Note that VeryLargePart is creating the zip file in-memory.
• Can you please test that the generated zip file can be read with external zip tools like 7-zip, windows explorer, winzip, unzip, etc? Testing reading the generated zip file with external programming languages would also be great.

Once merged, we can backport it to any requested active servicing branches (8.0 or 6.0).

[danmoseley]

@karakasa thanks for your patience. Do you expect to be able to continue with this PR, and address the feedback?

[karakasa]

@carlossanlop

• Addressing @danmoseley 's suggestion of adding a test case that uses compression.

Sure.

• Adding an extra test to System.IO.Packaging. We should test the original reported issue as close as possible. In the src/libraries/System.IO.Packaging/tests/Tests.cs file, there is a test called VeryLargePart, which can be used as inspiration for yours. Note that VeryLargePart is creating the zip file in-memory.

May you elaborate how to achieve that? I believe the objective is essentially executing a single test, while the first part runs in the latest .NET and the second part runs in the .NET Framework. The VeryLargePart test you referenced doesn't address such problem.

• Can you please test that the generated zip file can be read with external zip tools like 7-zip, windows explorer, winzip, unzip, etc? Testing reading the generated zip file with external programming languages would also be great.

I have tested. Is replying this PR with images sufficient?

[carlossanlop]

I have tested. Is replying this PR with images sufficient?

Yes, absolutely. And thank you for doing that.

May you elaborate how to achieve that?

My suggestion is that you open src/libraries/System.IO.Packaging/tests/Tests.cs, duplicate the VeryLargePart, test, rename it, and then adjust it slightly to ensure it touches the code you're fixing. Let me know if you have additional questions.

[carlossanlop]

@karakasa please let us know if you'll be able to continue looking into this. Otherwise, I can take over.

[karakasa]

@karakasa please let us know if you'll be able to continue looking into this. Otherwise, I can take over.

Go ahead. Apologize I don’t have resources recently.

[carlossanlop]

No worries, @karakasa. Thanks anyway for helping move this along.