From 2b73eff8bb2a3d33a04a43a54342100a0a46839a Mon Sep 17 00:00:00 2001 From: Weilin Du Date: Sat, 15 Aug 2026 00:02:37 +0800 Subject: [PATCH 1/9] Fix GH-23276: Collect ZipArchive subclasses holding their own streams --- NEWS | 2 + ext/zip/php_zip.c | 101 ++++++++++++++++++++++++------------- ext/zip/php_zip.h | 11 +++- ext/zip/tests/gh23276.phpt | 47 +++++++++++++++++ ext/zip/zip_stream.c | 18 +++---- 5 files changed, 131 insertions(+), 48 deletions(-) create mode 100644 ext/zip/tests/gh23276.phpt diff --git a/NEWS b/NEWS index 1f1aab1cd076..b05f3850d921 100644 --- a/NEWS +++ b/NEWS @@ -10,6 +10,8 @@ PHP NEWS . Fixed opcache.protect_memory race under ZTS. (realFlowControl) - Zip: + . Fixed bug GH-23276 (ZipArchive subclass storing its own stream cannot be + garbage collected). (Weilin Du) . Fixed ZipArchive::extractTo() and ZipArchive::getFrom*() reporting success on corrupted entries. (David Carlier) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 3d6abde1c312..c48cbaa5b43f 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -1101,26 +1101,43 @@ static void _php_zip_cancel_callback_free(void *ptr) } #endif +php_zip_archive *php_zip_archive_create(struct zip *za) +{ + php_zip_archive *archive = emalloc(sizeof(*archive)); + + archive->za = za; + archive->refcount = 1; + return archive; +} + +void php_zip_archive_addref(php_zip_archive *archive) +{ + archive->refcount++; +} + +void php_zip_archive_release(php_zip_archive *archive) +{ + ZEND_ASSERT(archive->refcount > 0); + + if (--archive->refcount == 0) { + if (archive->za && zip_close(archive->za) != 0) { + php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(archive->za)); + zip_discard(archive->za); + } + efree(archive); + } +} + static void php_zip_object_free_storage(zend_object *object) /* {{{ */ { ze_zip_object * intern = php_zip_fetch_object(object); - int i; if (!intern) { return; } - if (intern->za) { - if (zip_close(intern->za) != 0) { - php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(intern->za)); - zip_discard(intern->za); - } - } - - if (intern->buffers_cnt>0) { - for (i=0; ibuffers_cnt; i++) { - efree(intern->buffers[i]); - } - efree(intern->buffers); + if (intern->archive) { + php_zip_archive_release(intern->archive); + intern->archive = NULL; } #ifdef HAVE_PROGRESS_CALLBACK @@ -1539,12 +1556,17 @@ PHP_METHOD(ZipArchive, open) } if (ze_obj->za) { - /* we already have an opened zip, free it */ - if (zip_close(ze_obj->za) != 0) { - php_error_docref(NULL, E_WARNING, "Empty string as source"); - efree(resolved_path); - RETURN_FALSE; + if (ze_obj->archive->refcount == 1) { + /* we already have an opened zip, free it */ + if (zip_close(ze_obj->za) != 0) { + php_error_docref(NULL, E_WARNING, "Empty string as source"); + efree(resolved_path); + RETURN_FALSE; + } + ze_obj->archive->za = NULL; } + php_zip_archive_release(ze_obj->archive); + ze_obj->archive = NULL; ze_obj->za = NULL; } if (ze_obj->filename) { @@ -1578,6 +1600,7 @@ PHP_METHOD(ZipArchive, open) ze_obj->filename = resolved_path; ze_obj->filename_len = strlen(resolved_path); ze_obj->za = intern; + ze_obj->archive = php_zip_archive_create(intern); RETURN_TRUE; } /* }}} */ @@ -1625,13 +1648,14 @@ PHP_METHOD(ZipArchive, close) ze_obj = Z_ZIP_P(self); - err = zip_close(intern); - if (err) { - php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern)); - /* Save error for property reader */ - #if LIBZIP_VERSION_MAJOR < 1 + if (ze_obj->archive->refcount == 1) { + err = zip_close(intern); + if (err) { + php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern)); + /* Save error for property reader */ + #if LIBZIP_VERSION_MAJOR < 1 zip_error_get(intern, &ze_obj->err_zip, &ze_obj->err_sys); - #else + #else { zip_error_t *ziperr; @@ -1640,9 +1664,15 @@ PHP_METHOD(ZipArchive, close) ze_obj->err_sys = zip_error_code_system(ziperr); zip_error_fini(ziperr); } - #endif - zip_discard(intern); + #endif + zip_discard(intern); + } else { + ze_obj->err_zip = 0; + ze_obj->err_sys = 0; + } + ze_obj->archive->za = NULL; } else { + err = 0; ze_obj->err_zip = 0; ze_obj->err_sys = 0; } @@ -1653,6 +1683,8 @@ PHP_METHOD(ZipArchive, close) efree(ze_obj->filename); ze_obj->filename = NULL; ze_obj->filename_len = 0; + php_zip_archive_release(ze_obj->archive); + ze_obj->archive = NULL; ze_obj->za = NULL; if (!err) { @@ -2003,7 +2035,7 @@ PHP_METHOD(ZipArchive, addFromString) size_t name_len; ze_zip_object *ze_obj; struct zip_source *zs; - int pos = 0; + char *data; zend_long flags = ZIP_FL_OVERWRITE; if (zend_parse_parameters(ZEND_NUM_ARGS(), "sS|l", @@ -2014,20 +2046,17 @@ PHP_METHOD(ZipArchive, addFromString) ZIP_FROM_OBJECT(intern, self); ze_obj = Z_ZIP_P(self); - if (ze_obj->buffers_cnt) { - ze_obj->buffers = (char **)safe_erealloc(ze_obj->buffers, sizeof(char *), (ze_obj->buffers_cnt+1), 0); - pos = ze_obj->buffers_cnt++; + if (ZSTR_LEN(buffer)) { + data = pemalloc(ZSTR_LEN(buffer), 1); + memcpy(data, ZSTR_VAL(buffer), ZSTR_LEN(buffer)); } else { - ze_obj->buffers = (char **)emalloc(sizeof(char *)); - ze_obj->buffers_cnt++; - pos = 0; + data = NULL; } - ze_obj->buffers[pos] = (char *)safe_emalloc(ZSTR_LEN(buffer), 1, 1); - memcpy(ze_obj->buffers[pos], ZSTR_VAL(buffer), ZSTR_LEN(buffer) + 1); - zs = zip_source_buffer(intern, ze_obj->buffers[pos], ZSTR_LEN(buffer), 0); + zs = zip_source_buffer(intern, data, ZSTR_LEN(buffer), 1); if (zs == NULL) { + pefree(data, 1); RETURN_FALSE; } diff --git a/ext/zip/php_zip.h b/ext/zip/php_zip.h index 84fdd21e3476..8c83feb5c4d4 100644 --- a/ext/zip/php_zip.h +++ b/ext/zip/php_zip.h @@ -57,6 +57,11 @@ typedef struct _ze_zip_rsrc { typedef zip_rsrc * zip_rsrc_ptr; +typedef struct _php_zip_archive { + struct zip *za; + uint32_t refcount; +} php_zip_archive; + typedef struct _ze_zip_read_rsrc { struct zip_file *zf; struct zip_stat sb; @@ -68,11 +73,10 @@ typedef struct _ze_zip_read_rsrc { /* Extends zend object */ typedef struct _ze_zip_object { struct zip *za; - char **buffers; + php_zip_archive *archive; HashTable *prop_handler; char *filename; int filename_len; - int buffers_cnt; zip_int64_t last_id; int err_zip; int err_sys; @@ -93,6 +97,9 @@ static inline ze_zip_object *php_zip_fetch_object(zend_object *obj) { 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(ze_zip_object *obj, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC); +php_zip_archive *php_zip_archive_create(struct zip *za); +void php_zip_archive_addref(php_zip_archive *archive); +void php_zip_archive_release(php_zip_archive *archive); extern const php_stream_wrapper php_stream_zip_wrapper; diff --git a/ext/zip/tests/gh23276.phpt b/ext/zip/tests/gh23276.phpt new file mode 100644 index 000000000000..5f1a49791ca6 --- /dev/null +++ b/ext/zip/tests/gh23276.phpt @@ -0,0 +1,47 @@ +--TEST-- +GH-23276 (ZipArchive subclass storing its own stream is collectable) +--EXTENSIONS-- +zip +--FILE-- +open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE); +$zip->addFromString('entry.txt', 'contents'); +$zip->close(); +$zip->open($filename, ZipArchive::RDONLY); +$zip->stream = $zip->getStream('entry.txt'); +$weakRef = WeakReference::create($zip); +unset($zip); + +gc_collect_cycles(); +var_dump($weakRef->get()); + +// An externally held stream must still keep its archive alive. +$zip = new ZipArchive; +$zip->open($filename, ZipArchive::RDONLY); +$stream = $zip->getStream('entry.txt'); +$weakRef = WeakReference::create($zip); +unset($zip); + +gc_collect_cycles(); +var_dump($weakRef->get() instanceof ZipArchive); +var_dump(stream_get_contents($stream)); +fclose($stream); +gc_collect_cycles(); +var_dump($weakRef->get()); +?> +--CLEAN-- + +--EXPECT-- +NULL +bool(true) +string(8) "contents" +NULL diff --git a/ext/zip/zip_stream.c b/ext/zip/zip_stream.c index 7f6990962d00..42f4efbcc444 100644 --- a/ext/zip/zip_stream.c +++ b/ext/zip/zip_stream.c @@ -34,7 +34,7 @@ struct php_zip_stream_data_t { struct zip_file *zf; size_t cursor; php_stream *stream; - ze_zip_object *owner; + php_zip_archive *archive; }; #define STREAM_DATA_FROM_STREAM() \ @@ -103,10 +103,9 @@ static int php_zip_ops_close(php_stream *stream, int close_handle) } } - /* 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; + if (self->archive) { + php_zip_archive_release(self->archive); + self->archive = NULL; } efree(self); stream->abstract = NULL; @@ -258,13 +257,12 @@ php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const c if (zf) { self = emalloc(sizeof(*self)); - self->za = NULL; /* to keep it open on stream close */ + self->za = NULL; 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); + self->archive = obj->archive; + php_zip_archive_addref(self->archive); #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); @@ -350,7 +348,7 @@ php_stream *php_stream_zip_opener(php_stream_wrapper *wrapper, self->zf = zf; self->stream = NULL; self->cursor = 0; - self->owner = NULL; + self->archive = 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); From 7c12367d976ea64b927a11399f709f86313d1cbb Mon Sep 17 00:00:00 2001 From: Weilin Du Date: Sat, 15 Aug 2026 00:22:51 +0800 Subject: [PATCH 2/9] fix CI --- ext/zip/php_zip.c | 54 ++++++++++++++++---------------------- ext/zip/tests/gh23276.phpt | 4 +-- 2 files changed, 25 insertions(+), 33 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index c48cbaa5b43f..90ddb7c475fd 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -1556,15 +1556,13 @@ PHP_METHOD(ZipArchive, open) } if (ze_obj->za) { - if (ze_obj->archive->refcount == 1) { - /* we already have an opened zip, free it */ - if (zip_close(ze_obj->za) != 0) { - php_error_docref(NULL, E_WARNING, "Empty string as source"); - efree(resolved_path); - RETURN_FALSE; - } - ze_obj->archive->za = NULL; + /* we already have an opened zip, free it */ + if (zip_close(ze_obj->za) != 0) { + php_error_docref(NULL, E_WARNING, "Empty string as source"); + efree(resolved_path); + RETURN_FALSE; } + ze_obj->archive->za = NULL; php_zip_archive_release(ze_obj->archive); ze_obj->archive = NULL; ze_obj->za = NULL; @@ -1648,34 +1646,28 @@ PHP_METHOD(ZipArchive, close) ze_obj = Z_ZIP_P(self); - if (ze_obj->archive->refcount == 1) { - err = zip_close(intern); - if (err) { - php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern)); - /* Save error for property reader */ - #if LIBZIP_VERSION_MAJOR < 1 - zip_error_get(intern, &ze_obj->err_zip, &ze_obj->err_sys); - #else - { - zip_error_t *ziperr; - - ziperr = zip_get_error(intern); - ze_obj->err_zip = zip_error_code_zip(ziperr); - ze_obj->err_sys = zip_error_code_system(ziperr); - zip_error_fini(ziperr); - } - #endif - zip_discard(intern); - } else { - ze_obj->err_zip = 0; - ze_obj->err_sys = 0; + err = zip_close(intern); + if (err) { + php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern)); + /* Save error for property reader */ + #if LIBZIP_VERSION_MAJOR < 1 + zip_error_get(intern, &ze_obj->err_zip, &ze_obj->err_sys); + #else + { + zip_error_t *ziperr; + + ziperr = zip_get_error(intern); + ze_obj->err_zip = zip_error_code_zip(ziperr); + ze_obj->err_sys = zip_error_code_system(ziperr); + zip_error_fini(ziperr); } - ze_obj->archive->za = NULL; + #endif + zip_discard(intern); } else { - err = 0; ze_obj->err_zip = 0; ze_obj->err_sys = 0; } + ze_obj->archive->za = NULL; /* clear cache as empty zip are not created but deleted */ php_clear_stat_cache(1, ze_obj->filename, ze_obj->filename_len); diff --git a/ext/zip/tests/gh23276.phpt b/ext/zip/tests/gh23276.phpt index 5f1a49791ca6..4d8f3c35b504 100644 --- a/ext/zip/tests/gh23276.phpt +++ b/ext/zip/tests/gh23276.phpt @@ -22,7 +22,7 @@ unset($zip); gc_collect_cycles(); var_dump($weakRef->get()); -// An externally held stream must still keep its archive alive. +// An externally held stream must remain usable after archive collection. $zip = new ZipArchive; $zip->open($filename, ZipArchive::RDONLY); $stream = $zip->getStream('entry.txt'); @@ -30,7 +30,7 @@ $weakRef = WeakReference::create($zip); unset($zip); gc_collect_cycles(); -var_dump($weakRef->get() instanceof ZipArchive); +var_dump($weakRef->get() === null); var_dump(stream_get_contents($stream)); fclose($stream); gc_collect_cycles(); From c90d8a722eee2f17d0419c7243023ef49b910c45 Mon Sep 17 00:00:00 2001 From: Weilin Du Date: Sat, 15 Aug 2026 02:58:27 +0800 Subject: [PATCH 3/9] Try to use get_gc and implement buffers. I still don't want to change Zend code --- ext/zip/php_zip.c | 142 +++++++++++++++++++++++-------------- ext/zip/php_zip.h | 12 +--- ext/zip/tests/gh23276.phpt | 10 +-- ext/zip/zip_stream.c | 31 +++++--- 4 files changed, 119 insertions(+), 76 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 90ddb7c475fd..981425241ff4 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -1044,9 +1044,62 @@ static int php_zip_has_property(zend_object *object, zend_string *name, int type static HashTable *php_zip_get_gc(zend_object *object, zval **gc_data, int *gc_data_count) /* {{{ */ { - *gc_data = NULL; - *gc_data_count = 0; - return zend_std_get_properties(object); + ze_zip_object *obj = php_zip_fetch_object(object); + HashTable *properties = zend_std_get_properties(object); + HashTable streams; + zend_get_gc_buffer *gc_buffer = NULL; + zval *value; + + zend_hash_init(&streams, 0, NULL, NULL, 0); + ZEND_HASH_FOREACH_VAL_IND(properties, value) { + php_stream *stream; + zend_resource *resource; + uint32_t property_refs = 0; + zval *property; + + if (Z_TYPE_P(value) != IS_RESOURCE) { + continue; + } + + resource = Z_RES_P(value); + if (zend_hash_index_exists(&streams, resource->handle)) { + continue; + } + zend_hash_index_add_empty_element(&streams, resource->handle); + + stream = zend_fetch_resource2(resource, NULL, php_file_le_stream(), php_file_le_pstream()); + if (stream == NULL || !php_zip_stream_has_owner(stream, obj)) { + continue; + } + + ZEND_HASH_FOREACH_VAL_IND(properties, property) { + if (Z_TYPE_P(property) == IS_RESOURCE && Z_RES_P(property) == resource) { + property_refs++; + } + } ZEND_HASH_FOREACH_END(); + + /* + * A Zip stream pins its owner natively. Expose that reference only + * when the resource is held exclusively by this object's properties; + * an externally-held stream must keep its owner alive. + */ + if (GC_REFCOUNT(resource) == property_refs) { + if (gc_buffer == NULL) { + gc_buffer = zend_get_gc_buffer_create(); + } + zend_get_gc_buffer_add_obj(gc_buffer, object); + } + } ZEND_HASH_FOREACH_END(); + zend_hash_destroy(&streams); + + if (gc_buffer != NULL) { + zend_get_gc_buffer_use(gc_buffer, gc_data, gc_data_count); + } else { + *gc_data = NULL; + *gc_data_count = 0; + } + + return properties; } /* }}} */ @@ -1101,43 +1154,26 @@ static void _php_zip_cancel_callback_free(void *ptr) } #endif -php_zip_archive *php_zip_archive_create(struct zip *za) -{ - php_zip_archive *archive = emalloc(sizeof(*archive)); - - archive->za = za; - archive->refcount = 1; - return archive; -} - -void php_zip_archive_addref(php_zip_archive *archive) -{ - archive->refcount++; -} - -void php_zip_archive_release(php_zip_archive *archive) -{ - ZEND_ASSERT(archive->refcount > 0); - - if (--archive->refcount == 0) { - if (archive->za && zip_close(archive->za) != 0) { - php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(archive->za)); - zip_discard(archive->za); - } - efree(archive); - } -} - static void php_zip_object_free_storage(zend_object *object) /* {{{ */ { ze_zip_object * intern = php_zip_fetch_object(object); + int i; if (!intern) { return; } - if (intern->archive) { - php_zip_archive_release(intern->archive); - intern->archive = NULL; + if (intern->za) { + if (zip_close(intern->za) != 0) { + php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(intern->za)); + zip_discard(intern->za); + } + } + + if (intern->buffers_cnt>0) { + for (i=0; ibuffers_cnt; i++) { + efree(intern->buffers[i]); + } + efree(intern->buffers); } #ifdef HAVE_PROGRESS_CALLBACK @@ -1562,9 +1598,6 @@ PHP_METHOD(ZipArchive, open) efree(resolved_path); RETURN_FALSE; } - ze_obj->archive->za = NULL; - php_zip_archive_release(ze_obj->archive); - ze_obj->archive = NULL; ze_obj->za = NULL; } if (ze_obj->filename) { @@ -1598,7 +1631,6 @@ PHP_METHOD(ZipArchive, open) ze_obj->filename = resolved_path; ze_obj->filename_len = strlen(resolved_path); ze_obj->za = intern; - ze_obj->archive = php_zip_archive_create(intern); RETURN_TRUE; } /* }}} */ @@ -1651,23 +1683,22 @@ PHP_METHOD(ZipArchive, close) php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern)); /* Save error for property reader */ #if LIBZIP_VERSION_MAJOR < 1 - zip_error_get(intern, &ze_obj->err_zip, &ze_obj->err_sys); + zip_error_get(intern, &ze_obj->err_zip, &ze_obj->err_sys); #else - { - zip_error_t *ziperr; + { + zip_error_t *ziperr; - ziperr = zip_get_error(intern); - ze_obj->err_zip = zip_error_code_zip(ziperr); - ze_obj->err_sys = zip_error_code_system(ziperr); - zip_error_fini(ziperr); - } + ziperr = zip_get_error(intern); + ze_obj->err_zip = zip_error_code_zip(ziperr); + ze_obj->err_sys = zip_error_code_system(ziperr); + zip_error_fini(ziperr); + } #endif zip_discard(intern); } else { ze_obj->err_zip = 0; ze_obj->err_sys = 0; } - ze_obj->archive->za = NULL; /* clear cache as empty zip are not created but deleted */ php_clear_stat_cache(1, ze_obj->filename, ze_obj->filename_len); @@ -1675,8 +1706,6 @@ PHP_METHOD(ZipArchive, close) efree(ze_obj->filename); ze_obj->filename = NULL; ze_obj->filename_len = 0; - php_zip_archive_release(ze_obj->archive); - ze_obj->archive = NULL; ze_obj->za = NULL; if (!err) { @@ -2027,7 +2056,7 @@ PHP_METHOD(ZipArchive, addFromString) size_t name_len; ze_zip_object *ze_obj; struct zip_source *zs; - char *data; + int pos = 0; zend_long flags = ZIP_FL_OVERWRITE; if (zend_parse_parameters(ZEND_NUM_ARGS(), "sS|l", @@ -2038,17 +2067,20 @@ PHP_METHOD(ZipArchive, addFromString) ZIP_FROM_OBJECT(intern, self); ze_obj = Z_ZIP_P(self); - if (ZSTR_LEN(buffer)) { - data = pemalloc(ZSTR_LEN(buffer), 1); - memcpy(data, ZSTR_VAL(buffer), ZSTR_LEN(buffer)); + if (ze_obj->buffers_cnt) { + ze_obj->buffers = (char **)safe_erealloc(ze_obj->buffers, sizeof(char *), (ze_obj->buffers_cnt+1), 0); + pos = ze_obj->buffers_cnt++; } else { - data = NULL; + ze_obj->buffers = (char **)emalloc(sizeof(char *)); + ze_obj->buffers_cnt++; + pos = 0; } + ze_obj->buffers[pos] = (char *)safe_emalloc(ZSTR_LEN(buffer), 1, 1); + memcpy(ze_obj->buffers[pos], ZSTR_VAL(buffer), ZSTR_LEN(buffer) + 1); - zs = zip_source_buffer(intern, data, ZSTR_LEN(buffer), 1); + zs = zip_source_buffer(intern, ze_obj->buffers[pos], ZSTR_LEN(buffer), 0); if (zs == NULL) { - pefree(data, 1); RETURN_FALSE; } diff --git a/ext/zip/php_zip.h b/ext/zip/php_zip.h index 8c83feb5c4d4..e84d278d8194 100644 --- a/ext/zip/php_zip.h +++ b/ext/zip/php_zip.h @@ -57,11 +57,6 @@ typedef struct _ze_zip_rsrc { typedef zip_rsrc * zip_rsrc_ptr; -typedef struct _php_zip_archive { - struct zip *za; - uint32_t refcount; -} php_zip_archive; - typedef struct _ze_zip_read_rsrc { struct zip_file *zf; struct zip_stat sb; @@ -73,10 +68,11 @@ typedef struct _ze_zip_read_rsrc { /* Extends zend object */ typedef struct _ze_zip_object { struct zip *za; - php_zip_archive *archive; + char **buffers; HashTable *prop_handler; char *filename; int filename_len; + int buffers_cnt; zip_int64_t last_id; int err_zip; int err_sys; @@ -97,9 +93,7 @@ static inline ze_zip_object *php_zip_fetch_object(zend_object *obj) { 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(ze_zip_object *obj, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC); -php_zip_archive *php_zip_archive_create(struct zip *za); -void php_zip_archive_addref(php_zip_archive *archive); -void php_zip_archive_release(php_zip_archive *archive); +bool php_zip_stream_has_owner(php_stream *stream, ze_zip_object *owner); extern const php_stream_wrapper php_stream_zip_wrapper; diff --git a/ext/zip/tests/gh23276.phpt b/ext/zip/tests/gh23276.phpt index 4d8f3c35b504..a3a9900cf152 100644 --- a/ext/zip/tests/gh23276.phpt +++ b/ext/zip/tests/gh23276.phpt @@ -22,17 +22,19 @@ unset($zip); gc_collect_cycles(); var_dump($weakRef->get()); -// An externally held stream must remain usable after archive collection. -$zip = new ZipArchive; +// An external reference to the property stream must keep its archive alive. +$zip = new Holder; $zip->open($filename, ZipArchive::RDONLY); -$stream = $zip->getStream('entry.txt'); +$zip->stream = $zip->getStream('entry.txt'); +$stream = $zip->stream; $weakRef = WeakReference::create($zip); unset($zip); gc_collect_cycles(); -var_dump($weakRef->get() === null); +var_dump($weakRef->get() instanceof ZipArchive); var_dump(stream_get_contents($stream)); fclose($stream); +unset($stream); gc_collect_cycles(); var_dump($weakRef->get()); ?> diff --git a/ext/zip/zip_stream.c b/ext/zip/zip_stream.c index 42f4efbcc444..376dbc4cbffb 100644 --- a/ext/zip/zip_stream.c +++ b/ext/zip/zip_stream.c @@ -34,7 +34,7 @@ struct php_zip_stream_data_t { struct zip_file *zf; size_t cursor; php_stream *stream; - php_zip_archive *archive; + ze_zip_object *owner; }; #define STREAM_DATA_FROM_STREAM() \ @@ -103,9 +103,10 @@ static int php_zip_ops_close(php_stream *stream, int close_handle) } } - if (self->archive) { - php_zip_archive_release(self->archive); - self->archive = 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; @@ -239,6 +240,19 @@ const php_stream_ops php_stream_zipio_ops = { NULL /* set_option */ }; +bool php_zip_stream_has_owner(php_stream *stream, ze_zip_object *owner) +{ + if (stream->ops != &php_stream_zipio_ops +#if LIBZIP_ATLEAST(1,9,1) + && stream->ops != &php_stream_zipio_seek_ops +#endif + ) { + return false; + } + + return stream->abstract && ((struct php_zip_stream_data_t *) stream->abstract)->owner == owner; +} + /* {{{ php_stream_zip_open */ php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC) { @@ -257,12 +271,13 @@ php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const c if (zf) { self = emalloc(sizeof(*self)); - self->za = NULL; + self->za = NULL; /* to keep it open on stream close */ self->zf = zf; self->stream = NULL; self->cursor = 0; - self->archive = obj->archive; - php_zip_archive_addref(self->archive); + /* 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); @@ -348,7 +363,7 @@ php_stream *php_stream_zip_opener(php_stream_wrapper *wrapper, self->zf = zf; self->stream = NULL; self->cursor = 0; - self->archive = NULL; + 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); From 9ee52ae7e6c91b105a57ab5c5ea06597a03c4b74 Mon Sep 17 00:00:00 2001 From: Weilin Du Date: Sat, 15 Aug 2026 18:34:25 +0800 Subject: [PATCH 4/9] A ZipArchive that reports this pin from its get_gc handler has already had the reference consumed if it is being collected. --- ext/zip/zip_stream.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ext/zip/zip_stream.c b/ext/zip/zip_stream.c index 376dbc4cbffb..5ecdeeac3d2a 100644 --- a/ext/zip/zip_stream.c +++ b/ext/zip/zip_stream.c @@ -105,7 +105,13 @@ static int php_zip_ops_close(php_stream *stream, int close_handle) /* the pinned object ref is tied to self, so release it regardless of close_handle */ if (self->owner) { - OBJ_RELEASE(&self->owner->zo); + /* + * A ZipArchive that reports this pin from its get_gc handler has + * already had the reference consumed if it is being collected. + */ + if (GC_TYPE(&self->owner->zo) != IS_NULL) { + OBJ_RELEASE(&self->owner->zo); + } self->owner = NULL; } efree(self); From 11f2ffa4ea6ce1f4f662c4809f84f55e459c3e75 Mon Sep 17 00:00:00 2001 From: Weilin Du Date: Sat, 15 Aug 2026 19:01:19 +0800 Subject: [PATCH 5/9] Fix memory leak --- ext/zip/zip_stream.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ext/zip/zip_stream.c b/ext/zip/zip_stream.c index 5ecdeeac3d2a..15fa6e17462b 100644 --- a/ext/zip/zip_stream.c +++ b/ext/zip/zip_stream.c @@ -106,10 +106,12 @@ static int php_zip_ops_close(php_stream *stream, int close_handle) /* the pinned object ref is tied to self, so release it regardless of close_handle */ if (self->owner) { /* - * A ZipArchive that reports this pin from its get_gc handler has - * already had the reference consumed if it is being collected. + * The GC consumes the reference reported by ZipArchive::get_gc() + * while collecting an otherwise unreachable archive. In that case, + * releasing it again would underflow the refcount. A non-zero + * refcount means that this stream is being closed normally. */ - if (GC_TYPE(&self->owner->zo) != IS_NULL) { + if (GC_REFCOUNT(&self->owner->zo) != 0) { OBJ_RELEASE(&self->owner->zo); } self->owner = NULL; From 4ebda237f2da546cc47ec92e7d51a5ff1764885b Mon Sep 17 00:00:00 2001 From: Weilin Du Date: Sat, 15 Aug 2026 19:02:36 +0800 Subject: [PATCH 6/9] Zip: Track GC-visible stream owner pins --- ext/zip/php_zip.c | 1 + ext/zip/php_zip.h | 1 + ext/zip/zip_stream.c | 16 +++++++++++++--- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 981425241ff4..5490cf09afd3 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -1087,6 +1087,7 @@ static HashTable *php_zip_get_gc(zend_object *object, zval **gc_data, int *gc_da if (gc_buffer == NULL) { gc_buffer = zend_get_gc_buffer_create(); } + php_zip_stream_mark_owner_gc_visible(stream, obj); zend_get_gc_buffer_add_obj(gc_buffer, object); } } ZEND_HASH_FOREACH_END(); diff --git a/ext/zip/php_zip.h b/ext/zip/php_zip.h index e84d278d8194..9d9965585e9b 100644 --- a/ext/zip/php_zip.h +++ b/ext/zip/php_zip.h @@ -94,6 +94,7 @@ static inline ze_zip_object *php_zip_fetch_object(zend_object *obj) { 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(ze_zip_object *obj, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC); bool php_zip_stream_has_owner(php_stream *stream, ze_zip_object *owner); +void php_zip_stream_mark_owner_gc_visible(php_stream *stream, ze_zip_object *owner); extern const php_stream_wrapper php_stream_zip_wrapper; diff --git a/ext/zip/zip_stream.c b/ext/zip/zip_stream.c index 15fa6e17462b..5f93c298d848 100644 --- a/ext/zip/zip_stream.c +++ b/ext/zip/zip_stream.c @@ -35,6 +35,7 @@ struct php_zip_stream_data_t { size_t cursor; php_stream *stream; ze_zip_object *owner; + bool owner_gc_visible; }; #define STREAM_DATA_FROM_STREAM() \ @@ -108,10 +109,10 @@ static int php_zip_ops_close(php_stream *stream, int close_handle) /* * The GC consumes the reference reported by ZipArchive::get_gc() * while collecting an otherwise unreachable archive. In that case, - * releasing it again would underflow the refcount. A non-zero - * refcount means that this stream is being closed normally. + * releasing it again would underflow the refcount. Only skip the + * release for a pin that was actually reported to the GC. */ - if (GC_REFCOUNT(&self->owner->zo) != 0) { + if (!self->owner_gc_visible || GC_TYPE(&self->owner->zo) != IS_NULL) { OBJ_RELEASE(&self->owner->zo); } self->owner = NULL; @@ -261,6 +262,13 @@ bool php_zip_stream_has_owner(php_stream *stream, ze_zip_object *owner) return stream->abstract && ((struct php_zip_stream_data_t *) stream->abstract)->owner == owner; } +void php_zip_stream_mark_owner_gc_visible(php_stream *stream, ze_zip_object *owner) +{ + if (php_zip_stream_has_owner(stream, owner)) { + ((struct php_zip_stream_data_t *) stream->abstract)->owner_gc_visible = true; + } +} + /* {{{ php_stream_zip_open */ php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC) { @@ -285,6 +293,7 @@ php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const c self->cursor = 0; /* keep the archive object alive while the stream borrows its zip_t */ self->owner = obj; + self->owner_gc_visible = false; GC_ADDREF(&obj->zo); #if LIBZIP_ATLEAST(1,9,1) if (zip_file_is_seekable(zf) > 0) { @@ -372,6 +381,7 @@ php_stream *php_stream_zip_opener(php_stream_wrapper *wrapper, self->stream = NULL; self->cursor = 0; self->owner = NULL; + self->owner_gc_visible = false; #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); From 97bf7a37f0d45eec5e433e0a3919812aa92a12c1 Mon Sep 17 00:00:00 2001 From: Weilin Du Date: Sat, 15 Aug 2026 19:09:59 +0800 Subject: [PATCH 7/9] Zip: Test globally held streams in GH-23276 --- ext/zip/tests/gh23276.phpt | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/ext/zip/tests/gh23276.phpt b/ext/zip/tests/gh23276.phpt index a3a9900cf152..06827cf30d72 100644 --- a/ext/zip/tests/gh23276.phpt +++ b/ext/zip/tests/gh23276.phpt @@ -37,6 +37,21 @@ fclose($stream); unset($stream); gc_collect_cycles(); var_dump($weakRef->get()); + +// A global reference has the same lifetime requirements as a local one. +$zip = new Holder; +$zip->open($filename, ZipArchive::RDONLY); +$zip->stream = $zip->getStream('entry.txt'); +$GLOBALS['gh23276_stream'] = $zip->stream; +$weakRef = WeakReference::create($zip); +unset($zip); + +gc_collect_cycles(); +var_dump($weakRef->get() instanceof ZipArchive); +fclose($GLOBALS['gh23276_stream']); +unset($GLOBALS['gh23276_stream']); +gc_collect_cycles(); +var_dump($weakRef->get()); ?> --CLEAN-- Date: Sat, 15 Aug 2026 19:11:21 +0800 Subject: [PATCH 8/9] Zip: Verify closing globally held stream releases owner --- ext/zip/tests/gh23276.phpt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ext/zip/tests/gh23276.phpt b/ext/zip/tests/gh23276.phpt index 06827cf30d72..92d3b3e76610 100644 --- a/ext/zip/tests/gh23276.phpt +++ b/ext/zip/tests/gh23276.phpt @@ -49,9 +49,8 @@ unset($zip); gc_collect_cycles(); var_dump($weakRef->get() instanceof ZipArchive); fclose($GLOBALS['gh23276_stream']); -unset($GLOBALS['gh23276_stream']); -gc_collect_cycles(); var_dump($weakRef->get()); +unset($GLOBALS['gh23276_stream']); ?> --CLEAN-- Date: Sat, 15 Aug 2026 19:36:03 +0800 Subject: [PATCH 9/9] Zip: Keep get_gc stream edges stable per collection --- ext/zip/php_zip.c | 6 ++++-- ext/zip/php_zip.h | 2 +- ext/zip/tests/gh23276.phpt | 20 ++++++++++++++++++++ ext/zip/zip_stream.c | 33 +++++++++++++++++++++------------ 4 files changed, 46 insertions(+), 15 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 5490cf09afd3..6056291f43f1 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -1048,8 +1048,10 @@ static HashTable *php_zip_get_gc(zend_object *object, zval **gc_data, int *gc_da HashTable *properties = zend_std_get_properties(object); HashTable streams; zend_get_gc_buffer *gc_buffer = NULL; + zend_gc_status gc_status; zval *value; + zend_gc_get_status(&gc_status); zend_hash_init(&streams, 0, NULL, NULL, 0); ZEND_HASH_FOREACH_VAL_IND(properties, value) { php_stream *stream; @@ -1083,11 +1085,11 @@ static HashTable *php_zip_get_gc(zend_object *object, zval **gc_data, int *gc_da * when the resource is held exclusively by this object's properties; * an externally-held stream must keep its owner alive. */ - if (GC_REFCOUNT(resource) == property_refs) { + if (php_zip_stream_owner_is_gc_visible( + stream, obj, gc_status.runs, GC_REFCOUNT(resource) == property_refs)) { if (gc_buffer == NULL) { gc_buffer = zend_get_gc_buffer_create(); } - php_zip_stream_mark_owner_gc_visible(stream, obj); zend_get_gc_buffer_add_obj(gc_buffer, object); } } ZEND_HASH_FOREACH_END(); diff --git a/ext/zip/php_zip.h b/ext/zip/php_zip.h index 9d9965585e9b..bfd38c975d5c 100644 --- a/ext/zip/php_zip.h +++ b/ext/zip/php_zip.h @@ -94,7 +94,7 @@ static inline ze_zip_object *php_zip_fetch_object(zend_object *obj) { 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(ze_zip_object *obj, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC); bool php_zip_stream_has_owner(php_stream *stream, ze_zip_object *owner); -void php_zip_stream_mark_owner_gc_visible(php_stream *stream, ze_zip_object *owner); +bool php_zip_stream_owner_is_gc_visible(php_stream *stream, ze_zip_object *owner, uint32_t gc_run, bool visible); extern const php_stream_wrapper php_stream_zip_wrapper; diff --git a/ext/zip/tests/gh23276.phpt b/ext/zip/tests/gh23276.phpt index 92d3b3e76610..30523b24161a 100644 --- a/ext/zip/tests/gh23276.phpt +++ b/ext/zip/tests/gh23276.phpt @@ -8,6 +8,12 @@ class Holder extends ZipArchive { public $stream; } +class ResurrectingHolder extends Holder { + public function __destruct() { + $GLOBALS['gh23276_zombie'] = $this; + } +} + $filename = __DIR__ . '/gh23276.zip'; $zip = new Holder; @@ -51,6 +57,18 @@ var_dump($weakRef->get() instanceof ZipArchive); fclose($GLOBALS['gh23276_stream']); var_dump($weakRef->get()); unset($GLOBALS['gh23276_stream']); + +// Resurrecting the archive from its destructor must preserve the stream pin. +$zip = new ResurrectingHolder; +$zip->open($filename, ZipArchive::RDONLY); +$zip->stream = $zip->getStream('entry.txt'); +unset($zip); +gc_collect_cycles(); + +var_dump($GLOBALS['gh23276_zombie'] instanceof ZipArchive); +var_dump(stream_get_contents($GLOBALS['gh23276_zombie']->stream)); +unset($GLOBALS['gh23276_zombie']); +gc_collect_cycles(); ?> --CLEAN-- owner) { - /* - * The GC consumes the reference reported by ZipArchive::get_gc() - * while collecting an otherwise unreachable archive. In that case, - * releasing it again would underflow the refcount. Only skip the - * release for a pin that was actually reported to the GC. - */ - if (!self->owner_gc_visible || GC_TYPE(&self->owner->zo) != IS_NULL) { - OBJ_RELEASE(&self->owner->zo); - } + OBJ_RELEASE(&self->owner->zo); self->owner = NULL; } efree(self); @@ -262,11 +255,25 @@ bool php_zip_stream_has_owner(php_stream *stream, ze_zip_object *owner) return stream->abstract && ((struct php_zip_stream_data_t *) stream->abstract)->owner == owner; } -void php_zip_stream_mark_owner_gc_visible(php_stream *stream, ze_zip_object *owner) +bool php_zip_stream_owner_is_gc_visible(php_stream *stream, ze_zip_object *owner, uint32_t gc_run, bool visible) { - if (php_zip_stream_has_owner(stream, owner)) { - ((struct php_zip_stream_data_t *) stream->abstract)->owner_gc_visible = true; + struct php_zip_stream_data_t *self; + + if (!php_zip_stream_has_owner(stream, owner)) { + return false; } + + self = (struct php_zip_stream_data_t *) stream->abstract; + /* + * get_gc() is called repeatedly while the collector temporarily adjusts + * refcounts. Keep the first decision stable throughout one GC run. + */ + if (self->owner_gc_run != gc_run) { + self->owner_gc_run = gc_run; + self->owner_gc_visible = visible; + } + + return self->owner_gc_visible; } /* {{{ php_stream_zip_open */ @@ -293,6 +300,7 @@ php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const c self->cursor = 0; /* keep the archive object alive while the stream borrows its zip_t */ self->owner = obj; + self->owner_gc_run = 0; self->owner_gc_visible = false; GC_ADDREF(&obj->zo); #if LIBZIP_ATLEAST(1,9,1) @@ -381,6 +389,7 @@ php_stream *php_stream_zip_opener(php_stream_wrapper *wrapper, self->stream = NULL; self->cursor = 0; self->owner = NULL; + self->owner_gc_run = 0; self->owner_gc_visible = false; #if LIBZIP_ATLEAST(1,9,1) if (zip_file_is_seekable(zf) > 0) {