From 5ebe084eb53acc8d0d5b3b4a3cfde6ec6b32499a Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Fri, 14 Oct 2022 19:29:01 +0200 Subject: [PATCH] Fix generator memory leaks when interrupted during argument evaluation --- Zend/tests/generators/gh9750-001.phpt | 33 +++++++ Zend/tests/generators/gh9750-002.phpt | 33 +++++++ Zend/tests/generators/gh9750-003.phpt | 33 +++++++ Zend/tests/generators/gh9750-004.phpt | 33 +++++++ Zend/tests/generators/gh9750-005.phpt | 34 +++++++ Zend/tests/generators/gh9750-006.phpt | 33 +++++++ Zend/tests/generators/gh9750-007.phpt | 33 +++++++ Zend/tests/generators/gh9750-008.phpt | 40 ++++++++ Zend/tests/generators/gh9750-009.phpt | 37 ++++++++ Zend/tests/generators/gh9750-010.phpt | 33 +++++++ Zend/tests/generators/gh9750-011.phpt | 38 ++++++++ Zend/zend_execute.c | 130 ++++++++++++++++++++++++++ Zend/zend_execute.h | 1 + Zend/zend_generators.c | 29 +++++- 14 files changed, 537 insertions(+), 3 deletions(-) create mode 100644 Zend/tests/generators/gh9750-001.phpt create mode 100644 Zend/tests/generators/gh9750-002.phpt create mode 100644 Zend/tests/generators/gh9750-003.phpt create mode 100644 Zend/tests/generators/gh9750-004.phpt create mode 100644 Zend/tests/generators/gh9750-005.phpt create mode 100644 Zend/tests/generators/gh9750-006.phpt create mode 100644 Zend/tests/generators/gh9750-007.phpt create mode 100644 Zend/tests/generators/gh9750-008.phpt create mode 100644 Zend/tests/generators/gh9750-009.phpt create mode 100644 Zend/tests/generators/gh9750-010.phpt create mode 100644 Zend/tests/generators/gh9750-011.phpt diff --git a/Zend/tests/generators/gh9750-001.phpt b/Zend/tests/generators/gh9750-001.phpt new file mode 100644 index 0000000000000..9a64912c25255 --- /dev/null +++ b/Zend/tests/generators/gh9750-001.phpt @@ -0,0 +1,33 @@ +--TEST-- +Bug GH-9750 001 (Generator memory leak when interrupted during argument evaluation) +--FILE-- + +==DONE== +--EXPECT-- +C::__destruct +==DONE== diff --git a/Zend/tests/generators/gh9750-002.phpt b/Zend/tests/generators/gh9750-002.phpt new file mode 100644 index 0000000000000..f46bd381283e4 --- /dev/null +++ b/Zend/tests/generators/gh9750-002.phpt @@ -0,0 +1,33 @@ +--TEST-- +Bug GH-9750 002 (Generator memory leak when interrupted during argument evaluation) +--FILE-- + +==DONE== +--EXPECT-- +C::__destruct +==DONE== diff --git a/Zend/tests/generators/gh9750-003.phpt b/Zend/tests/generators/gh9750-003.phpt new file mode 100644 index 0000000000000..f58abb2433124 --- /dev/null +++ b/Zend/tests/generators/gh9750-003.phpt @@ -0,0 +1,33 @@ +--TEST-- +Bug GH-9750 003 (Generator memory leak when interrupted during argument evaluation) +--FILE-- + +==DONE== +--EXPECT-- +C::__destruct +==DONE== diff --git a/Zend/tests/generators/gh9750-004.phpt b/Zend/tests/generators/gh9750-004.phpt new file mode 100644 index 0000000000000..ad26a9029e4bf --- /dev/null +++ b/Zend/tests/generators/gh9750-004.phpt @@ -0,0 +1,33 @@ +--TEST-- +Bug GH-9750 004 (Generator memory leak when interrupted during argument evaluation) +--FILE-- + +==DONE== +--EXPECT-- +C::__destruct +==DONE== diff --git a/Zend/tests/generators/gh9750-005.phpt b/Zend/tests/generators/gh9750-005.phpt new file mode 100644 index 0000000000000..1e849946917a8 --- /dev/null +++ b/Zend/tests/generators/gh9750-005.phpt @@ -0,0 +1,34 @@ +--TEST-- +Bug GH-9750 002 (Generator memory leak when interrupted during argument evaluation) +--FILE-- + +==DONE== +--EXPECT-- +C::__destruct +==DONE== diff --git a/Zend/tests/generators/gh9750-006.phpt b/Zend/tests/generators/gh9750-006.phpt new file mode 100644 index 0000000000000..6ba6c416d4af3 --- /dev/null +++ b/Zend/tests/generators/gh9750-006.phpt @@ -0,0 +1,33 @@ +--TEST-- +Bug GH-9750 006 (Generator memory leak when interrupted during argument evaluation) +--FILE-- + +==DONE== +--EXPECT-- +C::__destruct +==DONE== diff --git a/Zend/tests/generators/gh9750-007.phpt b/Zend/tests/generators/gh9750-007.phpt new file mode 100644 index 0000000000000..dbf91752ef5af --- /dev/null +++ b/Zend/tests/generators/gh9750-007.phpt @@ -0,0 +1,33 @@ +--TEST-- +Bug GH-9750 007 (Generator memory leak when interrupted during argument evaluation) +--FILE-- + +==DONE== +--EXPECT-- +C::__destruct +==DONE== diff --git a/Zend/tests/generators/gh9750-008.phpt b/Zend/tests/generators/gh9750-008.phpt new file mode 100644 index 0000000000000..a9cdacd19d156 --- /dev/null +++ b/Zend/tests/generators/gh9750-008.phpt @@ -0,0 +1,40 @@ +--TEST-- +Bug GH-9750 008 (Generator memory leak when interrupted during argument evaluation) +--SKIPIF-- + +--FILE-- + +==DONE== +--EXPECT-- +C::__destruct +==DONE== diff --git a/Zend/tests/generators/gh9750-009.phpt b/Zend/tests/generators/gh9750-009.phpt new file mode 100644 index 0000000000000..561ed081e379b --- /dev/null +++ b/Zend/tests/generators/gh9750-009.phpt @@ -0,0 +1,37 @@ +--TEST-- +Bug GH-9750 009 (Generator memory leak when interrupted during argument evaluation) +--FILE-- + +==DONE== +--EXPECT-- +C::__destruct +==DONE== diff --git a/Zend/tests/generators/gh9750-010.phpt b/Zend/tests/generators/gh9750-010.phpt new file mode 100644 index 0000000000000..ab5b6bf6d48ca --- /dev/null +++ b/Zend/tests/generators/gh9750-010.phpt @@ -0,0 +1,33 @@ +--TEST-- +Bug GH-9750 010 (Generator memory leak when interrupted during argument evaluation) +--FILE-- +valid(yield); +}; + +$gen = $gen(new C()); + +foreach ($gen as $value) { + break; +} + +$gen = null; + +gc_collect_cycles(); + +?> +==DONE== +--EXPECT-- +C::__destruct +==DONE== diff --git a/Zend/tests/generators/gh9750-011.phpt b/Zend/tests/generators/gh9750-011.phpt new file mode 100644 index 0000000000000..ab4c00a17358c --- /dev/null +++ b/Zend/tests/generators/gh9750-011.phpt @@ -0,0 +1,38 @@ +--TEST-- +Bug GH-9750 011 (Generator memory leak when interrupted during argument evaluation) +--FILE-- +getClosure()); + +foreach ($gen as $value) { + break; +} + +$gen = null; + +gc_collect_cycles(); + +?> +==DONE== +--EXPECT-- +C::__destruct +==DONE== diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index e13092491b59f..d5e35898f998d 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -3841,6 +3841,136 @@ static zend_always_inline zend_generator *zend_get_running_generator(EXECUTE_DAT } /* }}} */ +ZEND_API void zend_unfinished_calls_gc(zend_execute_data *execute_data, zend_execute_data *call, uint32_t op_num, zend_get_gc_buffer *buf) /* {{{ */ +{ + zend_op *opline = EX(func)->op_array.opcodes + op_num; + int level; + int do_exit; + uint32_t num_args; + + if (UNEXPECTED(opline->opcode == ZEND_INIT_FCALL || + opline->opcode == ZEND_INIT_FCALL_BY_NAME || + opline->opcode == ZEND_INIT_NS_FCALL_BY_NAME || + opline->opcode == ZEND_INIT_DYNAMIC_CALL || + opline->opcode == ZEND_INIT_USER_CALL || + opline->opcode == ZEND_INIT_METHOD_CALL || + opline->opcode == ZEND_INIT_STATIC_METHOD_CALL || + opline->opcode == ZEND_NEW)) { + ZEND_ASSERT(op_num); + opline--; + } + + do { + /* find the number of actually passed arguments */ + level = 0; + do_exit = 0; + num_args = ZEND_CALL_NUM_ARGS(call); + do { + switch (opline->opcode) { + case ZEND_DO_FCALL: + case ZEND_DO_ICALL: + case ZEND_DO_UCALL: + case ZEND_DO_FCALL_BY_NAME: + level++; + break; + case ZEND_INIT_FCALL: + case ZEND_INIT_FCALL_BY_NAME: + case ZEND_INIT_NS_FCALL_BY_NAME: + case ZEND_INIT_DYNAMIC_CALL: + case ZEND_INIT_USER_CALL: + case ZEND_INIT_METHOD_CALL: + case ZEND_INIT_STATIC_METHOD_CALL: + case ZEND_NEW: + if (level == 0) { + num_args = 0; + do_exit = 1; + } + level--; + break; + case ZEND_SEND_VAL: + case ZEND_SEND_VAL_EX: + case ZEND_SEND_VAR: + case ZEND_SEND_VAR_EX: + case ZEND_SEND_FUNC_ARG: + case ZEND_SEND_REF: + case ZEND_SEND_VAR_NO_REF: + case ZEND_SEND_VAR_NO_REF_EX: + case ZEND_SEND_USER: + if (level == 0) { + /* For named args, the number of arguments is up to date. */ + if (opline->op2_type != IS_CONST) { + num_args = opline->op2.num; + } + do_exit = 1; + } + break; + case ZEND_SEND_ARRAY: + case ZEND_SEND_UNPACK: + case ZEND_CHECK_UNDEF_ARGS: + if (level == 0) { + do_exit = 1; + } + break; + } + if (!do_exit) { + opline--; + } + } while (!do_exit); + if (call->prev_execute_data) { + /* skip current call region */ + level = 0; + do_exit = 0; + do { + switch (opline->opcode) { + case ZEND_DO_FCALL: + case ZEND_DO_ICALL: + case ZEND_DO_UCALL: + case ZEND_DO_FCALL_BY_NAME: + level++; + break; + case ZEND_INIT_FCALL: + case ZEND_INIT_FCALL_BY_NAME: + case ZEND_INIT_NS_FCALL_BY_NAME: + case ZEND_INIT_DYNAMIC_CALL: + case ZEND_INIT_USER_CALL: + case ZEND_INIT_METHOD_CALL: + case ZEND_INIT_STATIC_METHOD_CALL: + case ZEND_NEW: + if (level == 0) { + do_exit = 1; + } + level--; + break; + } + opline--; + } while (!do_exit); + } + + if (EXPECTED(num_args > 0)) { + zval *p = ZEND_CALL_ARG(call, 1); + do { + zend_get_gc_buffer_add_zval(buf, p); + p++; + } while (--num_args); + } + if (ZEND_CALL_INFO(call) & ZEND_CALL_RELEASE_THIS) { + zend_get_gc_buffer_add_obj(buf, Z_OBJ(call->This)); + } + if (ZEND_CALL_INFO(call) & ZEND_CALL_HAS_EXTRA_NAMED_PARAMS) { + zval *val; + ZEND_HASH_FOREACH_VAL(call->extra_named_params, val) { + zend_get_gc_buffer_add_zval(buf, val); + } ZEND_HASH_FOREACH_END(); + } + if (call->func->common.fn_flags & ZEND_ACC_CLOSURE) { + zend_get_gc_buffer_add_obj(buf, ZEND_CLOSURE_OBJECT(call->func)); + } + + call = call->prev_execute_data; + } while (call); +} +/* }}} */ + static void cleanup_unfinished_calls(zend_execute_data *execute_data, uint32_t op_num) /* {{{ */ { if (UNEXPECTED(EX(call))) { diff --git a/Zend/zend_execute.h b/Zend/zend_execute.h index ab96d43c4c351..072b794f7c76f 100644 --- a/Zend/zend_execute.h +++ b/Zend/zend_execute.h @@ -349,6 +349,7 @@ ZEND_API zval *zend_get_zval_ptr(const zend_op *opline, int op_type, const znode ZEND_API void zend_clean_and_cache_symbol_table(zend_array *symbol_table); ZEND_API void zend_free_compiled_variables(zend_execute_data *execute_data); +ZEND_API void zend_unfinished_calls_gc(zend_execute_data *execute_data, zend_execute_data *call, uint32_t op_num, zend_get_gc_buffer *buf); ZEND_API void zend_cleanup_unfinished_execution(zend_execute_data *execute_data, uint32_t op_num, uint32_t catch_op_num); zval * ZEND_FASTCALL zend_handle_named_arg( diff --git a/Zend/zend_generators.c b/Zend/zend_generators.c index a62ebaf5e3525..71022df8e548c 100644 --- a/Zend/zend_generators.c +++ b/Zend/zend_generators.c @@ -95,6 +95,20 @@ ZEND_API zend_execute_data* zend_generator_freeze_call_stack(zend_execute_data * } /* }}} */ +static zend_execute_data* zend_generator_revert_call_stack(zend_execute_data *call) +{ + zend_execute_data *prev = NULL; + + do { + zend_execute_data *next = call->prev_execute_data; + call->prev_execute_data = prev; + prev = call; + call = next; + } while (call); + + return prev; +} + static void zend_generator_cleanup_unfinished_execution( zend_generator *generator, zend_execute_data *execute_data, uint32_t catch_op_num) /* {{{ */ { @@ -370,6 +384,15 @@ static HashTable *zend_generator_get_gc(zend_object *object, zval **table, int * zend_get_gc_buffer_add_zval(gc_buffer, &extra_named_params); } + if (UNEXPECTED(generator->frozen_call_stack)) { + /* The frozen stack is linked in reverse order */ + zend_execute_data *call = zend_generator_revert_call_stack(generator->frozen_call_stack); + /* -1 required because we want the last run opcode, not the next to-be-run one. */ + uint32_t op_num = execute_data->opline - op_array->opcodes - 1; + zend_unfinished_calls_gc(execute_data, call, op_num, gc_buffer); + zend_generator_revert_call_stack(call); + } + if (execute_data->opline != op_array->opcodes) { uint32_t i, op_num = execute_data->opline - op_array->opcodes - 1; for (i = 0; i < op_array->last_live_range; i++) { @@ -604,7 +627,7 @@ ZEND_API zend_generator *zend_generator_update_current(zend_generator *generator static zend_result zend_generator_get_next_delegated_value(zend_generator *generator) /* {{{ */ { --generator->execute_data->opline; - + zval *value; if (Z_TYPE(generator->values) == IS_ARRAY) { HashTable *ht = Z_ARR(generator->values); @@ -670,7 +693,7 @@ static zend_result zend_generator_get_next_delegated_value(zend_generator *gener ZVAL_LONG(&generator->key, iter->index); } } - + ++generator->execute_data->opline; return SUCCESS; @@ -726,7 +749,7 @@ ZEND_API void zend_generator_resume(zend_generator *orig_generator) /* {{{ */ generator->execute_data->prev_execute_data = &orig_generator->execute_fake; orig_generator->execute_fake.prev_execute_data = original_execute_data; } - + /* Ensure this is run after executor_data swap to have a proper stack trace */ if (UNEXPECTED(!Z_ISUNDEF(generator->values))) { if (EXPECTED(zend_generator_get_next_delegated_value(generator) == SUCCESS)) {