corrupt zip file generation #32

Closed
inolen opened this Issue Jun 15, 2013 · 14 comments

Comments

Projects
None yet
4 participants
@inolen

inolen commented Jun 15, 2013

First off, thanks for the hard work, it was great to find an archive module that functions well with node's streams.

However, after getting setup, I realized I was having an issue on one of my linode boxes where corrupt zip archives were being generated. I didn't have the time right now to track down why they were corrupt, but I did isolate what was causing it, and I stripped down part of the app to what was breaking:

https://gist.github.com/inolen/5789563

Most importantly, this was the culprit:

https://gist.github.com/inolen/5789563#file-gistfile1-js-L60

The eachLimit functions exists to allow at max limit number of iterators running at any one time. I had originally throttled the each to avoid issues related to opening too many file descriptors, but while reviewing the code (in order to track down the corruption issue) I realized it was silly and just executed each append sequentially using async's eachSeries.

To my surprise, this fixed the issue. Now again, this wasn't affecting my local machine, but was affecting my production linode box; it's 100% reproducible there.

I don't have time right now to dig into the issue, but I wanted to leave this issue in case you wanted to poke around and see if something is obviously wrong (nothing immediately jumped at me scrolling through zip.js and core.js). If not, I'll try and circle back to this in the next few weeks.

Edit: if it helps, both my local and the remote machines are running v0.10.11.

@inolen

This comment has been minimized.

Show comment Hide comment
@inolen

inolen Jun 15, 2013

To note, the corrupt archives all had the same file size, but a hex comparison showed that there were several places in the archive where data had been shifted around. I assumed that chunks of each stream were perhaps being incorrectly interleaved.

inolen commented Jun 15, 2013

To note, the corrupt archives all had the same file size, but a hex comparison showed that there were several places in the archive where data had been shifted around. I assumed that chunks of each stream were perhaps being incorrectly interleaved.

@ctalkington

This comment has been minimized.

Show comment Hide comment
@ctalkington

ctalkington Jun 15, 2013

Member

you may want to listen for the close event of output stream (to do your callback), as finalize doesn't guarantee that your streams have finished dealing with the data sent to them.

Member

ctalkington commented Jun 15, 2013

you may want to listen for the close event of output stream (to do your callback), as finalize doesn't guarantee that your streams have finished dealing with the data sent to them.

@ctalkington

This comment has been minimized.

Show comment Hide comment
@ctalkington

ctalkington Jun 15, 2013

Member

since archiver can handle being passed files right after each other and then dealing dealing with it internally 1 by 1. you really shouldn't need async at all on that part and there is a nice little helper for lazy loading streams that should ease open file limits.

https://npmjs.org/package/lazystream

Member

ctalkington commented Jun 15, 2013

since archiver can handle being passed files right after each other and then dealing dealing with it internally 1 by 1. you really shouldn't need async at all on that part and there is a nice little helper for lazy loading streams that should ease open file limits.

https://npmjs.org/package/lazystream

@inolen

This comment has been minimized.

Show comment Hide comment
@inolen

inolen Aug 17, 2013

@ctalkington I just had time to look into this finally, and you were 100% correct about waiting for events on the output stream. Thanks!

inolen commented Aug 17, 2013

@ctalkington I just had time to look into this finally, and you were 100% correct about waiting for events on the output stream. Thanks!

@inolen

This comment has been minimized.

Show comment Hide comment
@inolen

inolen Aug 17, 2013

Actually, I lied, still hitting some issues. They appear to be the same as ctalkington#38

inolen commented Aug 17, 2013

Actually, I lied, still hitting some issues. They appear to be the same as ctalkington#38

@ctalkington

This comment has been minimized.

Show comment Hide comment
@ctalkington

ctalkington Aug 17, 2013

Member

yah, i really need to get a test environment for Mac so I can see whats going on as I have been unable to reproduce to this point.

Member

ctalkington commented Aug 17, 2013

yah, i really need to get a test environment for Mac so I can see whats going on as I have been unable to reproduce to this point.

@ctalkington ctalkington reopened this Aug 18, 2013

@ctalkington

This comment has been minimized.

Show comment Hide comment
@ctalkington

ctalkington Aug 18, 2013

Member

bit of update from discoveries, this corruption seems to happen more when you are writing multiple zips at once and only when the source is a buffer and goes through zlib.

i messed with this some more, it appears if JUST the line to checksum is removed from the DeflateRawChecksum, it doesn't go crazy and start overwriting itself.

Member

ctalkington commented Aug 18, 2013

bit of update from discoveries, this corruption seems to happen more when you are writing multiple zips at once and only when the source is a buffer and goes through zlib.

i messed with this some more, it appears if JUST the line to checksum is removed from the DeflateRawChecksum, it doesn't go crazy and start overwriting itself.

@ctalkington

This comment has been minimized.

Show comment Hide comment
@ctalkington

ctalkington Aug 18, 2013

Member

also to be noted this corruption also happens randomly and some times its none, one, two, or all the zips that end up corrupt. It almost makes me feel like its a global leak or there is some memory corruption going on from all the buffers being created. yet that doesn't really fit with the DeflateRawChecksum line removal fixing it...

EDIT: after the latest commits the DeflateRawChecksum line removal seems to have been a fluke as its back to erroring. I think its still related to something with buffers.

Member

ctalkington commented Aug 18, 2013

also to be noted this corruption also happens randomly and some times its none, one, two, or all the zips that end up corrupt. It almost makes me feel like its a global leak or there is some memory corruption going on from all the buffers being created. yet that doesn't really fit with the DeflateRawChecksum line removal fixing it...

EDIT: after the latest commits the DeflateRawChecksum line removal seems to have been a fluke as its back to erroring. I think its still related to something with buffers.

@ctalkington

This comment has been minimized.

Show comment Hide comment
@ctalkington

ctalkington Aug 18, 2013

Member
Member

ctalkington commented Aug 18, 2013

@ctalkington

This comment has been minimized.

Show comment Hide comment
@ctalkington

ctalkington Aug 18, 2013

Member

during testing, it was found that a higher internal buffer highWaterMark seemed to reduce the occurrence of the issue cropping up. i will be releasing a bugfix in the next day with such a change as a temporary solution.

Member

ctalkington commented Aug 18, 2013

during testing, it was found that a higher internal buffer highWaterMark seemed to reduce the occurrence of the issue cropping up. i will be releasing a bugfix in the next day with such a change as a temporary solution.

@ctalkington

This comment has been minimized.

Show comment Hide comment
@ctalkington

ctalkington Aug 18, 2013

Member

ok, im hoping the latest release eliminates the issue. i spent way too much trying to track down why deflate.pipe was causing such a random race condition so I've moved it back to event listeners for the time being.

Member

ctalkington commented Aug 18, 2013

ok, im hoping the latest release eliminates the issue. i spent way too much trying to track down why deflate.pipe was causing such a random race condition so I've moved it back to event listeners for the time being.

@sokra

This comment has been minimized.

Show comment Hide comment
@sokra

sokra Sep 11, 2013

It does fix my issue with corrupt zip files... Thanks 😄

sokra commented Sep 11, 2013

It does fix my issue with corrupt zip files... Thanks 😄

@ctalkington

This comment has been minimized.

Show comment Hide comment
@ctalkington

ctalkington Sep 20, 2013

Member

closing. please open new issue if issue remains.

Member

ctalkington commented Sep 20, 2013

closing. please open new issue if issue remains.

@reid

This comment has been minimized.

Show comment Hide comment
@reid

reid Oct 7, 2013

Thank you @ctalkington for researching and fixing this! <3

reid commented Oct 7, 2013

Thank you @ctalkington for researching and fixing this! <3

caridy pushed a commit to caridy/yui3 that referenced this issue Oct 30, 2013

Upgrade archiver to 0.4.10.
Recent versions of archiver fix bugs that can generate corrupt ZIPs,
so upgrade to the most recent version of archiver that also includes
a fix for this bug:

archiverjs/node-archiver#32

Keep specifying a specific archiver version to prevent glitches
caused by using different older or newer versions.

@ctalkington ctalkington referenced this issue in sindresorhus/gulp-zip Feb 18, 2014

Closed

Invalid zip archive produced #7

@mortengf mortengf referenced this issue in Scripler/scripler Sep 7, 2014

Closed

Downloading EPUB sometimes fails #391

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