Observers reinitialized for closures defined in methods when OPcache is enabled

[TimWolla]

Description

The following test:

--TEST--
XXX
--EXTENSIONS--
zend_test
--INI--
zend_test.observer.enabled=1
zend_test.observer.show_output=1
zend_test.observer.observe_all=1
--FILE--
<?php

class Foo {
  function foo() {
    (function () {
      echo "x", PHP_EOL;
    })();
  }
}

$f = new Foo();
$f->foo();
$f->foo();

?>
--EXPECTF--
<!-- init '%s' -->
<file '%s'>
  <!-- init Foo::foo() -->
  <Foo::foo>
    <!-- init Foo::{closure:Foo::foo():5}() -->
    <Foo::{closure:Foo::foo():5}>
x
    </Foo::{closure:Foo::foo():5}>
  </Foo::foo>
  <Foo::foo>
    <Foo::{closure:Foo::foo():5}>
x
    </Foo::{closure:Foo::foo():5}>
  </Foo::foo>
</file '%s'>

passes for:

sapi/cli/php run-tests.php --asan -d zend_extension=$(pwd)/modules/opcache.so -d opcache.enable_cli=0 -d opcache.protect_memory=1 --show-diff ext/zend_test/tests/observer_closure_04.phpt

but fails when opcache.enable_cli=1:

sapi/cli/php run-tests.php --asan -d zend_extension=$(pwd)/modules/opcache.so -d opcache.enable_cli=1 -d opcache.protect_memory=1 --show-diff ext/zend_test/tests/observer_closure_04.phpt                           

with the following output:

TEST 1/1 [ext/zend_test/tests/observer_closure_04.phpt]
========DIFF========
--
         </Foo::{closure:Foo::foo():5}>
       </Foo::foo>
       <Foo::foo>
011+     <!-- init Foo::{closure:Foo::foo():5}() -->
         <Foo::{closure:Foo::foo():5}>
     x
         </Foo::{closure:Foo::foo():5}>
--
========DONE========

PHP Version

git master

Operating System

No response

[bwoebi]

@TimWolla That's to some degree expected behaviour, because without opcache the scope is changed on first usage of the closure. With opcache the scope is immutable.

And Closures have to allocate a different run_time_cache (which also causes an observer initialization).
There's no guarantee that observer is only initialized once for a given set of opcodes. The guarantee is once per run_time_cache.

For this specific case, we can however do if ((op_array->fn_flags & ZEND_ACC_STATIC) == 0) op_array->scope = CG(active_class_entry); for Closures in compiler - I think.

[TimWolla]

For this specific case, we can however do if ((op_array->fn_flags & ZEND_ACC_STATIC) == 0) op_array->scope = CG(active_class_entry); for Closures in compiler - I think.

Why limit it to non-static closures? static closures also have a scope and are affected by this issue. They just don't have $this, but can access private members of the surrounding class.

And indeed:

--- i/Zend/zend_compile.c
+++ w/Zend/zend_compile.c
@@ -8306,6 +8306,7 @@ static zend_op_array *zend_compile_func_decl_ex(
 
        if (decl->kind == ZEND_AST_CLOSURE || decl->kind == ZEND_AST_ARROW_FUNC) {
                op_array->fn_flags |= ZEND_ACC_CLOSURE;
+               op_array->scope = CG(active_class_entry);
        }
 
        if (is_hook) {

seems to do the trick and seems to be a valid optimization for the assumption that most closures don't change their scope.