Skip to content

Conversation

sagara-
Copy link
Contributor

@sagara- sagara- commented Nov 3, 2017

This fixes the CRC calculation for data added from streams. It should be calculating the CRC over the uncompressed data, not the compressed data. unzip -t file.zip on linux can be used to verify the integrity of zipped data.

The following short program can can be used to demonstrate the issue. Without this patch, the CRC verification fails because it calculates the CRC over the compressed data instead of the original data.

<?php

require_once 'vendor/autoload.php';

use ZipStream\ZipStream;

$zipPath = '/tmp/file.zip';

$inputFile = fopen('php://memory', 'r+');
fwrite($inputFile, 'testing');
rewind($inputFile);

$fd = fopen($zipPath, 'w');
try {
    $zip = new ZipStream('output.zip', [
        ZipStream::OPTION_OUTPUT_STREAM => $fd,
    ]);

    $zip->addFileFromStream('test.txt', $inputFile);
    $zip->finish();
}
finally {
    fclose($fd);
}

system("unzip -t $zipPath");
unlink($zipPath);

@maennchen
Copy link
Owner

@sagara- Sorry, I totally missed your PR.

@maennchen maennchen merged commit 670dae1 into maennchen:master Nov 29, 2017
@sagara- sagara- deleted the fix-stream-crc branch December 3, 2017 00:36
@sagara-
Copy link
Contributor Author

sagara- commented Dec 3, 2017

Don't worry about it, thanks for getting around to it.

@rodush
Copy link

rodush commented Dec 4, 2017

@maennchen when are you planning to create a new tag with this fix?

Strange enough, our module which uses this library worked pretty well, until some moment ago, so apparently there was some regression introduced in the development.

@Petah
Copy link

Petah commented Dec 7, 2017

I tried running this fix with s3 streams and I still get the same bad CRC error.

@sagara-
Copy link
Contributor Author

sagara- commented Dec 8, 2017

@Petah Can you try using the patch from #67? I had forgotten to submit this patch as well. I'm guessing you're compressing data greater than 1mb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants