7z reports CRC error on files downloaded from NextCloud
andyboeh opened this issue · 14 comments
There is already a NC bug open: nextcloud/server/issues/2352
Some files downloaded from NextCloud won't work in Windows ZIP or, in my case. 7z for Linux or any frontend (e.g. Engrampa).
Disabling Zip64 seems to fix the issue.
I get the following output using 7z t file.zip:
7z t file.zip
7-Zip [64] 16.02 : Copyright (c) 1999-2016 Igor Pavlov : 2016-05-21
p7zip Version 16.02 (locale=en_GB.utf8,Utf16=on,HugeFiles=on,64 bits,4 CPUs Intel(R) Core(TM) i5-4200U CPU @ 1.60GHz (40651),ASM,AES-NI)
Scanning the drive for archives:
1 file, 8363809 bytes (8168 KiB)
Testing archive: file.zip
ERRORS:
Headers Error
--
Path = file.zip
Type = zip
ERRORS:
Headers Error
Physical Size = 8363809
64-bit = +
Archives with Errors: 1
Open Errors: 1
I will first try to find the source of the problem in the nextcloud issue (unless someone provided reproduction of the problem with ZipStreamer directly).
With ZipStreamer, I can reproduce this 7zip issue with the following code:
<?php
require("src/ZipStreamer.php");
$zip = new ZipStreamer\ZipStreamer();
$stream = fopen("README.md", "r");
$zip->addFileFromStream($stream, 'README.test');
fclose($stream);
$zip->finalize();
?>
Running php test.php >test.zip and 7z t test.zip
gives me the same Headers Error.
I'm using p7zip 16.02 on Linux. Some debugging showed me this error is caused when reading the end of central directory record.
From CPP/7zip/Archive/Zip/ZipIn.cpp:2077:
if (ecd.NumEntries > items.Size())
HeadersError = true;
ecd.NumEntries is 0xffff.
If I modify buildEndOfCentralDirectoryRecord() to put the correct number of entries in the record:
$cdRecCount = min(sizeof($this->cdRec), 0xffff);
p7zip can test the archive without errors.
As far as I can see, the behaviour of ZipStreamer is compliant with the spec. See https://pkware.cachefly.net/webdocs/APPNOTE/APPNOTE-6.3.4.TXT, section 4.4.21 and 4.4.22:
The total number of files [...]. If an archive is in ZIP64 format and the value in this field is 0xFFFF, the size will be in the corresponding 8 byte zip64 end of central directory field.
As already noted in #2352, the 7zip author acknowledged this to be a bug in 7zip: https://sourceforge.net/p/sevenzip/discussion/45797/thread/cc8912f1/
But 7zip does not seem to be the only software with problems. And just because you can max out the ZIP64 specification does not mean you have to. I don't see a good reason to insist on using the extended fields when the traditional fields are sufficient.
If you can work around the issues in ZipStreamer while not losing anything (except perhaps for your pride) and still producing valid zip files, you should do it.
There's really no need to get personal. This is not about my pride. The Zip64 support in ZipStreamer was not even written by me.
Let me assure, if I knew a way to support files bigger than 4GB without the Zip64 extension, I would do so. If I knew a way to stream zip files on the fly without using the data descriptor and not setting the "traditional" fields, I would do so.
You may be actually wrong about being compliant to the spec. As far as I understand section 4.4.1.4, it says that you should put 0xffff only if the value is too big for the regular record.
But even if you're right, and this behaviour actually is correct, the spec doesn't require you to put f's there. You still can put normal value in both places, no-one will be hurt, and more unpackers will be happy. I just tested it by changing the line 528 to put the meaningful value there. p7zip was quite happy with the result.
Looks like it is not a bug in 7z.
https://sourceforge.net/p/sevenzip/discussion/45797/thread/cc8912f1/
"That archive contains unused space in zip64 extra fields.
You can report about that problem to creators of that archive (Owncloud or someone else)."
Changing Line 528 to "$cdRecCount = min(sizeof($this->cdRec), 0xffff);" gets valid zips for 7z
Just bumped into this too. I was quite surprised a zip archive created by Owncloud doesn't open on Ubuntu.
Do you see any issues with the following patch?
Changing Line 528 to
$cdRecCount = min(sizeof($this->cdRec), 0xffff);gets valid zips for 7z
From my limited understanding, it seems as if this patch provides compatibility with 7z while still maintaining spec compliance. Is this correct or am I missing something?
Changing Line 528 to $cdRecCount = min(sizeof($this->cdRec), 0xffff); gets valid zips for 7z
Tested on Ubuntu with different tools. Works fine!
What is the current state on this issue?
As per the 7z programmer's comment:
"That archive contains unused space in zip64 extra fields.
You can report about that problem to creators of that archive (Owncloud or someone else)."
It seems to me that perhaps the proposed patch is working around the problem, by not requiring the decompresser to go into the zip64 fields. That is, there may be an actual problem with the zip64 fields generated by PHPZipStreamer, at least according to the 7z programmer. However, I do not know the ZIP format myself, so I may be wrong.
@McNetic Would it be fair to assume that, at least for files < 4GB, the traditional size field can be used? Even if this is not the problem, it might be good to do it as a workaround, if it helps users deal with the vast majority of cases... (I'm assuming the vast majority of use cases don't involve huge multi-GB ZIP files).
PR is open ....
Hello everyone, the project was abandonned? @DeepDiver1975 Your PR is in error ..
Hello everyone, the project was abandonned? @DeepDiver1975 Your PR is in error ..
see #39 (comment)