ext/zip: fix use-after-free in the archive destructor path

[jvoisin]

Commit 132403d 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.

[devnexen]

suggestion (does not have to be in the same PR, but since the goal is to "plug" cases where the zip stream close):

• protecting the getStream() case that can still reach the assert (I think adding php_zipobj_closing check is enough).
• guarding php_zip_progress_callback_free like php_zip_progress_callback does ?

Other than that, the fix itself here is correct from my POV.

[jvoisin]

suggestion (does not have to be in the same PR, but since the goal is to "plug" cases where the zip stream close):

* protecting the getStream() case that can still reach the assert (I think adding php_zipobj_closing check is enough).

* guarding php_zip_progress_callback_free like php_zip_progress_callback does ?

Other than that, the fix itself here is correct from my POV.

Yeah, good points, I piled a commit doing this on top of the initial one

[devnexen]

Could these tests be added ?

this one, normally should pass with your branch

--TEST--
GH-23747 (re-entrant close()/getStream() from a callback fired by the ZipArchive destructor)
--EXTENSIONS--
zip
--SKIPIF--
<?php
if (!method_exists(ZipArchive::class, 'registerProgressCallback')) {
    die('skip progress callbacks are not supported');
}
?>
--FILE--
<?php
function populate(ZipArchive $zip, string $filename): void {
    $zip->open($filename, ZipArchive::CREATE | ZipArchive::OVERWRITE);
    for ($i = 0; $i < 64; $i++) {
        $zip->addFromString("f$i.txt", str_repeat('x', 2000));
    }
}

$filename = __DIR__ . '/gh23747_dtor.zip';

$zip = new ZipArchive();
populate($zip, $filename);
$weak = WeakReference::create($zip);
$zip->registerProgressCallback(0.0, function ($rate) use ($weak) {
    static $done = false;
    if ($done) {
        return;
    }
    $done = true;
    try {
        $weak->get()->close();
    } catch (Error $e) {
        echo $e::class, ': ', $e->getMessage(), PHP_EOL;
    }
});
unset($zip);
echo 'destroyed', PHP_EOL;

$zip = new ZipArchive();
populate($zip, $filename);
$weak = WeakReference::create($zip);
$zip->registerProgressCallback(0.0, function ($rate) use ($weak) {
    static $done = false;
    if ($done) {
        return;
    }
    $done = true;
    try {
        $weak->get()->getStreamName('f0.txt');
    } catch (Error $e) {
        echo $e::class, ': ', $e->getMessage(), PHP_EOL;
    }
});
unset($zip);
echo 'destroyed', PHP_EOL;
?>
--CLEAN--
<?php
@unlink(__DIR__ . '/gh23747_dtor.zip');
?>
--EXPECT--
Error: Already being closed
destroyed
Error: Already being closed
destroyed

however, not sure about this one

$dir = __DIR__ . '/zdir'; @mkdir($dir);
$zip = new ZipArchive();
$zip->open($dir . '/t.zip', ZipArchive::CREATE | ZipArchive::OVERWRITE);
for ($i = 0; $i < 8; $i++) { $zip->addFromString("f$i.txt", str_repeat('x', 200)); }
set_error_handler(function ($no, $str) use ($zip) {
    static $done = false;
    if ($done) { return true; }
    $done = true;
    try { var_dump($zip->close()); }
    catch (Error $e) { echo $e::class, ': ', $e->getMessage(), PHP_EOL; }
    return true;
});
chmod($dir, 0555);
var_dump($zip->close());

[jvoisin]

Thank you for taking the time to write the tests, I should have been the one doing it in the first place :/

[devnexen]

One last thing I did not notice, target branch should be PHP-8.4. CI should be green so LGTM but I ll let Weilin handles the matter.

[jvoisin]

One last thing I did not notice, target branch should be PHP-8.4. CI should be green so LGTM but I ll let Weilin handles the matter.

Why should it target 8.4?

[LamentXU123]

Because this is a bugfix. And our lowest bugfix support minor version is 8.4 :)

[jvoisin]

The other PR I sent recently to address heap corruptions (#23818, #23814, #23771, #23770, …) were all against the master branch, and were merged there :/

[LamentXU123]

I think they were wrongly merged. I will take care of backporting them :)

[LamentXU123]

LGTM. Nice work!
Last thing, please rebase this to 8.4. I've talked to other people about your previous fixes about whether they should be backport and we'll see. At least it is nice to have this patch in stable branches. Thank you.

Update: They are backported.

[LamentXU123]

Thank you!