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 both cases. The pending-changes one is a better test than I expected, since unpatched it does not just fail the read, it drops the queued write (numFiles 1, getFromName false).

One thing I hit while stress testing the patch. Pinning the object makes a subclass that stores its own stream uncollectable, because php_zip_get_gc cannot see the GC_ADDREF at zip_stream.c:266.

class Z extends ZipArchive { public $stream; }
$z = new Z; $z->open($f, ZipArchive::RDONLY);
$z->stream = $z->getStreamIndex(1, ZipArchive::FL_UNCHANGED);
$w = WeakReference::create($z); unset($z);
var_dump(gc_collect_cycles(), $w->get() instanceof Z);  // int(0), bool(true)

Without the fix it prints bool(false). Over 500 iterations that is 700KB and one leaked fd each time. It needs the stream reachable from the object and never closed, and fclose recovers everything.

I tried refcounting the zip_t instead so the stream never points back. That fixes the leak and breaks the fix, because addFromString hands libzip an object owned buffer via zip_source_buffer(..., 0) which free_storage frees, so the queued entry writes back garbage.

The same prototype with addFile is fine, so it is the buffer ownership. Looks like the pin is needed here and a real fix means moving that ownership. Want me to open a separate issue for it?

[eyupcanakman]

Added a getStreamName case to gh17787.phpt.

[LamentXU123]

The circleCI failure is unrelated. @devnexen Do you think this is good to merge?