4 ms·

I don't understand why this program should fail, though, if other tools can unzip the file just fine. I specifically wonder about this part of the code [0]:

by arh68 10y ago

I don't understand why this program should fail, though, if other tools can unzip the file just fine. I specifically wonder about this part of the code [0]: if (commentLength !== expectedCommentLength) { return callback(new Error("invalid comment length. expected: " + expectedCommentLength + ". found: " + commentLength)); } Why not just determine this isn't the record, keep looking, and handle the error after the loop? if (commentLength !== expectedCommentLength) { continue } [0] https://github.com/thejoshwolfe/yauzl/blob/master/index.js#L124-L126 https://github.com/thejoshwolfe/yauzl/blob/master/index.js#L...

bonzini 10y ago
This would not fix the problem. The issue is that there is extra content after the comment (for what we know there could be no comment at all, actually). The library expected the comment to end right at the end of the file.
x1798DE 10y ago
I think the argument would be that if you write a non-standards-compliant unzip utility, in many cases people generating malformed zip files won't get any warning because the software "knows what you meant to do", then people emitting malformed zip files with a random amount of data at the end won't know that they're doing anything wrong until one day they hit the limit of what is acceptable, way down the road and after they've been emitting malformed zip files for ages. I think it's the programming equivalent of not telling someone they have some food on their face - yeah, it's no skin off your back if you don't tell them, but who knows who they're going to run into later. That said, a hybrid approach which fails by default but then offers a way to "force unzip" the file in a non-standards-compliant way might be the best of both worlds here, because the users get an immediate way to fix the problem and the emitters are likely to be notified that they are creating malformed zips.
- TAForObvReasons 10y ago
  The problem is that the ZIP format is not a full-file specification: it permits data before and after the logical extents of the ZIP archive. Among other things, it allows for self-extracting archives by prepending the extractor code. the library in this case is erroneously assuming that the archive spans the entire file.
- jonahrd 10y ago
  I think you're missing arh68's point. If it's supposed to find the magic number, and by definition the comment is allowed to contain the magic number, then this section of the code should (correctly) diagnose that this is not the magic number, but (incorrect in the current implementation) then continue to search backward for the REAL magic number.
  - Drdrdrq 10y ago
    The way I understood it magic number is not allowed in the comment.
grkvlt 10y ago
Reading that code and seeing the multi-disk (non) support [0] brought back awful memories of .zip files spanning ten or twenty disks which you would feed in, one by one, until you god the dreaded 'zip file corrupted' error on disk N-1, almost inevitably. Meaning you had to try and re-download the data again, over your trusty 19K2 link... [0] https://github.com/thejoshwolfe/yauzl/blob/master/index.js#L112 https://github.com/thejoshwolfe/yauzl/blob/master/index.js#L...

by arh68 10y ago

bonzini 10y ago

This would not fix the problem. The issue is that there is extra content after the comment (for what we know there could be no comment at all, actually). The library expected the comment to end right at the end of the file.

x1798DE 10y ago

I think the argument would be that if you write a non-standards-compliant unzip utility, in many cases people generating malformed zip files won't get any warning because the software "knows what you meant to do", then people emitting malformed zip files with a random amount of data at the end won't know that they're doing anything wrong until one day they hit the limit of what is acceptable, way down the road and after they've been emitting malformed zip files for ages. I think it's the programming equivalent of not telling someone they have some food on their face - yeah, it's no skin off your back if you don't tell them, but who knows who they're going to run into later. That said, a hybrid approach which fails by default but then offers a way to "force unzip" the file in a non-standards-compliant way might be the best of both worlds here, because the users get an immediate way to fix the problem and the emitters are likely to be notified that they are creating malformed zips.

TAForObvReasons 10y ago
The problem is that the ZIP format is not a full-file specification: it permits data before and after the logical extents of the ZIP archive. Among other things, it allows for self-extracting archives by prepending the extractor code. the library in this case is erroneously assuming that the archive spans the entire file.
jonahrd 10y ago
I think you're missing arh68's point. If it's supposed to find the magic number, and by definition the comment is allowed to contain the magic number, then this section of the code should (correctly) diagnose that this is not the magic number, but (incorrect in the current implementation) then continue to search backward for the REAL magic number.
- Drdrdrq 10y ago
  The way I understood it magic number is not allowed in the comment.

grkvlt 10y ago

Reading that code and seeing the multi-disk (non) support [0] brought back awful memories of .zip files spanning ten or twenty disks which you would feed in, one by one, until you god the dreaded 'zip file corrupted' error on disk N-1, almost inevitably. Meaning you had to try and re-download the data again, over your trusty 19K2 link... [0] https://github.com/thejoshwolfe/yauzl/blob/master/index.js#L112 https://github.com/thejoshwolfe/yauzl/blob/master/index.js#L...