-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
isPasswordValidForArchiveAtPath returning wrong result if first file is directory #462
base: main
Are you sure you want to change the base?
Conversation
It turned out the problem occurs when the first file in the archive is an empty directory. The call to unzReadCurrentFile in this case will be given a len parameter of 0 which results in readBytes being 0 and the method returning YES. |
Tried to fix the problem by skipping over directories, i don't know enough about Zipfiles to be sure i didn't miss any cases. (i.e. is it 100% safe to read the filename and check for the ending character for each and every file?) Please review. |
Another solution might be just to check for readBytes > 0 ... But i actually have no clue which of the solutions is more failsafe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're allocating filename, but you have some early returns which don't get call free(filename).
Possible concerns:
We should explore those and see if we can come up with some better fix. |
Sorry, i must ask you for some patience, i will not be able to review your comments and respond for the next two weeks. I will try to respond asap thereafter.
|
* master: Bump version to 2.1.4 unzipping a directory doesn't require a password Compatibility with CocoaPods module for Swift. pod update handle empty directory path gracefully when keepParentDirectory is YES optimization, swifter empty status of a directory updating minizip: * avoid buffers overwrite (ZipArchive#438) * avoid that the applications continue to read incorrect data when wrong password is entered. (zlib-ng/minizip-ng#210) optimization, judge the empty status of a directory, perform a shallow search instead of a deep enumeration workaround for "Malformed version number string" with CocoaPods use macro ZIP_OK instead of UNZ_OK, in -[SSZipArchive close]
I've addressed the memory leak you had in filename, but it still leaves my other concerns, notably a test file to verify this behavior. |
Thanks for fixing the memory issues! I reproduced and tracked down the problem today, this is what i found: 1.) The problem is not reproducible in v2.1.4 because 2.) The problem only occurred with zip files generated by SSZipArchive itself because with SSZipArchive (empty) directories zipped with password get the "flag & 1" set. Although i discovered, if zip files are created with the "zip" command, only files (not directories) seem to get this flag set. Therefore, i'm not quite sure if the behavior described in 1.) is correct. For files zipped from command line I currently do have a zip file which reproduces the problem with v2.1.3 of SSZipArchive and added some test cases, but as the problem is gone now... do you still want those unit tests? I will push those testcases onto a new branch in my fork, if you want them i can add a new pull requests only containing the test cases... |
for unit tests and test files see 5b5@2a4144c If you think the behavior described in 1.) above is correct, please close this PR. |
I meant to add test cases if there was something broken in current version (2.1.4). We should focus on attempting to make a legit password-protected archive with a different tool (comment line, WinZip, Keka, etc.) and some empty folders/files to see the password detection is working fine in all scenarios. |
As i described above, when a password-encrypted zip file is generated by SSZipArchive it will differ from an encrypted archive zipped from command line (even in 2.1.4) in having the encryption flag set on directory entries, whereas the flag is not set for directory entries in the file generated from command line. I just wanted let you know that there is a difference in the results between both as i don't know which one is the right method. (I had a look into the ZIP-Spec but i didn't find a clear answer and i am not at all familiar with the Zip-file format details.) It MIGHT be a bug in the current version even if it doesn't lead to broken unit-tests as both files work together with I hope this made things a bit clearer. I cannot help you much further at the moment because i already spent about 7 hours on investigating this issue and i'm quite busy at the moment. |
Hi,
i suppose the method was not meant to exit with YES at this point but only when the while loop has finished? I recognized that the method did exit with YES even when the wrong or no password was given for password encrypted files, but only for some files. With this fix it works for my files. Maybe the assumptions in this part of the code are wrong? At least in my case it wasn't sufficient to test the first file though.
Cheers & thank you for your work!