Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
eyupcanakman wants to merge 3 commits into from
+99 −3
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be41aef
ext/zip: Fix GH-17787: ZipArchive stream truncates when the archive i…
eyupcanakman Jul 2, 2026
bafc2ac
Add test for getStreamName
eyupcanakman Aug 12, 2026
239a9b7
Add tests for getStream and for pending changes on destroy
eyupcanakman Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ PHP NEWS
. Fixed bug GH-22395 (base_convert() outputs at most 64 characters).
(Weilin Du)

- Zip:
. Fixed bug GH-17787 (ZipArchive stream stops reading early when the archive
is freed while the stream is still open). (Eyüp Can Akman)

02 Jul 2026, PHP 8.4.23

- Core:
Expand Down
2 changes: 1 addition & 1 deletion ext/zip/php_zip.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3024,7 +3024,7 @@ static void php_zip_get_stream(INTERNAL_FUNCTION_PARAMETERS, int type, bool acce
PHP_ZIP_STAT_INDEX(intern, index, flags, sb);
}

stream = php_stream_zip_open(intern, &sb, mode, flags STREAMS_CC);
stream = php_stream_zip_open(Z_ZIP_P(self), &sb, mode, flags STREAMS_CC);
if (stream) {
php_stream_to_zval(stream, return_value);
} else {
Expand Down
2 changes: 1 addition & 1 deletion ext/zip/php_zip.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ static inline ze_zip_object *php_zip_fetch_object(zend_object *obj) {
#define Z_ZIP_P(zv) php_zip_fetch_object(Z_OBJ_P((zv)))

php_stream *php_stream_zip_opener(php_stream_wrapper *wrapper, const char *path, const char *mode, int options, zend_string **opened_path, php_stream_context *context STREAMS_DC);
php_stream *php_stream_zip_open(struct zip *arch, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC);
php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC);

@devnexen devnexen Aug 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@eyupcanakman eyupcanakman Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 devnexen Aug 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 eyupcanakman Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


extern const php_stream_wrapper php_stream_zip_wrapper;

Expand Down
80 changes: 80 additions & 0 deletions ext/zip/tests/gh17787.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
--TEST--
GH-17787 (ZipArchive stream stops reading early when the archive is freed while the stream is open)
--EXTENSIONS--
zip
--FILE--
<?php
$name = __DIR__ . '/gh17787.zip';
$data = str_repeat("The quick brown fox jumps over the lazy dog.\n", 4000);

$zip = new ZipArchive;
$zip->open($name, ZipArchive::CREATE | ZipArchive::OVERWRITE);
$zip->addFromString('entry.txt', $data);
$zip->close();

$zip = new ZipArchive;
$zip->open($name, ZipArchive::RDONLY);
$stream = $zip->getStreamIndex(0, ZipArchive::FL_UNCHANGED);

// Free the archive while the stream is still open
$zip = null;

var_dump(stream_get_contents($stream) === $data);
fclose($stream);

// Same with getStreamName()
$zip = new ZipArchive;
$zip->open($name, ZipArchive::RDONLY);
$stream = $zip->getStreamName('entry.txt', ZipArchive::FL_UNCHANGED);
$zip = null;

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"
14 changes: 13 additions & 1 deletion ext/zip/zip_stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ struct php_zip_stream_data_t {
struct zip_file *zf;
size_t cursor;
php_stream *stream;
ze_zip_object *owner;
};

#define STREAM_DATA_FROM_STREAM() \
Expand Down Expand Up @@ -101,6 +102,12 @@ static int php_zip_ops_close(php_stream *stream, int close_handle)
self->za = NULL;
}
}

/* the pinned object ref is tied to self, so release it regardless of close_handle */
if (self->owner) {
OBJ_RELEASE(&self->owner->zo);
self->owner = NULL;
}
efree(self);
stream->abstract = NULL;
return EOF;
Expand Down Expand Up @@ -234,8 +241,9 @@ const php_stream_ops php_stream_zipio_ops = {
};

/* {{{ php_stream_zip_open */
php_stream *php_stream_zip_open(struct zip *arch, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC)
php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC)
{
struct zip *arch = obj->za;
struct zip_file *zf = NULL;

php_stream *stream = NULL;
Expand All @@ -254,6 +262,9 @@ php_stream *php_stream_zip_open(struct zip *arch, struct zip_stat *sb, const cha
self->zf = zf;
self->stream = NULL;
self->cursor = 0;
/* keep the archive object alive while the stream borrows its zip_t */
self->owner = obj;
GC_ADDREF(&obj->zo);
#if LIBZIP_ATLEAST(1,9,1)
if (zip_file_is_seekable(zf) > 0) {
stream = php_stream_alloc(&php_stream_zipio_seek_ops, self, NULL, mode);
Expand Down Expand Up @@ -339,6 +350,7 @@ php_stream *php_stream_zip_opener(php_stream_wrapper *wrapper,
self->zf = zf;
self->stream = NULL;
self->cursor = 0;
self->owner = NULL;
#if LIBZIP_ATLEAST(1,9,1)
if (zip_file_is_seekable(zf) > 0) {
stream = php_stream_alloc(&php_stream_zipio_seek_ops, self, NULL, mode);
Expand Down
Loading