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..6056291f43f1 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -1044,9 +1044,65 @@ 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; + 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; + 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 (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(); + } + 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; } /* }}} */ diff --git a/ext/zip/php_zip.h b/ext/zip/php_zip.h index 84fdd21e3476..bfd38c975d5c 100644 --- a/ext/zip/php_zip.h +++ b/ext/zip/php_zip.h @@ -93,6 +93,8 @@ 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); +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 new file mode 100644 index 000000000000..30523b24161a --- /dev/null +++ b/ext/zip/tests/gh23276.phpt @@ -0,0 +1,85 @@ +--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 external reference to the property stream must keep its archive alive. +$zip = new Holder; +$zip->open($filename, ZipArchive::RDONLY); +$zip->stream = $zip->getStream('entry.txt'); +$stream = $zip->stream; +$weakRef = WeakReference::create($zip); +unset($zip); + +gc_collect_cycles(); +var_dump($weakRef->get() instanceof ZipArchive); +var_dump(stream_get_contents($stream)); +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']); +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-- + +--EXPECT-- +NULL +bool(true) +string(8) "contents" +NULL +bool(true) +NULL +bool(true) +string(8) "contents" diff --git a/ext/zip/zip_stream.c b/ext/zip/zip_stream.c index 7f6990962d00..1ceb7950a81f 100644 --- a/ext/zip/zip_stream.c +++ b/ext/zip/zip_stream.c @@ -35,6 +35,8 @@ struct php_zip_stream_data_t { size_t cursor; php_stream *stream; ze_zip_object *owner; + uint32_t owner_gc_run; + bool owner_gc_visible; }; #define STREAM_DATA_FROM_STREAM() \ @@ -240,6 +242,40 @@ 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; +} + +bool php_zip_stream_owner_is_gc_visible(php_stream *stream, ze_zip_object *owner, uint32_t gc_run, bool visible) +{ + 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 */ php_stream *php_stream_zip_open(ze_zip_object *obj, struct zip_stat *sb, const char *mode, zip_flags_t flags STREAMS_DC) { @@ -264,6 +300,8 @@ 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) if (zip_file_is_seekable(zf) > 0) { @@ -351,6 +389,8 @@ 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) { stream = php_stream_alloc(&php_stream_zipio_seek_ops, self, NULL, mode);