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

experiment with removing descriptors from zip file entries during updates #512

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Aug 19, 2020

a spin off from #475 which makes the other change suggsted there to remove descriptors from entries during updates, in cases where the header is rewritten rather than just copied directly.

Put in a seperate PR to keep the experiment seperate in case the existing change is the one that's used.

Also adds an extra test, using a zip file created by a version of ZipOutputStream tweaked to not write the signature bytes, on top of the ones created directly during the tests.

The extra tests don't call TestArchive because it thinks that entries whose descriptors don't have signature bytes are invalid, which doesn't seem correct either?

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.

@Numpsy
Copy link
Contributor Author

Numpsy commented Aug 19, 2020

Something i'm not sure about when looking at the existing code - what's the intention of the bit at

entry.Flags |= (int)GeneralBitFlags.Descriptor;
, where it enables descriptors for entries which are being encrypted - i'm not sure how that would effect the need for a descriptor?

(possibly the tests should be run with both plain and encrypted entries in case there are any differences?))

@piksel
Copy link
Member

piksel commented Aug 28, 2020

Well, just before that it checks if the CRC is present. If it isn't then it would have to be added in the descriptor. It might be the case that this couldn't be established before the crypto logic in some cases? It's a perfect example of where a comment would be really helpful.

@Numpsy
Copy link
Contributor Author

Numpsy commented Sep 8, 2020

Well, just before that it checks if the CRC is present. If it isn't then it would have to be added in the descriptor.

Would'nt that only be the case if the header can't be patched later?

@Numpsy
Copy link
Contributor Author

Numpsy commented Sep 22, 2020

In any case, any thoughts on the changed update.Command == UpdateCommand.Copy case in WriteLocalEntryHeader ?

@piksel
Copy link
Member

piksel commented Oct 3, 2020

@Numpsy I can't say that I fully understand the zipcrypto, but AFAIK it relates to how the CRC works when using encryption. The current implementation of OutputZipStream forces Descriptors on encrypted Store entries, even with a seekable stream. I have noticed this behaviour, but not investigated much further.

As for the copy, that does seem like what you would want, but the archive updating code is really hard to test correctly!

@Numpsy
Copy link
Contributor Author

Numpsy commented Oct 8, 2020

Required a bit more thought then I think, in case there are any other uninteded consequences.

(If the logic is related to ZipCrypto, that might mean there is something to think about for AES mode as well?)

@Numpsy
Copy link
Contributor Author

Numpsy commented Dec 6, 2020

I've been hoping to get back to this one, but haven't had any time recently to have a real think about it, but still hope to get back to it later.

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