finish/close event emitted after error event and then crashes

[rhodgkins]

Both the finish and close events get emitted after the error event (as FILE_ENDED) with the following ZIP file. Then once this has happened a TypeError is thrown from:

node-unzipper/lib/PullStream.js

Line 59 in a0efc16

if (self.buffer.length === (eof.length || 0)) self.cb();

I'm not sure whether or not finish (or close) should be emitted if an error occurs - to my mind they shouldn't, but I can't work out why this ZIP is causing the crash.

I've managed to stop the crash (but not the multiple events) by checking __ended before calling the write's cb() - have no idea if this is the correct fix:
https://github.com/bookcreator/node-unzipper/blob/bad-zip/lib/PullStream.js#L59
The above "fix" actually breaks normal ZIP files.

Reproduction code with code from master branch:

const fs = require('fs')
const unzipper = require('unzipper')

fs.createReadStream('./bad.zip')
    .pipe(unzipper.Extract({ path: './bad' })
    .on('error', err => console.error('error', err))
    .on('finish', () => console.log('finish'))
    .on('close', () => console.log('close')))

Output:

error FILE_ENDED
close
finish
_stream_writable.js:464
  cb();
  ^

TypeError: cb is not a function
    at afterWrite (_stream_writable.js:464:3)
    at onwrite (_stream_writable.js:455:7)
    at ./node-unzipper/lib/PullStream.js:59:60
    at afterWrite (_stream_writable.js:464:3)
    at _combinedTickCallback (internal/process/next_tick.js:144:20)
    at process._tickCallback (internal/process/next_tick.js:180:9)

[rhodgkins]

After looking into it again, there's a more reliable fix here:

bookcreator@c512d34

This removes the write callback to prevent it from being called again.

[ZJONSSON]

Thanks @rhodgkins, can you recreate this in a test (that fails before and succeeds after)?
PR would be much appreciated - if not I can pull this in directly

[rhodgkins]

No problem @ZJONSSON (only issue would be with a failing test case - the ZIP attached to this issue is over 2MB).

I'm not sure if this fixes the issue though? The finish/close events are still being emitted after the error event - like I said above, this seems wrong? Maybe its OK for the close event to be emitted but not so sure about finish!

[ZJONSSON]

Thanks again @rhodgkins, sorry for delay in response. OK I think I figured out what is going on here, at least with the original issue.

One of the files in the zip file (META-INF/container.xml) has an unknown filesize, in the file header itself. According to spec, when filesize is unknown the file has to be terminated with a special value 0x08074b50, however in this particular file the termination value seems to be missing.

If we were to follow the central directory information (and ignore the file header) we would be able to extract successfully.

Here is what I propose:

• If streaming an file with unknown size fails to find the special value 0x08074b50 we show a more specific error (with reference to the underlying file in question) stating that this zip has to be Open (i.e. opened by reading the central directory first)
• Add extract method to .Open to provided a safer way to extract through directory
• We need to patch unzip so that if uncompressedSize is not defined but is available through directory, we opt for the directory information.

Separately we need to ensure finish and close events are not emitted upon error as it can lead to confusion.

For reference here is the file header (note the uncompressedSize):

{ 
  signature: 67324752,
  versionsNeededToExtract: 20,
  flags: 2078,
  compressionMethod: 8,
  lastModifiedTime: 20739,
  lastModifiedDate: 19579,
  crc32: 0,
  compressedSize: 0,
  uncompressedSize: 0,
  fileNameLength: 22,
  extraFieldLength: 0,
  fileName: 'META-INF/container.xml',
  extra: {} }

and here is the same header in the directory:

{ signature: 33639248,
  versionMadeBy: 813,
  versionsNeededToExtract: 20,
  flags: 2078,
  compressionMethod: 8,
  lastModifiedTime: 20739,
  lastModifiedDate: 19579,
  crc32: 1085018556,
  compressedSize: 154,
  uncompressedSize: 222,
  fileNameLength: 22,
  extraFieldLength: 0,
  fileCommentLength: 0,
  diskNumber: 0,
  internalFileAttributes: 0,
  externalFileAttributes: 2175008800,
  offsetToLocalFileHeader: 58,
  path: 'META-INF/container.xml',
  extra: {},
  comment: ''
}

[ZJONSSON]

@rhodgkins #71 Added an extract method to Open which successfully extracts your file
Also ensure that finish is not emitted upon error.
Can you take a look?

[rhodgkins]

@ZJONSSON sorry for taking so long to get back to you. Yeh this looks all good from my side 👍

[rhodgkins]

@ZJONSSON I'm just had a more extensive test and it seems to produce { Error: unexpected end of file at InflateRaw.zlibOnError (zlib.js:153:15) errno: -5, code: 'Z_BUF_ERROR' } errors where previously it was all ok :-/

Just to note I'm testing with https://github.com/bookcreator/node-unzipper/tree/bad-zip-working which has the latest changes from this repos master branch merged into it.
I've also tried without the merged changes - just using #71 to the same affect.

I haven't tried to use the new methods you created (as I'm streaming the ZIP), just using the existing code as in the original post.

Attached ZIP where it worked before the change:
NoLongerWorking.zip

[ZJONSSON]

Awesome thanks @rhodgkins I'll look at this later today - in the meantime if you have a fix please post

[ChrisAlvares]

Any updates here? I am also getting this error with corrupted zips.

[ZJONSSON]

Latest version is: + unzipper@0.9.9
I am able to successfully extract NotLongerWorking.zip Can you please verify and let me know? If this issue persist please reopen the ticket