archiverjs / node-zip-stream

a streaming zip archive generator
http://archiverjs.com/zip-stream
MIT License
154 stars 21 forks source link

Zero length files written incorrectly #7

Closed danyocom closed 10 years ago

danyocom commented 10 years ago

The file headers for zero length files should have a compression method of 0 (zero) (same as directories). Otherwise various zip utilities choke/complain.

See section 4.3.8 in https://www.pkware.com/documents/casestudies/APPNOTE.TXT

ctalkington commented 10 years ago

this should be the case already, unless im missing something.

the test for this situation passes 7zip verify tests,

ctalkington commented 10 years ago

nvm, i see what you mean. there really isn't a good way to detect this with streams as the data size isnt known til after its been piped. i know the size itself is set after such via a builtin feature of zip that allows putting size and CRC in file descriptor record. However, im pretty sure this isn't possible with compression method.

danyocom commented 10 years ago

Right, not sure how you would do this with a stream. But with buffers and strings you can determine the length and set the compression method accordingly.

ctalkington commented 10 years ago

im not sure that section of APPNOTE explains what you think.

Zero-byte files, directories, and other file types that contain no content MUST not include file data.

thats saying you shouldnt have actual content for the file, im fairly sure that streaming a zero length doesn't create any content. the size defaults to 0 and even zlib deflateraw doesnt append headers so should be no real output actually piped.

what program is the issue?

ctalkington commented 10 years ago

also to note, i go by APPNOTE 2.0 in most cases for compat reasons haven't explored higher until we get into zip64 and such.

danyocom commented 10 years ago

I have problems on android and MacOS with zero length files compressed using archiver unless I set the compression method to Zero.

if (source.length === 0) { data.compressionMethod = 0; }

in 'ZipStream.prototype._appendBuffer' before writing the file header.

danyocom commented 10 years ago

You mean appnote 2.0 from 1993? http://www.digitalpreservation.gov/formats/digformatspecs/APPNOTE(19930201)_Version_2.0.txt

ctalkington commented 10 years ago

yah, has the base features of zip and wide compat at this point.

https://github.com/ctalkington/node-zip-stream/blob/master/APPNOTE.txt

ctalkington commented 10 years ago

yah, we can def do that for buffer as its more static. the only way with a stream would be to read ahead but that may not work in every case.

ctalkington commented 10 years ago

0.3.1 should address the Buffer side of this.

danyocom commented 10 years ago

Thanks for your prompt response! I was expecting the support issue to sit for days-weeks.

ctalkington commented 10 years ago

there are some issues that end up like that due to lack of response or debug-ability but most the time, i shoot for bug fixes within the first 72 hours of report.

ctalkington commented 10 years ago

i'm going to think on the Streams side of this for a bit, not liking current solutions coming to mind.

danyocom commented 10 years ago

So while doing some testing I discovered that zero length files worked in node-archiver 0.4.10 (which zip-stream seems to be based on) but are broken in 0.5.2+.

So the fix I came up with yesterday could be a 'workaround' and the proper implementation could have been in archiver 0.4.10 or vise versa.

The difference seems to be that in 4.10 if the source file is zero bytes the the file is compressed and written anyway. In 0.5.2 if the file is zero bytes the the raw bytes (0 in this case are written) and the compression is not done.

So I guess the header and continents just need to match. If compressionMethod is 0 then then zero bytes should be written. If the compression method is 8 then the compressed version of zero bytes should be written.

Again, I am only looking at the buffer implementation not streams. (it could be that streams were never broken ... I did not check)

ctalkington commented 10 years ago

yah code has diverged a bit from archiver 0.4.10. however, the processStream that its writing to, should have the same effect as its just zlib.deflateRaw extended which has no zlib header so not sure what it was really writing before honestly. i think storing empty files is an acceptable solution but i honestly never had any issues with way zero files. tested with 7zip, total commander, and unzip on windows 7. do you have the exact error/warning you were getting?

EDIT: you can see what changed between archiver 0.4.10 and 0.5.2 here: https://github.com/ctalkington/node-archiver/compare/0.4.10...0.5.2

danyocom commented 10 years ago

I have a zero length file in my archive called _ZEROFILE created using zip-stream. This is the output of the 7Zip test (before the compression method = 0 fix).

screen shot 2014-04-16 at 11 34 22 am

ctalkington commented 10 years ago

what code do you use to create the _ZEROFILE?

ctalkington commented 10 years ago

closing for non-response. please open a new issue if you still have issues with newest version.