mattconnolly / ZipArchive

zip archive processing for Cocoa - iPhone and OS X
http://code.google.com/p/ziparchive/
MIT License
841 stars 260 forks source link

Using wrong password to test error case #10

Closed lordmacintosh closed 11 years ago

lordmacintosh commented 12 years ago

Hi, great work Matt!

I had a question for you about unzipping with a password. I am able to zip with a password, and unzip with the password without any problem.

I tried an error case using a wrong password to unzip. I expected to receive an indication that the unzip was unsuccessful, but I get a return of success. As expected, the zipped files are not actually unzipped (which is good), however, the directory now holds a bunch of zero byte files with the correct file names that were in the zip. Seems strange.

Is this expected, or should there be a return somewhere indicating an invalid password?

Thx! Anthony

mattconnolly commented 12 years ago

I've just reproduced that and wouldn't call it expected behaviour.

mattconnolly commented 12 years ago

How does this work for you? I made it create the file at the first successful read (which could be for a zero length file, since that is a possibility).

lordmacintosh commented 12 years ago

Definitely better. I confirm your change fixed the creation of zero length files. However, the unzip process still returns success, making my app layer believe the unzip was successful even though it was not. At what point, can it be determined that the unzip has failed due to bad password? Thanks for looking at this!

mattconnolly commented 12 years ago

Ah. Now it does call the error callback on the delegate. Do you think it should return failed of one or any files couldn't be extracted?

lordmacintosh commented 12 years ago

Lets say I have 100 files that were zipped with password. If a bad password is used for the unzip, then I receive 100 delegate calls to the delegate method "ErrorMessage:" with a failure of "Failed to reading zip file"

I would prefer one of the following:

  1. I do not implement the delegate method, and instead I rely on the "UnzipFileTo:(NSString_) path overWrite:(BOOL) overwrite" method return NO to indicate that a failure has occurred
  2. or, I implement the delegate so I know about the failure, and have the method "UnzipFileTo:(NSString_) path overWrite:(BOOL) overwrite" abort after the first file fails

What are your thoughts?

Currently, I believe I would do the following with the current implementation: make a call to "UnzipFileTo:(NSString*) path overWrite:(BOOL) overwrite" and if I get a delegate callback then I set an error flag. Later, when the UnzipFileTo method returns I check if the error flag was set or if the method returned a NO. if either is true, then the zip was unsuccessful.

mattconnolly commented 12 years ago

How about we an "abortOnError" property which when set will cause any operating to stop at the first error.

And fix the return result of "UnzipFileTo:" to be NO if ANY of the files fail. ie: YES means a completely successful operation. I'll do that tonight.

lordmacintosh commented 12 years ago

Hi Matt - Here is one solution, I believe that if you put success = NO where the error happens: [self OutputErrorMessage:@"Failed to reading zip file"]; success = NO; and then check for success in the top layer loop, it will break out on the first error: while( ret==UNZ_OK && UNZ_OK!=UNZ_END_OF_LIST_OF_FILE && NO!=success);

Some other thoughts: I thought about adding an UNZ_BAD_PASSWORD for ret status at the point of failure, but ret gets set after that point, so that isn't very clean.

Perhaps the addition of a new method for passwordIsCorrect that return BOOL Yes on success. This small check would be performed by the app layer before trying to unzip the whole file.

Just a few thoughts..

Thanks again!

mattconnolly commented 11 years ago

I've expanded a bit on your proposed solution and ensured that it returns NO when it fails. Also added some tests for it too.

Does this work for you now?

mattconnolly commented 11 years ago

Closing due to no responses for a few months.