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

file found but asBinary reads beyond range in nodejs #126

Closed
SheetJSDev opened this Issue Apr 30, 2014 · 5 comments

Comments

Projects
None yet
2 participants
@SheetJSDev
Contributor

SheetJSDev commented Apr 30, 2014

https://www.dropbox.com/s/w2hwdzfax6ci70y/apachepoi_46535.xlsx

The subfile 'xl/worksheets/sheet4.xml' exists and jszip confirms:

> new JSZip(fs.readFileSync('./apachepoi_46535.xlsx')).files['xl/worksheets/sheet4.xml']
{ name: 'xl/worksheets/sheet4.xml',
  _data:
   { compressedSize: 6162,
     uncompressedSize: 38279,
     crc32: -1943988139,
     compressionMethod: '\b\u0000',
     compressedContent: null,
     getCompressedContent: [Function],
     getContent: [Function] },
  options:
   { binary: true,
     optimizedBinaryString: true,
     date: Tue Jan 01 1980 00:00:00 GMT-0800 (Pacific Standard Time),
     dir: false,
     base64: false,
     compression: null } }

However, trying to read the file using .asBinary() throws an error:

> new JSZip(fs.readFileSync('./apachepoi_46535.xlsx')).files['xl/worksheets/sheet4.xml'].asBinary()
RangeError: Offset/length out of range.
    at Object.fnTyped.arraySet (c:\Users\me\Documents\GitHub\js-xlsx\node_module
s\jszip\node_modules\pako\lib\zlib\utils.js:44:12)
    at Object.inflate (c:\Users\me\Documents\GitHub\js-xlsx\node_modules\jszip\n
ode_modules\pako\lib\zlib\inflate.js:932:13)
    at Inflate.push (c:\Users\me\Documents\GitHub\js-xlsx\node_modules\jszip\nod
e_modules\pako\lib\inflate.js:176:27)
    at inflate (c:\Users\me\Documents\GitHub\js-xlsx\node_modules\jszip\node_mod
ules\pako\lib\inflate.js:277:12)
    at Object.inflateRaw (c:\Users\me\Documents\GitHub\js-xlsx\node_modules\jszi
p\node_modules\pako\lib\inflate.js:297:10)
    at Object.exports.uncompress (c:\Users\me\Documents\GitHub\js-xlsx\node_modu
les\jszip\lib\flate.js:13:17)
    at Object.getContent (c:\Users\me\Documents\GitHub\js-xlsx\node_modules\jszi
p\lib\zipEntry.js:64:52)
    at getRawData (c:\Users\me\Documents\GitHub\js-xlsx\node_modules\jszip\lib\o
bject.js:28:33)
    at Object.dataToString (c:\Users\me\Documents\GitHub\js-xlsx\node_modules\js
zip\lib\object.js:76:18)
    at Object.ZipObject.asBinary (c:\Users\me\Documents\GitHub\js-xlsx\node_modu
les\jszip\lib\object.js:125:29)

Note that this works using jszip@2.2.0 (which uses zlibjs)

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Apr 30, 2014

Collaborator

I can reproduce the issue, it comes from pako's inflateRaw. I've opened an issue on their side (nodeca/pako#22)

Collaborator

dduponchel commented Apr 30, 2014

I can reproduce the issue, it comes from pako's inflateRaw. I've opened an issue on their side (nodeca/pako#22)

@dduponchel dduponchel added the bug label Apr 30, 2014

SheetJSDev added a commit to SheetJS/js-xlsx that referenced this issue May 1, 2014

pinning jszip to 2.2.0 due to jszip bug
jszip 2.2.1 switched underlying zlib engines.  The new engine is broken.

(h/t @shawnpresser)

See:
- Stuk/jszip#126
- nodeca/pako#22

@dduponchel dduponchel referenced this issue May 1, 2014

Merged

Upgrade pako #127

@dduponchel dduponchel closed this in 9aa0af4 May 1, 2014

@SheetJSDev

This comment has been minimized.

Show comment
Hide comment
@SheetJSDev

SheetJSDev May 1, 2014

Contributor

@dduponchel a change like this (from zlibjs to pako) deserves at least a minor version bump (to 2.3.0), possibly a major version bump (3.0.0)

Contributor

SheetJSDev commented May 1, 2014

@dduponchel a change like this (from zlibjs to pako) deserves at least a minor version bump (to 2.3.0), possibly a major version bump (3.0.0)

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel May 1, 2014

Collaborator

Releasing it as a v2.2.1 was clearly a stupid idea and I apologize for that. Everyone with a dependency on ~2.2.0 automatically got the buggy v2.2.1 version, the new v2.2.2 release at least fixes the bug.

Now, we have a v2.2.2 which should have been a v2.3.1. To go to that state, we would need to revert the zlib.js -> pako migration (and get back the v2.2.0 bug), release it as v2.2.3 and revert the revert to get a v2.3.0.

I honestly don't know if a v2.3.0 release now is a good or a bad idea.

Collaborator

dduponchel commented May 1, 2014

Releasing it as a v2.2.1 was clearly a stupid idea and I apologize for that. Everyone with a dependency on ~2.2.0 automatically got the buggy v2.2.1 version, the new v2.2.2 release at least fixes the bug.

Now, we have a v2.2.2 which should have been a v2.3.1. To go to that state, we would need to revert the zlib.js -> pako migration (and get back the v2.2.0 bug), release it as v2.2.3 and revert the revert to get a v2.3.0.

I honestly don't know if a v2.3.0 release now is a good or a bad idea.

@SheetJSDev

This comment has been minimized.

Show comment
Hide comment
@SheetJSDev

SheetJSDev May 1, 2014

Contributor

@dduponchel it was my mistake to use ~2.2.0 rather than 2.2.0. That being said, there's no need to bump to 2.3.0 or change the history at this point. Just keep it in mind for next time :)

Contributor

SheetJSDev commented May 1, 2014

@dduponchel it was my mistake to use ~2.2.0 rather than 2.2.0. That being said, there's no need to bump to 2.3.0 or change the history at this point. Just keep it in mind for next time :)

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel May 2, 2014

Collaborator

it was my mistake to use ~2.2.0 rather than 2.2.0

The ~version was the default of npm install --save (now it's ^version) so it's not really a mistake, just the default settings.

Just keep it in mind for next time :)

I will !

Collaborator

dduponchel commented May 2, 2014

it was my mistake to use ~2.2.0 rather than 2.2.0

The ~version was the default of npm install --save (now it's ^version) so it's not really a mistake, just the default settings.

Just keep it in mind for next time :)

I will !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment