RangeError in buffer.js due to error at index.js:283

[downwa]

I was getting the following stack trace while unzipping a file:

buffer.js:620
throw new RangeError('index out of range');
^

RangeError: index out of range
at checkOffset (buffer.js:620:11)
at Buffer.readUInt16LE (buffer.js:666:5)
at /usr/local/lib/node_modules/yauzl/index.js:286:41
at /usr/local/lib/node_modules/yauzl/index.js:474:5
at /usr/local/lib/node_modules/yauzl/node_modules/fd-slicer/index.js:32:7
at FSReqWrap.wrapper as oncomplete

I tracked it down to line 283:
while (i < extraFieldBuffer.length) {

which should instead be at least:
while (i+4 < extraFieldBuffer.length) {

to avoid attempting to read past the end of the buffer at line 284 and 285.

However, I think you should add another check before line 289 to ensure the extraFieldBuffer.copy does not fail due to an invalid size field in the zip file. That is, if it is invalid, you should throw a descriptive exception rather than letting it be handled by another RangeCheck error (which doesn't explain the problem very well to the casual user of a damaged zip file).

(Note that there appears to be no easy way to trap this exception since it occurs within FSReqWrap wrapper).

[thejoshwolfe]

good catch. you've found a bug. I'll look into fixing this and creating test cases for it.

and thanks for your detailed analysis. you've made my job that much easier :)

[thejoshwolfe]

fix published in version 2.4.3. please let me know if you run into any other issues.

[downwa]

Thank you for the improvement. However, it appears the zip files I'm trying to unzip are (though out of spec), a common one "in the wild", Android .apk files.
See this discussion where another (Python) implementation dealt with the same issue: [(https://bugs.python.org/issue14315)]

I think the software needs to be lenient in a couple ways.

1. An extra field with length of 3 or less should be ignored (no room for headerId and dataSize, but rather than crashing, just ignore it). There appear to be zip generators which use the extra field for alignment or padding of some sort, and these uses are handled in many other zip readers by ignoring the field.

2. An extra field with a length of 4 (large enough to hold a valid headerId and dataSize), but a dataSize of zero, should also be ignored. Java .jar files may contain extra fields of this type with a headerId of 0xCAFE and dataSize of zero.

If you need example zip files I can send them, but hopefully the above description is sufficient to provide a workaround for out-of-spec but common zip files such as .apk and .jar. If you don't think always ignoring the out-of-spec headers is good, at least provide an option to ignore.

[thejoshwolfe]

You make some good arguments. Reopening.

[thejoshwolfe]

I double checked the zipfile spec to see if padding was allowed in the Extra Field, and I came to the same conclusion as this, which is that the padding is not allowed, and .apk files routinely violate the zipfile spec. that being said, tolerating some clearly fenced garbage in a zipfile isn't some big compromise of correctness for the rest of the interpretation; it's pretty obvious how to recover from the corrupt zipfile in this case. in fact, the change to yauzl would be mostly comprised of deleting code, and ending up with what you've been saying all along: iterating to .length - 4. So yauzl should turn a blind eye to padding in the Extra Field.

I was considering adding a flag to allow/disallow the tolerance (just like the linked python issue considered), but I decided that in the grand scheme of things it wouldn't be worth it. Adding a flag would allow some yauzl users to do a sanity check on one tiny aspect of the zipfile format, and given that yauzl is so far from comprehensive sanity checks already, it wouldn't make sense. For example, yauzl completely ignores local file headers (also, just like the linked python issue mentioned).

[thejoshwolfe]

I tested parsing the bla.apk attachment on the linked python issue, and this commit makes it work. published as version yauzl 2.5.0.

as before, please let me know of anything else i can do to help.

[thejoshwolfe]

2. An extra field with a length of 4 (large enough to hold a valid headerId and dataSize), but a dataSize of zero, should also be ignored. Java .jar files may contain extra fields of this type with a headerId of 0xCAFE and dataSize of zero.

Just to follow up on this, yauzl does not ignore these, but you can in your application if you want. yauzl says there is an extra field with id of 0xcafe and a data buffer of length 0. I think that's a little better than ignoring it entirely.

[downwa]

Works for me. Thanks!