Fix GH-23276: Collect ZipArchive subclasses holding their own streams

[LamentXU123]

I actually doubt this is a bug in the Zend engine. But since I am not familiar with how Zend GC works, I'd fix this is in a completely ext/zip way, to replace the stream's reference to the ZipArchive object with a refcounted archive state, so a stream can keep the underlying zip_t without preventing its owning ZipArchive object from being GC-ed.

There can be better fixes, as I said, to fix in the Zend engine. But I think this is a very rare case happening in the zip extension, so I'd rather re-implement it than touching the Zend core, so we don't bother other code.

Fixes GH-23276

[devnexen]

There can be better fixes, as I said, to fix in the Zend engine. But I think this is a very rare case happening in the zip extension, so I'd rather re-implement it than touching Zend core, so we don't bother other code.

It is probably best for a stable branch IMO, that being said I ll review it sometime this weekend.

[devnexen]

I would suggest one thing after a quick look. Keep at it, this refcounting approach is likely to cause indirect issues.

[ndossche]

I think this is wrong, I believe you need a get_gc object handler for ZipArchive.

[LamentXU123]

Apparently I have to take some time to study Zend GC. It seems like we don't have a reasonable way to bypass this entirely.

[devnexen]

I ll see the rest later but what happens if a php globals still holds the resource then we fclose on it for ex ?

[LamentXU123]

I think its fine at the first glance. Let's add a regression test just in case.

[devnexen]

came up with this

<?php
class Reader extends ZipArchive {
    public $stream;
    public function __destruct() { $GLOBALS['zombie'] = $this; }
}

$f = __DIR__ . '/n5s.zip';
$z = new ZipArchive;
$z->open($f, ZipArchive::CREATE | ZipArchive::OVERWRITE);
$z->addFromString('entry.txt', 'contents');
$z->close();

$zip = new Reader;
$zip->open($f, ZipArchive::RDONLY);
$zip->stream = $zip->getStream('entry.txt');
unset($zip);
gc_collect_cycles();

var_dump($GLOBALS['zombie'] instanceof ZipArchive);
var_dump($GLOBALS['zombie']->stream);
echo 'done', PHP_EOL;
@unlink($f);

[LamentXU123]

I ll see the rest later but what happens if a php globals still holds the resource then we fclose on it for ex ?

You are correct. ZipArchive's owner pin doesn't delete the object. Thanks.

[devnexen]

something tells me it s probably best to keep it as draft for now, I did not review the whole code yet but already unsure everything is fully correct.

[devnexen]

that seems overall costly performance wise. Prior, get_gc did absolutely nothing, that s a big jump not in a nice way.

[devnexen]

I would like Nora opinion on the get_gc topic, not feeling confident about it. It feels overall fragile but also borderline master-ish level of changes for a stable branch.

[ndossche]

I tried experimenting with this, but after all, I think that the original patch (the one introducing this issue) was not entirely right. What's especially painful is that it pins a resource to an object, something not well-supported by the engine (e.g. gc_mark_grey is not supported for IS_RESOURCE). Especially fun is how resources and objects interact with each other during shutdown.
Thinking about the design: we don't need the object to be alive, we just need the struct zip to be alive. If we can decouple those, then the fix becomes easier and we don't need special get_gc handling.

However, this causes a problem: we need to refactor how the objects are layed out I'm afraid... So my proposal, whilst IMO cleaner and more correct, is quite heavy of a change:
I propose the following diff on top of this PR (it will revert part of your work and rework part of the original patch):

https://gist.github.com/ndossche/8da9b0d6a323a4ad411844b62e37f33b

If you don't wish this heavy patch to be applied to 8.4, so be it, but then the original fix should also be reverted as I think that breaking the engine assumptions are more problematic.

[LamentXU123]

What's especially painful is that it pins a resource to an object, something not well-supported by the engine

Yes that's exactly my point. Ideally, there should be a fix in the Zend engine, however I don't think my knowledge to the engine can support me to write the patch. Also, I don't want to bother other behaviors in stable branches.

However, this causes a problem: we need to refactor how the objects are layed out I'm afraid... So my proposal, whilst IMO cleaner and more correct, is quite heavy of a change:
I propose the following diff on top of this PR (it will revert part of your work and rework part of the original patch):

https://gist.github.com/ndossche/8da9b0d6a323a4ad411844b62e37f33b

If you don't wish this heavy patch to be applied to 8.4, so be it, but then the original fix should also be reverted as I think that breaking the engine assumptions are more problematic.

I don't think the original patch is correct (I didn't realize this issue when reviewing) so I have no objections on reverting the original patch, it should be done.

The interesting thing is, my first version of patch is similar to yours, and then I change it because I thought it might be too heavy and I am not fully confident on it, so I finally chose to use get_gc weirdly here. So this basically makes the two of us. This patch is to a degree a good refactoring, and I fully agree to it.

[LamentXU123]

Branch-wise, IMO this should be done in 8.4 no matter how we fix this. Because the problematic patch is merged in lower branches, so the fix should be merged in lower branches.

[devnexen]

zip callbacks are a special case, could you try locally (with a debug build I do not think you need sanitizers)

$zip = new ZipArchive;
$zip->open($filename);
$zip->registerProgressCallback(0.5, function ($rate) {}); // same with cancel one
$zip->addFromString('late.txt', 'late');
$stream = $zip->getStream('entry.txt');
unset($zip);

[LamentXU123]

Yes ze_zip_object is freed in ur case but the stream preserve the zip_t. So when zip_close we got a UAF.
I don't think stream should have the ZipArchive object tho, lets use shared php_zip_archive instead.