From 9895509a12bae6e96be26054824bfe3d2972ca3f Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Wed, 27 Aug 2025 05:37:45 +0200 Subject: [PATCH 1/2] Use dtor instead of free_obj for ZipArchive free_obj for objects referenced in the main symbol table may be called only once the executor has already shut down. php_zip_cancel_callback() may attempt to invoke a user callback, which will terminate the process because user code is not expected to be executed at this point. We solve this by using the dtor_obj handler instead, which is called earlier in the shutdown sequence. Technically, this allows some shenanigans with escaping __destruct functions, but given destructors are useful, this may not be worth solving. For reference: class MyZipArchive extends ZipArchive { public function __destruct() { global $leak; $leak = $this; } } new MyZipArchive; $file = __DIR__ . '/gh18907.zip'; $leak->open($file, ZIPARCHIVE::CREATE); $leak->addFromString('test', 'test'); --- ext/zip/php_zip.c | 7 ++++--- ext/zip/tests/ZipArchive_bailout.phpt | 28 +++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) create mode 100644 ext/zip/tests/ZipArchive_bailout.phpt diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index d5f7b019eb9ea..fcb9ae180b4ca 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -1004,8 +1004,10 @@ static void php_zip_cancel_callback_free(void *ptr) } #endif -static void php_zip_object_free_storage(zend_object *object) /* {{{ */ +static void php_zip_object_dtor(zend_object *object) /* {{{ */ { + zend_objects_destroy_object(object); + ze_zip_object * intern = php_zip_fetch_object(object); int i; @@ -1034,7 +1036,6 @@ static void php_zip_object_free_storage(zend_object *object) /* {{{ */ #endif intern->za = NULL; - zend_object_std_dtor(&intern->zo); if (intern->filename) { efree(intern->filename); @@ -3119,7 +3120,7 @@ static PHP_MINIT_FUNCTION(zip) { memcpy(&zip_object_handlers, &std_object_handlers, sizeof(zend_object_handlers)); zip_object_handlers.offset = XtOffsetOf(ze_zip_object, zo); - zip_object_handlers.free_obj = php_zip_object_free_storage; + zip_object_handlers.dtor_obj = php_zip_object_dtor; zip_object_handlers.clone_obj = NULL; zip_object_handlers.get_property_ptr_ptr = php_zip_get_property_ptr_ptr; diff --git a/ext/zip/tests/ZipArchive_bailout.phpt b/ext/zip/tests/ZipArchive_bailout.phpt new file mode 100644 index 0000000000000..c7e4ede8446df --- /dev/null +++ b/ext/zip/tests/ZipArchive_bailout.phpt @@ -0,0 +1,28 @@ +--TEST-- +ZipArchive destructor should be called on bailout +--EXTENSIONS-- +zip +--FILE-- +open($file, ZIPARCHIVE::CREATE); +$zip->registerCancelCallback(cb(...)); +$zip->addFromString('test', 'test'); +$fusion = $zip; + +?> +--CLEAN-- + +--EXPECTF-- +Notice: Only variable references should be returned by reference in %s on line %d + +Notice: Only variable references should be returned by reference in %s on line %d + +Notice: Only variable references should be returned by reference in %s on line %d From 8ecada2afe33b91706d9348697ae742475060681 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Thu, 28 Aug 2025 15:56:05 +0200 Subject: [PATCH 2/2] Change approach --- ext/zip/php_zip.c | 25 ++++++++++++++++++++++- ext/zip/tests/ZipArchive_destruct.phpt | 28 ++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 ext/zip/tests/ZipArchive_destruct.phpt diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index fcb9ae180b4ca..f8e00650e6b65 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -1004,10 +1004,23 @@ static void php_zip_cancel_callback_free(void *ptr) } #endif -static void php_zip_object_dtor(zend_object *object) /* {{{ */ +static void php_zip_object_dtor(zend_object *object) { zend_objects_destroy_object(object); + ze_zip_object *intern = php_zip_fetch_object(object); + + 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); + } + intern->za = NULL; + } +} + +static void php_zip_object_free_storage(zend_object *object) /* {{{ */ +{ ze_zip_object * intern = php_zip_fetch_object(object); int i; @@ -1036,6 +1049,7 @@ static void php_zip_object_dtor(zend_object *object) /* {{{ */ #endif intern->za = NULL; + zend_object_std_dtor(&intern->zo); if (intern->filename) { efree(intern->filename); @@ -2988,6 +3002,10 @@ PHP_METHOD(ZipArchive, getStream) #ifdef HAVE_PROGRESS_CALLBACK static void php_zip_progress_callback(zip_t *arch, double state, void *ptr) { + if (!EG(active)) { + return; + } + zval cb_args[1]; ze_zip_object *obj = ptr; @@ -3034,6 +3052,10 @@ static int php_zip_cancel_callback(zip_t *arch, void *ptr) zval cb_retval; ze_zip_object *obj = ptr; + if (!EG(active)) { + return 0; + } + zend_call_known_fcc(&obj->cancel_callback, &cb_retval, 0, NULL, NULL); if (Z_ISUNDEF(cb_retval)) { /* Cancel if an exception has been thrown */ @@ -3120,6 +3142,7 @@ static PHP_MINIT_FUNCTION(zip) { memcpy(&zip_object_handlers, &std_object_handlers, sizeof(zend_object_handlers)); zip_object_handlers.offset = XtOffsetOf(ze_zip_object, zo); + zip_object_handlers.free_obj = php_zip_object_free_storage; zip_object_handlers.dtor_obj = php_zip_object_dtor; zip_object_handlers.clone_obj = NULL; zip_object_handlers.get_property_ptr_ptr = php_zip_get_property_ptr_ptr; diff --git a/ext/zip/tests/ZipArchive_destruct.phpt b/ext/zip/tests/ZipArchive_destruct.phpt new file mode 100644 index 0000000000000..7e3ef6c5dae42 --- /dev/null +++ b/ext/zip/tests/ZipArchive_destruct.phpt @@ -0,0 +1,28 @@ +--TEST-- +Leaking ZipArchive destructor +--EXTENSIONS-- +zip +--FILE-- +open($file, ZIPARCHIVE::CREATE); +$leak->addFromString('test', 'test'); + +?> +===DONE=== +--CLEAN-- + +--EXPECT-- +===DONE===