From eb8588319f1806552d05019d1cc66519418b5e65 Mon Sep 17 00:00:00 2001 From: jvoisin Date: Sat, 19 Sep 2026 23:03:25 +0200 Subject: [PATCH 1/2] ext/zip: fix use-after-free in the archive destructor path Commit 132403de396 guarded php_zipobj_close() and ZipArchive::open() with the archive->close flag so that a progress or cancel callback firing during zip_close() cannot re-enter and run a nested zip_close() followed by zip_discard(), which frees the archive still in use. php_zip_archive_release() was left unguarded. php_zipobj_close() sets archive->za = NULL before calling release(), so the close() path is safe, but when a ZipArchive is destroyed without an explicit close() the archive is finalized here with za still set. The progress/cancel callback fires during that zip_close(), and a re-entrant close() or open() then nests zip_close() on the same archive, causing the same use-after-free. Set archive->close around the release-path zip_close() as well, so the re-entrant call throws "Already being closed" instead of nesting. --- ext/zip/php_zip.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 88fdcaa9b03e..6dd482ac580c 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -1169,7 +1169,13 @@ bool php_zip_archive_release(php_zip_archive *archive) } if (archive->za) { - if (zip_close(archive->za) != 0) { + /* Guard against a re-entrant close() or open() from a progress/cancel + * callback fired during zip_close(), which would run a nested zip_close() + * on the same archive (see php_zipobj_close()). */ + archive->close = true; + int err = zip_close(archive->za); + archive->close = false; + if (err != 0) { if (!archive->bailout_callback) { php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(archive->za)); } From 80887025df830310682d6b0b54451bcd68396a88 Mon Sep 17 00:00:00 2001 From: jvoisin Date: Mon, 21 Sep 2026 11:29:25 +0200 Subject: [PATCH 2/2] ext/zip: guard getStream() and callback destructors against re-entrant close Follow-up to the archive destructor use-after-free fix, plugging two more paths where a progress or cancel callback firing during zip_close() can re-enter the archive. getStream() reached php_zip_archive_addref() with the archive already being closed (refcount 0), tripping the ZEND_ASSERT(refcount > 0). Guard it with php_zipobj_closing() so the call throws "Already being closed" instead. Guard php_zip_progress_callback_free() and php_zip_cancel_callback_free() like their callback counterparts, so the FCC destructor is not run while the engine is inactive or in a bailout. --- ext/zip/php_zip.c | 15 ++++++- ext/zip/tests/gh23747_close_error.phpt | 48 +++++++++++++++++++++ ext/zip/tests/gh23747_dtor.phpt | 58 ++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 2 deletions(-) create mode 100644 ext/zip/tests/gh23747_close_error.phpt create mode 100644 ext/zip/tests/gh23747_dtor.phpt diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 6dd482ac580c..72dd3a1db691 100644 --- a/ext/zip/php_zip.c +++ b/ext/zip/php_zip.c @@ -659,7 +659,6 @@ static bool php_zipobj_close(ze_zip_object *obj, zend_string **out_str) /* {{{ * if (intern) { archive->close = true; int err = zip_close(intern); - archive->close = false; if (err) { php_error_docref(NULL, E_WARNING, "%s", zip_strerror(intern)); /* Save error for property reader */ @@ -698,6 +697,7 @@ static bool php_zipobj_close(ze_zip_object *obj, zend_string **out_str) /* {{{ * if (archive) { archive->za = NULL; + archive->close = false; bailout = archive->bailout_callback; archive->bailout_callback = false; obj->archive = NULL; @@ -1128,6 +1128,10 @@ static void php_zip_progress_callback_free(void *ptr) { php_zip_archive *archive = ptr; + if (UNEXPECTED(!EG(active) || archive->bailout_callback)) { + return; + } + if (ZEND_FCC_INITIALIZED(archive->progress_callback)) { zend_fcc_dtor(&archive->progress_callback); } @@ -1139,6 +1143,10 @@ static void php_zip_cancel_callback_free(void *ptr) { php_zip_archive *archive = ptr; + if (UNEXPECTED(!EG(active) || archive->bailout_callback)) { + return; + } + if (ZEND_FCC_INITIALIZED(archive->cancel_callback)) { zend_fcc_dtor(&archive->cancel_callback); } @@ -1174,7 +1182,6 @@ bool php_zip_archive_release(php_zip_archive *archive) * on the same archive (see php_zipobj_close()). */ archive->close = true; int err = zip_close(archive->za); - archive->close = false; if (err != 0) { if (!archive->bailout_callback) { php_error_docref(NULL, E_WARNING, "Cannot destroy the zip context: %s", zip_strerror(archive->za)); @@ -3137,6 +3144,10 @@ static void php_zip_get_stream(INTERNAL_FUNCTION_PARAMETERS, int type, bool acce ZIP_FROM_OBJECT(intern, self); + if (php_zipobj_closing(Z_ZIP_P(self))) { + RETURN_THROWS(); + } + if (type) { PHP_ZIP_STAT_PATH(intern, ZSTR_VAL(filename), ZSTR_LEN(filename), flags, sb); } else { diff --git a/ext/zip/tests/gh23747_close_error.phpt b/ext/zip/tests/gh23747_close_error.phpt new file mode 100644 index 000000000000..b5d35544a0e7 --- /dev/null +++ b/ext/zip/tests/gh23747_close_error.phpt @@ -0,0 +1,48 @@ +--TEST-- +GH-23747 (Re-entrant close from a ZipArchive close warning is rejected) +--EXTENSIONS-- +zip +--FILE-- +open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE); + $zip->addFile($source, 'file.txt'); + unlink($source); + + $weak = WeakReference::create($zip); + set_error_handler(static function (int $errno, string $message) use ($weak): bool { + try { + $weak->get()->close(); + } catch (Error $error) { + echo $error::class, ': ', $error->getMessage(), "\n"; + } + return true; + }); + + if ($operation === 'close') { + var_dump($zip->close()); + } else { + unset($zip); + echo "destroyed\n"; + } + restore_error_handler(); +} +?> +--CLEAN-- + +--EXPECT-- +close: +Error: Already being closed +bool(false) +destruct: +Error: Already being closed +destroyed \ No newline at end of file diff --git a/ext/zip/tests/gh23747_dtor.phpt b/ext/zip/tests/gh23747_dtor.phpt new file mode 100644 index 000000000000..f1858c1b110c --- /dev/null +++ b/ext/zip/tests/gh23747_dtor.phpt @@ -0,0 +1,58 @@ +--TEST-- +GH-23747 (Re-entrant operations during ZipArchive destruction are rejected) +--EXTENSIONS-- +zip +--SKIPIF-- + +--FILE-- +open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE); + for ($index = 0; $index < 64; $index++) { + $zip->addFromString("f$index.txt", str_repeat('x', 2000)); + } + + $weak = WeakReference::create($zip); + $callback = static function (float $rate) use ($weak, $method, $arguments): void { + static $done = false; + if ($done) { + return; + } + $done = true; + + try { + $weak->get()->$method(...$arguments); + } catch (Error $error) { + echo $method, ': ', $error->getMessage(), "\n"; + } + }; + $zip->registerProgressCallback(0.0, $callback); + unset($zip); + echo "destroyed\n"; +} +?> +--CLEAN-- + +--EXPECT-- +close: Already being closed +destroyed +getStream: Already being closed +destroyed +getStreamName: Already being closed +destroyed +getStreamIndex: Already being closed +destroyed \ No newline at end of file