Skip to content
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

Fix writing extra data when updating Zip64 entries #314

Merged
merged 2 commits into from
Feb 3, 2019

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Jan 27, 2019

WriteCentralDirectoryHeader currently checks IsZip64Forced when deciding to write -1 to the base size fields ZipFile.cs#L2174 but not when writing the extra data ZipFile.cs#L2208, so the length doesn't get written.

This changes it to check in both places, and adds a test

Assuming that the length field and extra data should always be in sync (would you ever set size to -1 and not write the extra data?), might be better to set some 'wantsExtraSize' flags rather than checking ((entry.Size >= 0xffffffff) || otherFlags) in two places?

Closes #310.

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@piksel
Copy link
Member

piksel commented Jan 27, 2019

would you ever set size to -1 and not write the extra data?

No. At least not if you want to comply with the spec (APPNOTE 4.4.1.4).
It is just a recommendation to set the size to -1, but in doing so you opt in to also add the Zip64 extended length (as I interpret it).
So yeah, adding a variable is indeed better (although I prefer not to anthropomorphize code :), maybe useExtraSize).
It could of course be argued that if you want to force the zip64 field (for compliance with some inflater that only uses that field or whatever), and the size is < 65535, the non-extended size should still be the actual value.

@Numpsy
Copy link
Contributor Author

Numpsy commented Jan 27, 2019

Changed to use an extra pair of flags.

It could of course be argued that if you want to force the zip64 field (for compliance with some inflater that only uses that field or whatever), and the size is < 65535, the non-extended size should still be the actual value.

4.5.3 says that the extra fields must only appear if the corresponding record field is 0xFFFF, so I think you can't put the actual size in both fields?

@piksel
Copy link
Member

piksel commented Jan 28, 2019

Ah, I missed that. Looks good!

@piksel piksel changed the title #310 - fix writing extra data when updating Zip64 entries Fix writing extra data when updating Zip64 entries Jan 30, 2019
@piksel piksel merged commit fe9e0d8 into icsharpcode:master Feb 3, 2019
@Numpsy Numpsy deleted the zip64_testit branch February 3, 2019 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants