Fix GH-23276: Collect ZipArchive subclasses holding their own streams - #23282
Fix GH-23276: Collect ZipArchive subclasses holding their own streams#23282LamentXU123 wants to merge 9 commits into
Conversation
It is probably best for a stable branch IMO, that being said I ll review it sometime this weekend. |
|
I would suggest one thing after a quick look. Keep at it, this refcounting approach is likely to cause indirect issues. |
|
I think this is wrong, I believe you need a get_gc object handler for ZipArchive. |
|
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. |
…y had the reference consumed if it is being collected.
| * releasing it again would underflow the refcount. A non-zero | ||
| * refcount means that this stream is being closed normally. | ||
| */ | ||
| if (GC_REFCOUNT(&self->owner->zo) != 0) { |
There was a problem hiding this comment.
I ll see the rest later but what happens if a php globals still holds the resource then we fclose on it for ex ?
There was a problem hiding this comment.
I think its fine at the first glance. Let's add a regression test just in case.
There was a problem hiding this comment.
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);There was a problem hiding this comment.
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.
|
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. |
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
ZipArchiveobject with a refcounted archive state, so a stream can keep the underlyingzip_twithout preventing its owningZipArchiveobject 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