From 955da0af319133ccc9e5c31a0a0cfc10ba8e064c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Wed, 14 May 2025 09:52:17 +0200 Subject: [PATCH 1/3] zend_compile: Set `op_array->scope` for Closures as compile time see php/php-src#18546 --- Zend/zend_compile.c | 6 + ext/zend_test/tests/observer_closure_04.phpt | 126 +++++++++++++++++++ 2 files changed, 132 insertions(+) create mode 100644 ext/zend_test/tests/observer_closure_04.phpt diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 0669d106f15e9..85c893d30c78b 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -8306,6 +8306,12 @@ 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; + /* Set the closure scope at compile time as an optimization to + * prevent creating a separate runtime cache for every initialization + * of this closure. Most closures are expected not to change their + * scope in practice. + */ + op_array->scope = CG(active_class_entry); } if (is_hook) { diff --git a/ext/zend_test/tests/observer_closure_04.phpt b/ext/zend_test/tests/observer_closure_04.phpt new file mode 100644 index 0000000000000..2178bc578d4d2 --- /dev/null +++ b/ext/zend_test/tests/observer_closure_04.phpt @@ -0,0 +1,126 @@ +--TEST-- +Observer: Closures keep their runtime cache when not rebinding +--EXTENSIONS-- +zend_test +--INI-- +zend_test.observer.enabled=1 +zend_test.observer.show_output=1 +zend_test.observer.observe_all=1 +--FILE-- +getMessage(), PHP_EOL; + } + })->bindTo(null, null))(); + } +} + +$obj = new Foo(); +$obj->static(); +$obj->static(); +$obj->non_static(); +$obj->non_static(); +$obj->rebind(); +$obj->rebind(); + +$closure = function () { + echo Foo::$value, PHP_EOL; +}; + +($closure->bindTo(null, Foo::class))(); +($closure->bindTo(null, Foo::class))(); + +$boundClosure = $closure->bindTo(null, Foo::class); +$boundClosure(); +$boundClosure(); + +?> +--EXPECTF-- + + + + + + +x + + + + +x + + + + + + +x + + + + +x + + + + + + + + + <{closure:Foo::rebind():16}> +Error: + + +Cannot access private property Foo::$value + + + + + + + <{closure:Foo::rebind():16}> +Error: + +Cannot access private property Foo::$value + + + + + + +x + + + + + +x + + + + + +x + + +x + + From 0ec7095b6e72d8afeb9effc7a3e6e6746ed475e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Wed, 14 May 2025 10:21:29 +0200 Subject: [PATCH 2/3] Prevent the scope from affecting `__METHOD__` --- Zend/zend_compile.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 85c893d30c78b..7afa220ee08e4 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -8306,12 +8306,6 @@ 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; - /* Set the closure scope at compile time as an optimization to - * prevent creating a separate runtime cache for every initialization - * of this closure. Most closures are expected not to change their - * scope in practice. - */ - op_array->scope = CG(active_class_entry); } if (is_hook) { @@ -8468,6 +8462,15 @@ static zend_op_array *zend_compile_func_decl_ex( /* Pop the loop variable stack separator */ zend_stack_del_top(&CG(loop_var_stack)); + if (op_array->fn_flags & ZEND_ACC_CLOSURE) { + /* Set the closure scope at compile time as an optimization to + * prevent creating a separate runtime cache for every initialization + * of this closure. Most closures are expected not to change their + * scope in practice. + */ + op_array->scope = CG(active_class_entry); + } + if (level == FUNC_DECL_LEVEL_TOPLEVEL) { zend_observer_function_declared_notify(op_array, lcname); } From ef6d9cf03c799910b013d7367765740cef77db3a Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Wed, 14 May 2025 16:07:31 +0200 Subject: [PATCH 3/3] Store the Closure run_time_cache scope in the cache itself --- Zend/tests/closures/closure_069.phpt | 14 +++++++++ Zend/zend_closures.c | 44 +++++++++++++++++++++------- Zend/zend_compile.c | 9 ------ 3 files changed, 47 insertions(+), 20 deletions(-) create mode 100644 Zend/tests/closures/closure_069.phpt diff --git a/Zend/tests/closures/closure_069.phpt b/Zend/tests/closures/closure_069.phpt new file mode 100644 index 0000000000000..7ef0f9fdd7b0d --- /dev/null +++ b/Zend/tests/closures/closure_069.phpt @@ -0,0 +1,14 @@ +--TEST-- +Closure: Binding + RT cache edge cases +--FILE-- +bindTo(new class {}); + +?> +==DONE== +--EXPECT-- +==DONE== diff --git a/Zend/zend_closures.c b/Zend/zend_closures.c index 5777e1a34a2b8..c0c0b02131b60 100644 --- a/Zend/zend_closures.c +++ b/Zend/zend_closures.c @@ -733,10 +733,29 @@ static ZEND_NAMED_FUNCTION(zend_closure_internal_handler) /* {{{ */ } /* }}} */ +static zend_class_entry *zend_closure_rt_cache_scope(zend_function *func, void **run_time_cache, bool is_fake, zend_class_entry *default_scope) +{ + ZEND_ASSERT(!(func->op_array.fn_flags & ZEND_ACC_HEAP_RT_CACHE)); + + if (is_fake) { + return func->op_array.scope; + } + + /* Zero RT cache is valid for any scope */ + if (func->op_array.cache_size == 0) { + return default_scope; + } + + ZEND_ASSERT(func->common.fn_flags & ZEND_ACC_CLOSURE); + ZEND_ASSERT(!(func->common.fn_flags & ZEND_ACC_FAKE_CLOSURE)); + + return CACHED_PTR_EX(run_time_cache - 1); +} + static void zend_create_closure_ex(zval *res, zend_function *func, zend_class_entry *scope, zend_class_entry *called_scope, zval *this_ptr, bool is_fake) /* {{{ */ { zend_closure *closure; - void *ptr; + void **ptr; object_init_ex(res, zend_ce_closure); @@ -777,19 +796,22 @@ static void zend_create_closure_ex(zval *res, zend_function *func, zend_class_en /* Runtime cache is scope-dependent, so we cannot reuse it if the scope changed */ ptr = ZEND_MAP_PTR_GET(func->op_array.run_time_cache); if (!ptr - || func->common.scope != scope || (func->common.fn_flags & ZEND_ACC_HEAP_RT_CACHE) + || zend_closure_rt_cache_scope(func, ptr, is_fake, scope) != scope ) { - if (!ptr - && (func->common.fn_flags & ZEND_ACC_CLOSURE) - && (func->common.scope == scope || - !(func->common.fn_flags & ZEND_ACC_IMMUTABLE))) { + if (func->op_array.cache_size == 0) { + ptr = zend_arena_alloc(&CG(arena), 0); + ZEND_MAP_PTR_SET(func->op_array.run_time_cache, ptr); + closure->func.op_array.fn_flags &= ~ZEND_ACC_HEAP_RT_CACHE; + } else if (!ptr + && func->common.fn_flags & ZEND_ACC_CLOSURE) { + ZEND_ASSERT(!(func->common.fn_flags & ZEND_ACC_FAKE_CLOSURE)); /* If a real closure is used for the first time, we create a shared runtime cache - * and remember which scope it is for. */ - if (func->common.scope != scope) { - func->common.scope = scope; - } - ptr = zend_arena_alloc(&CG(arena), func->op_array.cache_size); + * and remember which scope it is for. The scope is stored in + * an extra slot before the run_time_cache. */ + size_t extra_slot_size = sizeof(void*); + ptr = zend_arena_alloc(&CG(arena), func->op_array.cache_size + extra_slot_size) + extra_slot_size; + CACHE_PTR_EX(ptr - 1, scope); ZEND_MAP_PTR_SET(func->op_array.run_time_cache, ptr); closure->func.op_array.fn_flags &= ~ZEND_ACC_HEAP_RT_CACHE; } else { diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 7afa220ee08e4..0669d106f15e9 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -8462,15 +8462,6 @@ static zend_op_array *zend_compile_func_decl_ex( /* Pop the loop variable stack separator */ zend_stack_del_top(&CG(loop_var_stack)); - if (op_array->fn_flags & ZEND_ACC_CLOSURE) { - /* Set the closure scope at compile time as an optimization to - * prevent creating a separate runtime cache for every initialization - * of this closure. Most closures are expected not to change their - * scope in practice. - */ - op_array->scope = CG(active_class_entry); - } - if (level == FUNC_DECL_LEVEL_TOPLEVEL) { zend_observer_function_declared_notify(op_array, lcname); }