fix GH-20469: unsafe inheritance cache replay with reentrant autoloading

[morrisonlevi]

Symptoms and Background

Crashes commonly manifest in instanceof_function_slow. unlinked_instanceof() may see a linked starting class while an ancestor in its parent chain is still unlinked.

This has been a bug since PHP 8.1, I can reproduce it with the dockerhub php:8.1-cli image. I'm targeting PHP 8.4 for the fix, as it's the oldest version in active support.

Description

Inheritance cache dependencies are collected while a class is being linked. During delayed variance resolution, autoloading can re-enter class linking and use the current class while it is only nearly linked. If that class is persisted in the inheritance cache, a later request can replay dependencies in a different order and observe an incomplete hierarchy.

When delayed autoloading causes the class to be used through the unlinked/nearly-linked lookup path, mark it as non-cacheable after load_delayed_classes() returns. This also catches cases where the class's variance obligations were resolved re-entrantly, before the direct resolve_delayed_variance_obligations() call would run.

If dependency tracking already allocated a temporary dependency table, free it when cache insertion is skipped. Restrict this cleanup to classes that entered inheritance-cache construction, because otherwise inheritance_cache is not a dependency table and may contain unrelated or uninitialized data. This preserves inheritance-cache use for delayed-variance classes that did not participate in this re-entrant cycle.

With the invalid cache entry prevented, unlinked_instanceof() can keep using instanceof_function() for linked classes.

[arnaud-lb]

There may be a deeper issue as I think that we are supposed to mark classes as linked only when the fully hierarchy is linked. Especially, we shouldn't cache such class in the inheritance cache.

We can observe that linking is not complete as methods from grand parent of ChildOfParentBeingLinked are not inherited:

diff --git a/ext/opcache/tests/gh20469.phpt b/ext/opcache/tests/gh20469.phpt
index 1cd826c177e..dfab60c79be 100644
--- a/ext/opcache/tests/gh20469.phpt
+++ b/ext/opcache/tests/gh20469.phpt
@@ -33,6 +33,9 @@
 <?php
 require __DIR__ . '/autoload.php';
 echo \APP\ParentBeingLinked::SOME_CONSTANT;
+$i = new \APP\ChildOfParentBeingLinked();
+var_dump($i->test());
 PHP);
 
 file_put_contents($dir . '/classes/RootForTraitReturn.php', <<<'PHP'
@@ -44,6 +47,7 @@ class RootForTraitReturn
     function createResult(): BaseCovariantReturn
     {
     }
+    function test() {}
 }
 PHP);

The call results in

Uncaught Error: Call to undefined method APP\ChildOfParentBeingLinked::test() in ext/opcache/tests/gh20469/test2.php:6

[bwoebi]

So, if I understand this correctly, the proper solution is excluding classes involved in a cycle during initial linking outside of opcache from is_cacheable?

It should be rather an edge case anyway and not have much impact thus to exclude these?

[morrisonlevi]

I've added more tests including the variant Arnaud showed, thanks! I force-pushed and brought it up to current for PHP 8.4, and redid the PR title and description to match.

Do you see any issues with the patch? Tests passed locally for me under ASAN, anyway (I did not run the full suite locally).