From e82d60415f710ffbc1db589e2b8f1330b06ecf0e Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Mon, 5 Aug 2024 17:45:39 +0200 Subject: [PATCH] Do not scan generator frames more than once --- Zend/tests/gh15330-001.phpt | 38 ++++++++++++++++++++++++ Zend/tests/gh15330-002.phpt | 33 +++++++++++++++++++++ Zend/tests/gh15330-003.phpt | 57 ++++++++++++++++++++++++++++++++++++ Zend/tests/gh15330-004.phpt | 52 +++++++++++++++++++++++++++++++++ Zend/tests/gh15330-005.phpt | 53 +++++++++++++++++++++++++++++++++ Zend/tests/gh15330-006.phpt | 56 +++++++++++++++++++++++++++++++++++ Zend/zend_execute.c | 21 +++++++++----- Zend/zend_fibers.c | 22 +++++++++++++- Zend/zend_generators.c | 58 ++++++++++++++++++++----------------- Zend/zend_generators.h | 2 ++ 10 files changed, 357 insertions(+), 35 deletions(-) create mode 100644 Zend/tests/gh15330-001.phpt create mode 100644 Zend/tests/gh15330-002.phpt create mode 100644 Zend/tests/gh15330-003.phpt create mode 100644 Zend/tests/gh15330-004.phpt create mode 100644 Zend/tests/gh15330-005.phpt create mode 100644 Zend/tests/gh15330-006.phpt diff --git a/Zend/tests/gh15330-001.phpt b/Zend/tests/gh15330-001.phpt new file mode 100644 index 000000000000..dafc31d8b8e2 --- /dev/null +++ b/Zend/tests/gh15330-001.phpt @@ -0,0 +1,38 @@ +--TEST-- +GH-15330 001: Do not scan generator frames more than once +--FILE-- +current()); + $iterable->next(); + var_dump("not executed"); +}); + +$ref = $fiber; + +$fiber->start(); + +gc_collect_cycles(); + +?> +==DONE== +--EXPECT-- +string(3) "foo" +==DONE== diff --git a/Zend/tests/gh15330-002.phpt b/Zend/tests/gh15330-002.phpt new file mode 100644 index 000000000000..cb2fdc560c5e --- /dev/null +++ b/Zend/tests/gh15330-002.phpt @@ -0,0 +1,33 @@ +--TEST-- +GH-15330 002: Do not scan generator frames more than once +--FILE-- +current()); + $iterable->next(); + var_dump("not executed"); +}); + +$ref = $fiber; + +$fiber->start(); + +gc_collect_cycles(); + +?> +==DONE== +--EXPECT-- +string(3) "foo" +==DONE== diff --git a/Zend/tests/gh15330-003.phpt b/Zend/tests/gh15330-003.phpt new file mode 100644 index 000000000000..f4208c544550 --- /dev/null +++ b/Zend/tests/gh15330-003.phpt @@ -0,0 +1,57 @@ +--TEST-- +GH-15330 003: Do not scan generator frames more than once +--FILE-- +current()); + $iterable->next(); + var_dump("not executed"); +}); + +$canary->value = $fiber; + +$fiber->start(); + +$iterable->current(); + +$fiber = $iterable = $canary = null; + +gc_collect_cycles(); + +?> +==DONE== +--EXPECTF-- +object(Canary)#%d (1) { + ["value"]=> + object(Fiber)#%d (0) { + } +} +string(3) "foo" +string(18) "Canary::__destruct" +==DONE== diff --git a/Zend/tests/gh15330-004.phpt b/Zend/tests/gh15330-004.phpt new file mode 100644 index 000000000000..9e971da7f98c --- /dev/null +++ b/Zend/tests/gh15330-004.phpt @@ -0,0 +1,52 @@ +--TEST-- +GH-15330 004: Do not scan generator frames more than once +--FILE-- +current()); + $iterable->next(); + var_dump("not executed"); +}); + +$canary->value = $fiber; + +$fiber->start(); + +$iterable->current(); + +$fiber = $iterable = $canary = null; + +gc_collect_cycles(); + +?> +==DONE== +--EXPECTF-- +object(Canary)#%d (1) { + ["value"]=> + object(Fiber)#%d (0) { + } +} +string(3) "foo" +string(18) "Canary::__destruct" +==DONE== diff --git a/Zend/tests/gh15330-005.phpt b/Zend/tests/gh15330-005.phpt new file mode 100644 index 000000000000..774b141baf93 --- /dev/null +++ b/Zend/tests/gh15330-005.phpt @@ -0,0 +1,53 @@ +--TEST-- +GH-15330 005: Do not scan generator frames more than once +--FILE-- +current()); + $f = $iterable->next(...); + $f(); + var_dump("not executed"); +}); + +$canary->value = $fiber; + +$fiber->start(); + +$iterable->current(); + +$fiber = $iterable = $canary = null; + +gc_collect_cycles(); + +?> +==DONE== +--EXPECTF-- +object(Canary)#%d (1) { + ["value"]=> + object(Fiber)#%d (0) { + } +} +string(3) "foo" +string(18) "Canary::__destruct" +==DONE== diff --git a/Zend/tests/gh15330-006.phpt b/Zend/tests/gh15330-006.phpt new file mode 100644 index 000000000000..fb9195663de6 --- /dev/null +++ b/Zend/tests/gh15330-006.phpt @@ -0,0 +1,56 @@ +--TEST-- +GH-15330 006: Do not scan generator frames more than once +--FILE-- +current()); + $iterable->next(); + var_dump("not executed"); +}); + +$canary->value = $fiber; + +$fiber->start(); + +$iterable->current(); + +$fiber = $iterable = $canary = null; + +gc_collect_cycles(); + +?> +==DONE== +--EXPECTF-- +object(Canary)#%d (1) { + ["value"]=> + object(Fiber)#%d (0) { + } +} +string(3) "foo" +string(18) "Canary::__destruct" +==DONE== diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index 716756ad93ba..7b7d618c37d2 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -4437,7 +4437,20 @@ ZEND_API ZEND_ATTRIBUTE_DEPRECATED HashTable *zend_unfinished_execution_gc(zend_ ZEND_API HashTable *zend_unfinished_execution_gc_ex(zend_execute_data *execute_data, zend_execute_data *call, zend_get_gc_buffer *gc_buffer, bool suspended_by_yield) { - if (!EX(func) || !ZEND_USER_CODE(EX(func)->common.type)) { + if (!EX(func)) { + return NULL; + } + + if (EX_CALL_INFO() & ZEND_CALL_RELEASE_THIS) { + zend_get_gc_buffer_add_obj(gc_buffer, Z_OBJ(execute_data->This)); + } + + if (EX_CALL_INFO() & ZEND_CALL_CLOSURE) { + zend_get_gc_buffer_add_obj(gc_buffer, ZEND_CLOSURE_OBJECT(EX(func))); + } + + if (!ZEND_USER_CODE(EX(func)->common.type)) { + ZEND_ASSERT(!(EX_CALL_INFO() & (ZEND_CALL_HAS_SYMBOL_TABLE|ZEND_CALL_FREE_EXTRA_ARGS|ZEND_CALL_HAS_EXTRA_NAMED_PARAMS))); return NULL; } @@ -4458,12 +4471,6 @@ ZEND_API HashTable *zend_unfinished_execution_gc_ex(zend_execute_data *execute_d } } - if (EX_CALL_INFO() & ZEND_CALL_RELEASE_THIS) { - zend_get_gc_buffer_add_obj(gc_buffer, Z_OBJ(execute_data->This)); - } - if (EX_CALL_INFO() & ZEND_CALL_CLOSURE) { - zend_get_gc_buffer_add_obj(gc_buffer, ZEND_CLOSURE_OBJECT(EX(func))); - } if (EX_CALL_INFO() & ZEND_CALL_HAS_EXTRA_NAMED_PARAMS) { zval extra_named_params; ZVAL_ARR(&extra_named_params, EX(extra_named_params)); diff --git a/Zend/zend_fibers.c b/Zend/zend_fibers.c index 81e6e8832a06..62d589233eb5 100644 --- a/Zend/zend_fibers.c +++ b/Zend/zend_fibers.c @@ -19,6 +19,7 @@ #include "zend.h" #include "zend_API.h" +#include "zend_gc.h" #include "zend_ini.h" #include "zend_vm.h" #include "zend_exceptions.h" @@ -27,6 +28,7 @@ #include "zend_mmap.h" #include "zend_compile.h" #include "zend_closures.h" +#include "zend_generators.h" #include "zend_fibers.h" #include "zend_fibers_arginfo.h" @@ -682,7 +684,25 @@ static HashTable *zend_fiber_object_gc(zend_object *object, zval **table, int *n HashTable *lastSymTable = NULL; zend_execute_data *ex = fiber->execute_data; for (; ex; ex = ex->prev_execute_data) { - HashTable *symTable = zend_unfinished_execution_gc_ex(ex, ex->func && ZEND_USER_CODE(ex->func->type) ? ex->call : NULL, buf, false); + HashTable *symTable; + if (ZEND_CALL_INFO(ex) & ZEND_CALL_GENERATOR) { + /* The generator object is stored in ex->return_value */ + zend_generator *generator = (zend_generator*)ex->return_value; + /* There are two cases to consider: + * - If the generator is currently running, the Generator's GC + * handler will ignore it because it is not collectable. However, + * in this context the generator is suspended in Fiber::suspend() + * and may be collectable, so we can inspect it. + * - If the generator is not running, the Generator's GC handler + * will inspect it. In this case we have to skip the frame. + */ + if (!(generator->flags & ZEND_GENERATOR_CURRENTLY_RUNNING)) { + continue; + } + symTable = zend_generator_frame_gc(buf, generator); + } else { + symTable = zend_unfinished_execution_gc_ex(ex, ex->func && ZEND_USER_CODE(ex->func->type) ? ex->call : NULL, buf, false); + } if (symTable) { if (lastSymTable) { zval *val; diff --git a/Zend/zend_generators.c b/Zend/zend_generators.c index 85127025679d..4ac45949bd34 100644 --- a/Zend/zend_generators.c +++ b/Zend/zend_generators.c @@ -398,32 +398,11 @@ static void zend_generator_free_storage(zend_object *object) /* {{{ */ } /* }}} */ -static HashTable *zend_generator_get_gc(zend_object *object, zval **table, int *n) /* {{{ */ +HashTable *zend_generator_frame_gc(zend_get_gc_buffer *gc_buffer, zend_generator *generator) { - zend_generator *generator = (zend_generator*)object; zend_execute_data *execute_data = generator->execute_data; zend_execute_data *call = NULL; - if (!execute_data) { - /* If the generator has been closed, it can only hold on to three values: The value, key - * and retval. These three zvals are stored sequentially starting at &generator->value. */ - *table = &generator->value; - *n = 3; - return NULL; - } - - if (generator->flags & ZEND_GENERATOR_CURRENTLY_RUNNING) { - /* If the generator is currently running, we certainly won't be able to GC any values it - * holds on to. The execute_data state might be inconsistent during execution (e.g. because - * GC has been triggered in the middle of a variable reassignment), so we should not try - * to inspect it here. */ - *table = NULL; - *n = 0; - return NULL; - } - - - zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); zend_get_gc_buffer_add_zval(gc_buffer, &generator->value); zend_get_gc_buffer_add_zval(gc_buffer, &generator->key); zend_get_gc_buffer_add_zval(gc_buffer, &generator->retval); @@ -434,7 +413,7 @@ static HashTable *zend_generator_get_gc(zend_object *object, zval **table, int * call = zend_generator_revert_call_stack(generator->frozen_call_stack); } - zend_unfinished_execution_gc_ex(execute_data, call, gc_buffer, true); + HashTable *ht = zend_unfinished_execution_gc_ex(execute_data, call, gc_buffer, true); if (UNEXPECTED(generator->frozen_call_stack)) { zend_generator_revert_call_stack(call); @@ -444,12 +423,37 @@ static HashTable *zend_generator_get_gc(zend_object *object, zval **table, int * zend_get_gc_buffer_add_obj(gc_buffer, &generator->node.parent->std); } - zend_get_gc_buffer_use(gc_buffer, table, n); - if (EX_CALL_INFO() & ZEND_CALL_HAS_SYMBOL_TABLE) { - return execute_data->symbol_table; - } else { + return ht; +} + +static HashTable *zend_generator_get_gc(zend_object *object, zval **table, int *n) /* {{{ */ +{ + zend_generator *generator = (zend_generator*)object; + zend_execute_data *execute_data = generator->execute_data; + + if (!execute_data) { + /* If the generator has been closed, it can only hold on to three values: The value, key + * and retval. These three zvals are stored sequentially starting at &generator->value. */ + *table = &generator->value; + *n = 3; return NULL; } + + if (generator->flags & ZEND_GENERATOR_CURRENTLY_RUNNING) { + /* If the generator is currently running, we certainly won't be able to GC any values it + * holds on to. The execute_data state might be inconsistent during execution (e.g. because + * GC has been triggered in the middle of a variable reassignment), so we should not try + * to inspect it here. */ + *table = NULL; + *n = 0; + return NULL; + } + + zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); + HashTable *ht = zend_generator_frame_gc(gc_buffer, generator); + zend_get_gc_buffer_use(gc_buffer, table, n); + + return ht; } /* }}} */ diff --git a/Zend/zend_generators.h b/Zend/zend_generators.h index a3daba3a839b..a41fb7699d84 100644 --- a/Zend/zend_generators.h +++ b/Zend/zend_generators.h @@ -127,6 +127,8 @@ static zend_always_inline zend_generator *zend_generator_get_current(zend_genera return zend_generator_update_current(generator); } +HashTable *zend_generator_frame_gc(zend_get_gc_buffer *gc_buffer, zend_generator *generator); + END_EXTERN_C() #endif