Skip to content

Fix handling of destructors during GC #4489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

nikic
Copy link
Member

@nikic nikic commented Aug 1, 2019

Fix for https://bugs.php.net/bug.php?id=72530.

The problem is that our current approach of comparing refcounts is not sufficient, because an external reference may be added in a way that does not change refcounts.

We can't detect this kind of situation without actually rerunning the full GC marking phase. As such, what I'm doing here is only calling destructors in the current run and leaving the actual freeing of the object and any nested data to the next run.

@nikic nikic requested a review from dstogov August 1, 2019 11:02
@dstogov
Copy link
Member

dstogov commented Aug 6, 2019

The patch misses something. The following script leaks.

<?php
class ryat {
    var $ryat;
    var $chtg;
    var $nested;

    function __destruct() {
        $this->chtg = $this->ryat;
        $this->ryat = 1;
    }
}

$o = new ryat;
$o->nested = [];
$o->nested[] =& $o->nested;
$o->ryat = $o;
$x =& $o->chtg;

unset($o);
gc_collect_cycles();
var_dump($x);
?>

@nikic nikic changed the base branch from master to PHP-7.4 August 8, 2019 14:49
@nikic
Copy link
Member Author

nikic commented Aug 8, 2019

The new approach already removes nested data during the collect_white phase (using remove_white) instead of later. This avoids issues with insufficient nested data being removed because the removal stops at references.

@nikic
Copy link
Member Author

nikic commented Aug 8, 2019

Unfortunately, I think that this approach is not correct. There is a race-condition here between collecting garbage (normal) and removing roots (for dtors). A part of the graph could already be visited and colored black before we see the object with dtor and try to remove roots.

I don't think it's possible to do the nested data removal earlier than we do right now, so I don't see a better solution than what I originally had here, plus #4497.

Zend/zend_gc.c Outdated
gc_root_buffer *root = gc_decompress(ref, GC_REF_ADDRESS(ref));
root->ref = GC_MAKE_DTOR_GARBAGE(root->ref);
GC_REF_SET_PURPLE(ref);
*flags |= GC_HAS_DESTRUCTORS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gc_remove_white() is called only when GC_HAS_DESTRUCTORS already set.

@nikic
Copy link
Member Author

nikic commented Aug 13, 2019

Merged as 60a7e60.

@nikic nikic closed this Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants