Fix GH-17787: ZipArchive stream truncates when the archive is freed

[eyupcanakman]

ZipArchive::getStreamIndex() (and getStream/getStreamName) hands back a stream that reads from the archive's underlying zip_t without keeping a reference to it. When the ZipArchive object is freed while the stream is still open, as in the report where $z gets reassigned inside the read loop, the object destructor closes the zip_t and the stream stops reading early. cmb69 diagnosed this on the issue. The fix keeps the archive object alive for the stream's lifetime, so the borrowed handle stays valid until the stream is closed.

One case this does not cover is calling $zip->close() or reopening the same object while a stream is open, which hits the same truncation by a different route. I can extend the fix to those paths if you prefer.

[pierrejoye]

is this PR still needed?

current master running the test from this PR:

--TEST--
GH-17787 (ZipArchive stream stops reading early when the archive is freed while the stream is open)
--EXTENSIONS--
zip
--FILE--
PHP Warning:  stream_get_contents(): Zip stream error: Containing zip archive was closed in /home/pierre/project/libgd/merge-php/php-src/test_gh_22555.php on line 22
PHP Stack trace:
PHP   1. {main}() /home/pierre/project/libgd/merge-php/php-src/test_gh_22555.php:0
PHP   2. stream_get_contents($stream = resource(4) of type (stream)) /home/pierre/project/libgd/merge-php/php-src/test_gh_22555.php:22
/home/pierre/project/libgd/merge-php/php-src/test_gh_22555.php:22:
bool(false)
--CLEAN--
--EXPECT--
bool(true)

which is the correct error.

[eyupcanakman]

That output is the bug the PR fixes. gh17787.phpt fails on current master the same way for me and passes with the patch applied, and cmb69 opened the issue back up because the zip_t shouldn't be closed while a stream is still open.

[LamentXU123]

Could we have a test for getStreamName?

[devnexen]

I would say it is probably possible to avoid this change in a stable branch.

[eyupcanakman]

php_stream_zip_open has a single caller in php_zip.c, the symbol isn't exported (no PHPAPI, hidden visibility) and php_zip.h isn't installed, so the prototype change isn't visible outside the tree. Can keep the old signature and add a second function that takes the object if you'd rather avoid it on 8.4.

[devnexen]

No you re right.
Would it be possible however to update the test as follow ?

diff --git a/ext/zip/tests/gh17787.phpt b/ext/zip/tests/gh17787.phpt
index c05558191cf..f82cfa1145b 100644
--- a/ext/zip/tests/gh17787.phpt
+++ b/ext/zip/tests/gh17787.phpt
@@ -30,11 +30,51 @@

 var_dump(stream_get_contents($stream) === $data);
 fclose($stream);
+
+// Same with getStream()
+$zip = new ZipArchive;
+$zip->open($name, ZipArchive::RDONLY);
+$stream = $zip->getStream('entry.txt');
+$zip = null;
+
+var_dump(stream_get_contents($stream) === $data);
+fclose($stream);
+
+// Pending changes are still committed once the last stream is closed
+$name = __DIR__ . '/gh17787_write.zip';
+
+$zip = new ZipArchive;
+var_dump($zip->open($name, ZipArchive::CREATE | ZipArchive::OVERWRITE));
+$zip->addFromString('first.txt', 'first');
+$zip->close();
+
+$zip = new ZipArchive;
+var_dump($zip->open($name));
+$zip->addFromString('second.txt', 'second');
+$stream = $zip->getStreamName('first.txt', ZipArchive::FL_UNCHANGED);
+$zip = null;
+
+var_dump(stream_get_contents($stream));
+fclose($stream);
+
+$zip = new ZipArchive;
+var_dump($zip->open($name, ZipArchive::RDONLY));
+var_dump($zip->numFiles);
+var_dump($zip->getFromName('second.txt'));
+$zip->close();
 ?>
 --CLEAN--
 <?php
 @unlink(__DIR__ . '/gh17787.zip');
+@unlink(__DIR__ . '/gh17787_write.zip');
 ?>
 --EXPECT--
 bool(true)
 bool(true)
+bool(true)
+bool(true)
+bool(true)
+string(5) "first"
+bool(true)
+int(2)
+string(6) "second"

[eyupcanakman]

Added a getStreamName case to gh17787.phpt.