Skip to content

Commit 3c288b1

Browse files
bwoebibwoebi
committed
Fix bad run_time_cache with Closure::call()
This also fixes a memory "leak" (memory is allocated on unbounded arena without limits) on each new Closure instantiation. Closures with same scope now all share the same run_time_cache (as long as it is arena allocated)
1 parent f3df3df commit 3c288b1

File tree

3 files changed

+55
-2
lines changed

3 files changed

+55
-2
lines changed

‎Zend/tests/closure_060.phpt

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
--TEST--
2+
runtime cache must be invalidated for Closure::call()
3+
--FILE--
4+
<?php
5+
6+
class A {
7+
private static $priv = 7;
8+
9+
static function get() {
10+
return function() {
11+
var_dump(isset(A::$priv));
12+
};
13+
}
14+
}
15+
16+
$closure = A::get();
17+
$closure(); // init rt_cache
18+
$closure->call(new class(){}, null);
19+
$closure();
20+
21+
?>
22+
--EXPECT--
23+
bool(true)
24+
bool(false)
25+
bool(true)

‎Zend/zend_closures.c

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,19 @@ ZEND_METHOD(Closure, call)
127127
my_function.common.scope = Z_OBJCE_P(newthis);
128128
fci_cache.function_handler = &my_function;
129129

130+
/* Runtime cache relies on bound scope to be immutable, hence we need a separate rt cache in case scope changed */
131+
if (ZEND_USER_CODE(my_function.type) && closure->func.common.scope != Z_OBJCE_P(newthis)) {
132+
my_function.op_array.run_time_cache = emalloc(my_function.op_array.cache_size);
133+
memset(my_function.op_array.run_time_cache, 0, my_function.op_array.cache_size);
134+
}
135+
130136
if (zend_call_function(&fci, &fci_cache) == SUCCESS && Z_TYPE(closure_result) != IS_UNDEF) {
131137
ZVAL_COPY_VALUE(return_value, &closure_result);
132138
}
139+
140+
if (ZEND_USER_CODE(my_function.type) && closure->func.common.scope != Z_OBJCE_P(newthis)) {
141+
efree(my_function.op_array.run_time_cache);
142+
}
133143
}
134144
/* }}} */
135145

@@ -138,7 +148,7 @@ ZEND_METHOD(Closure, call)
138148
ZEND_METHOD(Closure, bind)
139149
{
140150
zval *newthis, *zclosure, *scope_arg = NULL;
141-
zend_closure *closure;
151+
zend_closure *closure, *new_closure;
142152
zend_class_entry *ce, *called_scope;
143153

144154
if (zend_parse_method_parameters(ZEND_NUM_ARGS(), getThis(), "Oo!|z", &zclosure, zend_ce_closure, &newthis, &scope_arg) == FAILURE) {
@@ -183,6 +193,15 @@ ZEND_METHOD(Closure, bind)
183193
}
184194

185195
zend_create_closure(return_value, &closure->func, ce, called_scope, newthis);
196+
new_closure = (zend_closure *) Z_OBJ_P(return_value);
197+
198+
/* Runtime cache relies on bound scope to be immutable, hence we need a separate rt cache in case scope changed */
199+
if (ZEND_USER_CODE(closure->func.type) && (closure->func.common.scope != new_closure->func.common.scope || (closure->func.op_array.fn_flags & ZEND_ACC_NO_RT_ARENA))) {
200+
new_closure->func.op_array.run_time_cache = emalloc(new_closure->func.op_array.cache_size);
201+
memset(new_closure->func.op_array.run_time_cache, 0, new_closure->func.op_array.cache_size);
202+
203+
new_closure->func.op_array.fn_flags |= ZEND_ACC_NO_RT_ARENA;
204+
}
186205
}
187206
/* }}} */
188207

@@ -297,6 +316,10 @@ static void zend_closure_free_storage(zend_object *object) /* {{{ */
297316
zend_object_std_dtor(&closure->std);
298317

299318
if (closure->func.type == ZEND_USER_FUNCTION) {
319+
if (closure->func.op_array.fn_flags & ZEND_ACC_NO_RT_ARENA) {
320+
efree(closure->func.op_array.run_time_cache);
321+
closure->func.op_array.run_time_cache = NULL;
322+
}
300323
destroy_op_array(&closure->func.op_array);
301324
}
302325

@@ -510,7 +533,10 @@ ZEND_API void zend_create_closure(zval *res, zend_function *func, zend_class_ent
510533
zend_hash_init(closure->func.op_array.static_variables, zend_hash_num_elements(static_variables), NULL, ZVAL_PTR_DTOR, 0);
511534
zend_hash_apply_with_arguments(static_variables, zval_copy_static_var, 1, closure->func.op_array.static_variables);
512535
}
513-
closure->func.op_array.run_time_cache = NULL;
536+
if (UNEXPECTED(!closure->func.op_array.run_time_cache)) {
537+
closure->func.op_array.run_time_cache = func->op_array.run_time_cache = zend_arena_alloc(&CG(arena), func->op_array.cache_size);
538+
memset(func->op_array.run_time_cache, 0, func->op_array.cache_size);
539+
}
514540
if (closure->func.op_array.refcount) {
515541
(*closure->func.op_array.refcount)++;
516542
}

‎Zend/zend_compile.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,8 @@ typedef struct _zend_try_catch_element {
240240
#define ZEND_ACC_CLOSURE 0x100000
241241
#define ZEND_ACC_GENERATOR 0x800000
242242

243+
#define ZEND_ACC_NO_RT_ARENA 0x10000
244+
243245
/* call through user function trampoline. e.g. __call, __callstatic */
244246
#define ZEND_ACC_CALL_VIA_TRAMPOLINE 0x200000
245247

0 commit comments

Comments
 (0)