From 3e80499fa92a87a165e52c199fa9a970bf4e1435 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Fri, 29 May 2026 07:00:54 +0100 Subject: [PATCH 1/3] ext/zip: memory leak when zip cancel callback bails out. Fix #22176 A cancel callback that throws during the implicit zip_close() in the shutdown destructor triggers a zend_bailout that longjmps through libzip, skipping its free(filelist). Wrap the call in zend_try/zend_catch and cancel on bailout so libzip can unwind and clean up. While at it, apply the same guard to the progress callback, which is invoked from libzip the same way and is prone to the identical leak. close GH-22177 --- ext/zip/php_zip.c | 13 +++++++-- ext/zip/tests/gh22176.phpt | 59 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) create mode 100644 ext/zip/tests/gh22176.phpt diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 5df4d47740a8..9cc7ffdc4349 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -2993,7 +2993,10 @@ static void php_zip_progress_callback(zip_t *arch, double state, void *ptr) ze_zip_object *obj = ptr; ZVAL_DOUBLE(&cb_args[0], state); - zend_call_known_fcc(&obj->progress_callback, NULL, 1, cb_args, NULL); + + zend_try { + zend_call_known_fcc(&obj->progress_callback, NULL, 1, cb_args, NULL); + } zend_end_try(); } /* {{{ register a progression callback: void callback(double state); */ @@ -3040,7 +3043,13 @@ static int php_zip_cancel_callback(zip_t *arch, void *ptr) return 0; } - zend_call_known_fcc(&obj->cancel_callback, &cb_retval, 0, NULL, NULL); + zend_try { + zend_call_known_fcc(&obj->cancel_callback, &cb_retval, 0, NULL, NULL); + } zend_catch { + /* Cancel if a bailout occurs to allow cleanup to happen */ + return -1; + } zend_end_try(); + if (Z_ISUNDEF(cb_retval)) { /* Cancel if an exception has been thrown */ return -1; diff --git a/ext/zip/tests/gh22176.phpt b/ext/zip/tests/gh22176.phpt new file mode 100644 index 000000000000..5cc8b640a9ff --- /dev/null +++ b/ext/zip/tests/gh22176.phpt @@ -0,0 +1,59 @@ +--TEST-- +GH-22176 (Memory leak when a ZipArchive cancel/progress callback bails out in the shutdown destructor) +--EXTENSIONS-- +zip +--SKIPIF-- + +--FILE-- +open($dir . 'gh22176_cancel.zip', ZipArchive::CREATE); +$cancel->registerCancelCallback(function () { + throw new \Exception('cancel boom'); +}); +$cancel->addFromString('test', 'test'); + +$progress = new ZipArchive; +$progress->open($dir . 'gh22176_progress.zip', ZipArchive::CREATE); +$progress->registerProgressCallback(0.5, function ($r) { + throw new \Exception('progress boom'); +}); +$progress->addFromString('test', 'test'); + +echo "done\n"; +// Both archives are flushed and the objects destroyed during request +// shutdown; the thrown exceptions bail out through libzip's zip_close() +// without leaking its internal state. +?> +--CLEAN-- + +--EXPECTF-- +done + +Fatal error: Uncaught Exception: progress boom in %s:%d +Stack trace: +#0 [internal function]: {closure:%s:%d}(0.0) +#1 {main} + thrown in %s on line %d + +Fatal error: Uncaught Exception: progress boom in %s:%d +Stack trace: +#0 [internal function]: {closure:%s:%d}(1.0) +#1 {main} + thrown in %s on line %d + +Fatal error: Uncaught Exception: cancel boom in %s:%d +Stack trace: +#0 [internal function]: {closure:%s:%d}() +#1 {main} + thrown in %s on line %d + +Warning: PHP Request Shutdown: Cannot destroy the zip context: %s in Unknown on line 0 From 299227b5b761551f45403a3e044927b2fbf40751 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Fri, 29 May 2026 10:52:02 +0100 Subject: [PATCH 2/3] feedback then now we have to exercise the two cases separately. --- ext/zip/php_zip.c | 24 ++++++++++++++--- ext/zip/php_zip.h | 1 + ext/zip/tests/gh22176.phpt | 42 ++++++----------------------- ext/zip/tests/gh22176_progress.phpt | 33 +++++++++++++++++++++++ 4 files changed, 62 insertions(+), 38 deletions(-) create mode 100644 ext/zip/tests/gh22176_progress.phpt diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 9cc7ffdc4349..00a0153358cc 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -628,6 +628,12 @@ static bool php_zipobj_close(ze_zip_object *obj, zend_string **out_str) /* {{{ * obj->za = NULL; obj->from_string = false; + + if (obj->bailout_callback) { + obj->bailout_callback = false; + zend_bailout(); + } + return success; } /* }}} */ @@ -1073,10 +1079,16 @@ static void php_zip_object_dtor(zend_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)); + if (!intern->bailout_callback) { + php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(intern->za)); + } zip_discard(intern->za); } intern->za = NULL; + if (intern->bailout_callback) { + intern->bailout_callback = false; + zend_bailout(); + } } } @@ -2985,17 +2997,20 @@ PHP_METHOD(ZipArchive, getStream) #ifdef HAVE_PROGRESS_CALLBACK static void php_zip_progress_callback(zip_t *arch, double state, void *ptr) { - if (!EG(active)) { + ze_zip_object *obj = ptr; + + if (!EG(active) || obj->bailout_callback) { return; } zval cb_args[1]; - ze_zip_object *obj = ptr; ZVAL_DOUBLE(&cb_args[0], state); zend_try { zend_call_known_fcc(&obj->progress_callback, NULL, 1, cb_args, NULL); + } zend_catch { + obj->bailout_callback = true; } zend_end_try(); } @@ -3039,13 +3054,14 @@ static int php_zip_cancel_callback(zip_t *arch, void *ptr) zval cb_retval; ze_zip_object *obj = ptr; - if (!EG(active)) { + if (!EG(active) || obj->bailout_callback) { return 0; } zend_try { zend_call_known_fcc(&obj->cancel_callback, &cb_retval, 0, NULL, NULL); } zend_catch { + obj->bailout_callback = true; /* Cancel if a bailout occurs to allow cleanup to happen */ return -1; } zend_end_try(); diff --git a/ext/zip/php_zip.h b/ext/zip/php_zip.h index 9e57a54de1cc..408c29708231 100644 --- a/ext/zip/php_zip.h +++ b/ext/zip/php_zip.h @@ -74,6 +74,7 @@ typedef struct _ze_zip_object { zip_int64_t last_id; int err_zip; int err_sys; + bool bailout_callback; #ifdef HAVE_PROGRESS_CALLBACK zend_fcall_info_cache progress_callback; #endif diff --git a/ext/zip/tests/gh22176.phpt b/ext/zip/tests/gh22176.phpt index 5cc8b640a9ff..90b6327072c9 100644 --- a/ext/zip/tests/gh22176.phpt +++ b/ext/zip/tests/gh22176.phpt @@ -1,59 +1,33 @@ --TEST-- -GH-22176 (Memory leak when a ZipArchive cancel/progress callback bails out in the shutdown destructor) +GH-22176 (Memory leak when a ZipArchive cancel callback bails out in the shutdown destructor) --EXTENSIONS-- zip --SKIPIF-- --FILE-- open($dir . 'gh22176_cancel.zip', ZipArchive::CREATE); -$cancel->registerCancelCallback(function () { +$zip = new ZipArchive; +$zip->open(__DIR__ . '/gh22176_cancel.zip', ZipArchive::CREATE); +$zip->registerCancelCallback(function () { throw new \Exception('cancel boom'); }); -$cancel->addFromString('test', 'test'); - -$progress = new ZipArchive; -$progress->open($dir . 'gh22176_progress.zip', ZipArchive::CREATE); -$progress->registerProgressCallback(0.5, function ($r) { - throw new \Exception('progress boom'); -}); -$progress->addFromString('test', 'test'); - +$zip->addFromString('test', 'test'); echo "done\n"; -// Both archives are flushed and the objects destroyed during request -// shutdown; the thrown exceptions bail out through libzip's zip_close() -// without leaking its internal state. +// The archive is flushed and the object destroyed during request shutdown; +// the thrown exception bails out through libzip's zip_close() without leaking +// its internal state, and the bailout resumes once libzip has unwound. ?> --CLEAN-- --EXPECTF-- done -Fatal error: Uncaught Exception: progress boom in %s:%d -Stack trace: -#0 [internal function]: {closure:%s:%d}(0.0) -#1 {main} - thrown in %s on line %d - -Fatal error: Uncaught Exception: progress boom in %s:%d -Stack trace: -#0 [internal function]: {closure:%s:%d}(1.0) -#1 {main} - thrown in %s on line %d - Fatal error: Uncaught Exception: cancel boom in %s:%d Stack trace: #0 [internal function]: {closure:%s:%d}() #1 {main} thrown in %s on line %d - -Warning: PHP Request Shutdown: Cannot destroy the zip context: %s in Unknown on line 0 diff --git a/ext/zip/tests/gh22176_progress.phpt b/ext/zip/tests/gh22176_progress.phpt new file mode 100644 index 000000000000..5507fe24ab98 --- /dev/null +++ b/ext/zip/tests/gh22176_progress.phpt @@ -0,0 +1,33 @@ +--TEST-- +GH-22176 (Memory leak when a ZipArchive progress callback bails out in the shutdown destructor) +--EXTENSIONS-- +zip +--SKIPIF-- + +--FILE-- +open(__DIR__ . '/gh22176_progress.zip', ZipArchive::CREATE); +$zip->registerProgressCallback(0.5, function ($r) { + throw new \Exception('progress boom'); +}); +$zip->addFromString('test', 'test'); +echo "done\n"; +// The archive is flushed and the object destroyed during request shutdown; +// the thrown exception bails out through libzip's zip_close() without leaking +// its internal state, and the bailout resumes once libzip has unwound. +?> +--CLEAN-- + +--EXPECTF-- +done + +Fatal error: Uncaught Exception: progress boom in %s:%d +Stack trace: +#0 [internal function]: {closure:%s:%d}(0.0) +#1 {main} + thrown in %s on line %d From 1d5f74f9146943df64885a0bc5230bb9184946ec Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sun, 5 Jul 2026 11:29:04 +0100 Subject: [PATCH 3/3] feedback --- ext/zip/php_zip.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 00a0153358cc..9bc801455ed6 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -2999,7 +2999,7 @@ static void php_zip_progress_callback(zip_t *arch, double state, void *ptr) { ze_zip_object *obj = ptr; - if (!EG(active) || obj->bailout_callback) { + if (UNEXPECTED(!EG(active) || obj->bailout_callback)) { return; } @@ -3054,7 +3054,7 @@ static int php_zip_cancel_callback(zip_t *arch, void *ptr) zval cb_retval; ze_zip_object *obj = ptr; - if (!EG(active) || obj->bailout_callback) { + if (UNEXPECTED(!EG(active) || obj->bailout_callback)) { return 0; }