Skip to content

Commit 45d1acf

Browse files
TimWollailuuu1994
TimWolla
and
iluuu1994
authored
Zend: Fix reference counting for Closures in const-expr (php#17853)
* Clean up closure static variable handling * Zend: Fix reference counting for Closures in const-expr Fixes php#17851 --------- Co-authored-by: Ilija Tovilo <ilija.tovilo@me.com>
1 parent 0006522 commit 45d1acf

File tree

9 files changed

+169
-24
lines changed

9 files changed

+169
-24
lines changed

‎Zend/tests/closures/closure_const_expr/attributes.phpt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,13 @@ reflection
88
#[Attribute(Attribute::TARGET_CLASS | Attribute::IS_REPEATABLE)]
99
class Attr {
1010
public function __construct(public Closure $value) {
11-
$value('foo');
11+
$value('foo');
1212
}
1313
}
1414

1515
#[Attr(static function () { })]
1616
#[Attr(static function (...$args) {
17-
var_dump($args);
17+
var_dump($args);
1818
})]
1919
class C {}
2020

‎Zend/tests/closures/closure_const_expr/attributes_autoload.inc

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<?php
2+
3+
#[Attr(static function () { })]
4+
#[Attr(static function (...$args) {
5+
var_dump($args);
6+
})]
7+
class C {}
8+
9+
?>

‎Zend/tests/closures/closure_const_expr/attributes_autoload.phpt

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
--TEST--
2+
GH-17851: Use-after-free when instantiating autoloaded class with attribute having a Closure parameter.
3+
--EXTENSIONS--
4+
reflection
5+
--FILE--
6+
<?php
7+
8+
spl_autoload_register(static function ($className) {
9+
if ($className === 'C') {
10+
require(__DIR__ . '/attributes_autoload.inc');
11+
}
12+
});
13+
14+
#[Attribute(Attribute::TARGET_CLASS | Attribute::IS_REPEATABLE)]
15+
class Attr {
16+
public function __construct(public Closure $value) {
17+
$value('foo');
18+
}
19+
}
20+
21+
foreach ((new ReflectionClass(C::class))->getAttributes() as $reflectionAttribute) {
22+
var_dump($reflectionAttribute->newInstance());
23+
}
24+
25+
?>
26+
--EXPECTF--
27+
object(Attr)#%d (1) {
28+
["value"]=>
29+
object(Closure)#%d (3) {
30+
["name"]=>
31+
string(%d) "{closure:%s:%d}"
32+
["file"]=>
33+
string(%d) "%s"
34+
["line"]=>
35+
int(%d)
36+
}
37+
}
38+
array(1) {
39+
[0]=>
40+
string(3) "foo"
41+
}
42+
object(Attr)#%d (1) {
43+
["value"]=>
44+
object(Closure)#%d (4) {
45+
["name"]=>
46+
string(%d) "{closure:%s:%d}"
47+
["file"]=>
48+
string(%d) "%s"
49+
["line"]=>
50+
int(%d)
51+
["parameter"]=>
52+
array(1) {
53+
["$args"]=>
54+
string(10) "<optional>"
55+
}
56+
}
57+
}

‎Zend/tests/closures/closure_const_expr/static_variable.phpt

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
--TEST--
2+
Closures in const expressions support static variables.
3+
--FILE--
4+
<?php
5+
6+
const Closure = static function () {
7+
static $x = [];
8+
static $i = 1;
9+
$i *= 2;
10+
$x[] = $i;
11+
var_dump($x);
12+
};
13+
14+
var_dump(Closure);
15+
(Closure)();
16+
(Closure)();
17+
(Closure)();
18+
var_dump(Closure);
19+
20+
?>
21+
--EXPECTF--
22+
object(Closure)#%d (4) {
23+
["name"]=>
24+
string(%d) "{closure:%s:%d}"
25+
["file"]=>
26+
string(%d) "%s"
27+
["line"]=>
28+
int(3)
29+
["static"]=>
30+
array(2) {
31+
["x"]=>
32+
array(0) {
33+
}
34+
["i"]=>
35+
int(1)
36+
}
37+
}
38+
array(1) {
39+
[0]=>
40+
int(2)
41+
}
42+
array(2) {
43+
[0]=>
44+
int(2)
45+
[1]=>
46+
int(4)
47+
}
48+
array(3) {
49+
[0]=>
50+
int(2)
51+
[1]=>
52+
int(4)
53+
[2]=>
54+
int(8)
55+
}
56+
object(Closure)#%d (4) {
57+
["name"]=>
58+
string(%d) "{closure:%s:%d}"
59+
["file"]=>
60+
string(%d) "%s"
61+
["line"]=>
62+
int(3)
63+
["static"]=>
64+
array(2) {
65+
["x"]=>
66+
array(3) {
67+
[0]=>
68+
int(2)
69+
[1]=>
70+
int(4)
71+
[2]=>
72+
int(8)
73+
}
74+
["i"]=>
75+
int(8)
76+
}
77+
}

‎Zend/zend_ast.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1277,6 +1277,7 @@ static void* ZEND_FASTCALL zend_ast_tree_copy(zend_ast *ast, void *buf)
12771277
new->attr = old->attr;
12781278
new->lineno = old->lineno;
12791279
new->op_array = old->op_array;
1280+
function_add_ref((zend_function *)new->op_array);
12801281
buf = (void*)((char*)buf + sizeof(zend_ast_op_array));
12811282
} else if (ast->kind == ZEND_AST_CALLABLE_CONVERT) {
12821283
zend_ast_fcc *old = (zend_ast_fcc*)ast;
@@ -1353,7 +1354,7 @@ ZEND_API void ZEND_FASTCALL zend_ast_destroy(zend_ast *ast)
13531354
} else if (EXPECTED(ast->kind == ZEND_AST_CONSTANT)) {
13541355
zend_string_release_ex(zend_ast_get_constant_name(ast), 0);
13551356
} else if (EXPECTED(ast->kind == ZEND_AST_OP_ARRAY)) {
1356-
/* Nothing to do. */
1357+
destroy_op_array(zend_ast_get_op_array(ast)->op_array);
13571358
} else if (EXPECTED(zend_ast_is_decl(ast))) {
13581359
zend_ast_decl *decl = (zend_ast_decl *) ast;
13591360

‎Zend/zend_closures.c

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -528,7 +528,6 @@ static void zend_closure_free_storage(zend_object *object) /* {{{ */
528528
/* We don't own the static variables of fake closures. */
529529
if (!(closure->func.op_array.fn_flags & ZEND_ACC_FAKE_CLOSURE)) {
530530
zend_destroy_static_vars(&closure->func.op_array);
531-
closure->func.op_array.static_variables = NULL;
532531
}
533532
destroy_op_array(&closure->func.op_array);
534533
} else if (closure->func.type == ZEND_INTERNAL_FUNCTION) {
@@ -760,16 +759,14 @@ static void zend_create_closure_ex(zval *res, zend_function *func, zend_class_en
760759
}
761760

762761
/* For fake closures, we want to reuse the static variables of the original function. */
762+
HashTable *ht = ZEND_MAP_PTR_GET(func->op_array.static_variables_ptr);
763763
if (!is_fake) {
764-
if (closure->func.op_array.static_variables) {
765-
closure->func.op_array.static_variables =
766-
zend_array_dup(closure->func.op_array.static_variables);
764+
if (!ht) {
765+
ht = closure->func.op_array.static_variables;
767766
}
768767
ZEND_MAP_PTR_INIT(closure->func.op_array.static_variables_ptr,
769-
closure->func.op_array.static_variables);
768+
ht ? zend_array_dup(ht) : NULL);
770769
} else if (func->op_array.static_variables) {
771-
HashTable *ht = ZEND_MAP_PTR_GET(func->op_array.static_variables_ptr);
772-
773770
if (!ht) {
774771
ht = zend_array_dup(func->op_array.static_variables);
775772
ZEND_MAP_PTR_SET(func->op_array.static_variables_ptr, ht);

‎Zend/zend_compile.c

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8244,9 +8244,6 @@ static zend_string *zend_begin_func_decl(znode *result, zend_op_array *op_array,
82448244

82458245
zend_register_seen_symbol(lcname, ZEND_SYMBOL_FUNCTION);
82468246
switch (level) {
8247-
case FUNC_DECL_LEVEL_CONSTEXPR:
8248-
zend_add_dynamic_func_def(op_array);
8249-
break;
82508247
case FUNC_DECL_LEVEL_NESTED: {
82518248
uint32_t func_ref = zend_add_dynamic_func_def(op_array);
82528249
if (op_array->fn_flags & ZEND_ACC_CLOSURE) {
@@ -8261,6 +8258,7 @@ static zend_string *zend_begin_func_decl(znode *result, zend_op_array *op_array,
82618258
}
82628259
break;
82638260
}
8261+
case FUNC_DECL_LEVEL_CONSTEXPR:
82648262
case FUNC_DECL_LEVEL_TOPLEVEL:
82658263
/* Nothing to do. */
82668264
break;

‎Zend/zend_opcode.c

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -636,13 +636,6 @@ ZEND_API void destroy_op_array(zend_op_array *op_array)
636636
}
637637
if (op_array->num_dynamic_func_defs) {
638638
for (i = 0; i < op_array->num_dynamic_func_defs; i++) {
639-
/* Closures overwrite static_variables in their copy.
640-
* Make sure to destroy them when the prototype function is destroyed. */
641-
if (op_array->dynamic_func_defs[i]->static_variables
642-
&& (op_array->dynamic_func_defs[i]->fn_flags & ZEND_ACC_CLOSURE)) {
643-
zend_array_destroy(op_array->dynamic_func_defs[i]->static_variables);
644-
op_array->dynamic_func_defs[i]->static_variables = NULL;
645-
}
646639
destroy_op_array(op_array->dynamic_func_defs[i]);
647640
}
648641
efree(op_array->dynamic_func_defs);

‎ext/opcache/zend_file_cache.c

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,15 @@ static void zend_file_cache_unserialize_zval(zval *zv,
242242
zend_persistent_script *script,
243243
void *buf);
244244

245+
static void zend_file_cache_serialize_func(zval *zv,
246+
zend_persistent_script *script,
247+
zend_file_cache_metainfo *info,
248+
void *buf);
249+
250+
static void zend_file_cache_unserialize_func(zval *zv,
251+
zend_persistent_script *script,
252+
void *buf);
253+
245254
static void *zend_file_cache_serialize_interned(zend_string *str,
246255
zend_file_cache_metainfo *info)
247256
{
@@ -364,8 +373,10 @@ static void zend_file_cache_serialize_ast(zend_ast *ast,
364373
}
365374
}
366375
} else if (ast->kind == ZEND_AST_OP_ARRAY) {
367-
/* The op_array itself will be serialized as part of the dynamic_func_defs. */
368-
SERIALIZE_PTR(zend_ast_get_op_array(ast)->op_array);
376+
zval z;
377+
ZVAL_PTR(&z, zend_ast_get_op_array(ast)->op_array);
378+
zend_file_cache_serialize_func(&z, script, info, buf);
379+
zend_ast_get_op_array(ast)->op_array = Z_PTR(z);
369380
} else if (ast->kind == ZEND_AST_CALLABLE_CONVERT) {
370381
zend_ast_fcc *fcc = (zend_ast_fcc*)ast;
371382
ZEND_MAP_PTR_INIT(fcc->fptr, NULL);
@@ -1252,8 +1263,10 @@ static void zend_file_cache_unserialize_ast(zend_ast *ast,
12521263
}
12531264
}
12541265
} else if (ast->kind == ZEND_AST_OP_ARRAY) {
1255-
/* The op_array itself will be unserialized as part of the dynamic_func_defs. */
1256-
UNSERIALIZE_PTR(zend_ast_get_op_array(ast)->op_array);
1266+
zval z;
1267+
ZVAL_PTR(&z, zend_ast_get_op_array(ast)->op_array);
1268+
zend_file_cache_unserialize_func(&z, script, buf);
1269+
zend_ast_get_op_array(ast)->op_array = Z_PTR(z);
12571270
} else if (ast->kind == ZEND_AST_CALLABLE_CONVERT) {
12581271
zend_ast_fcc *fcc = (zend_ast_fcc*)ast;
12591272
ZEND_MAP_PTR_NEW(fcc->fptr);

0 commit comments

Comments
 (0)