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

Handling folder entries for Java programs #33

Merged
merged 3 commits into from
Feb 26, 2013
Merged

Handling folder entries for Java programs #33

merged 3 commits into from
Feb 26, 2013

Conversation

feugy
Copy link
Contributor

@feugy feugy commented Feb 14, 2013

Hello !

We found that Java programs cannot read zip generated by JSZip.
It appears that using the DEFLATE compression on folder entries with empty content cause the Java zip inflator to fail (error 'invalid stored block lengths').

This fix totally remove content when compressing a folder entry, and set compression to none for this entry. All tests passed, without modification on them.

Thank you !

Damien.

Using the DEFLATE compression on folder entries with empty content cause the Java zip inflator to fail.
This fix totally remove content when compressing a folder entry, and set compression to none for this entry.
if (!this.options.binary) {
result = JSZip.prototype.utf8encode(result);
}
return result;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you fix the indentation here to match the rest of the file (3 spaces)

@feugy
Copy link
Contributor Author

feugy commented Feb 14, 2013

Indentation fixed !

@@ -92,6 +92,9 @@ JSZip.prototype = (function () {
*/
asBinary : function () {
var result = this.data;
if (result == null) {
return result;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the same check in the asText method ?

@dduponchel
Copy link
Collaborator

Thanks for the pull request !
I reproduced the bug (even unzip -t was complaining) and your patch fixed it. Just add the check in the asText method and it's OK for me :-)

Check data nullity in asText() to avoid errors
@feugy
Copy link
Contributor Author

feugy commented Feb 15, 2013

And done !

@dduponchel
Copy link
Collaborator

I just found a case where the bug is still here.
When loading a zip file (and then generating it), the folder is still marked as using the DEFLATE compression.

I think that in the fileAdd function,

if (o.dir) {
   data = null;
} else if (JSZip.support...

will cover all the corner cases.

What do you think of it ?

@feugy
Copy link
Contributor Author

feugy commented Feb 16, 2013

I havn't tested yet (I'll do it on monday), but your fix seems right.
I wonder also if jszip-load handle no-data as well : I haven't test loading specifically.

@dduponchel
Copy link
Collaborator

jszip-load handles empty files : it will just INFLATE an empty string (and get an empty string).

But you found an other failing test case : a compressed zip with an empty file.

unzip don't like compressed empty files :

    testing: Hello.txt               
  error:  invalid compressed data to inflate

We should STORE entries with no content (folders of files).

@dduponchel
Copy link
Collaborator

Adding the condition data !== '' (with the data !== null) seems enough to fix the issue.

If you add a fix for this (I can't add a commit in your pull request), I'll merge this to completely fix the issue.

@dduponchel
Copy link
Collaborator

This pull request doesn't fix all the cases but doesn't break anything. I'm merging it (without reply from @feugy) and I'll create an other pull request to fix the remaining issues.

dduponchel added a commit that referenced this pull request Feb 26, 2013
Handling folder entries for Java programs
@dduponchel dduponchel merged commit 3d64219 into Stuk:master Feb 26, 2013
dduponchel added a commit to dduponchel/jszip that referenced this pull request Feb 26, 2013
If the user add an empty file (or a folder), we should STORE the file
and not DEFLATE it.
Stuk added a commit that referenced this pull request Feb 27, 2013
Fix the failing case of the issue #33
@feugy
Copy link
Contributor Author

feugy commented Mar 2, 2013

Hi David, Stuart.

Sorry not to have replied: I was sick for a few days and just after away from an internet connection.
I'll soon use our combined work in my projects !

Thank you again.

@dduponchel
Copy link
Collaborator

Glad to see that you're OK !

dduponchel added a commit to dduponchel/jszip that referenced this pull request Mar 3, 2013
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.

3 participants