From d721dcc2efd91dbefbca2e1d0e11982af30f4cc3 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Fri, 10 Feb 2023 18:07:34 +0100 Subject: [PATCH] Fix colletion of unfinished function call in fibers Fixes GH-10496. Co-authored-by: Bob Weinand --- NEWS | 2 ++ Zend/tests/fibers/gh10496.phpt | 33 +++++++++++++++++++++++++++++++++ Zend/zend_execute.c | 30 +++++++++++++++++++++++++++--- Zend/zend_execute.h | 3 ++- Zend/zend_fibers.c | 2 +- Zend/zend_generators.c | 2 +- 6 files changed, 66 insertions(+), 6 deletions(-) create mode 100644 Zend/tests/fibers/gh10496.phpt diff --git a/NEWS b/NEWS index 6d6114f7c0f..8976f64b3f4 100644 --- a/NEWS +++ b/NEWS @@ -24,6 +24,8 @@ PHP NEWS - Fiber: . Fixed assembly on alpine x86. (nielsdos) + . Fixed bug GH-10496 (segfault when garbage collector is invoked inside of + fiber). (Bob, Arnaud) - FPM: . Fixed bug GH-10315 (FPM unknown child alert not valid). (Jakub Zelenka) diff --git a/Zend/tests/fibers/gh10496.phpt b/Zend/tests/fibers/gh10496.phpt new file mode 100644 index 00000000000..76ea5fea78c --- /dev/null +++ b/Zend/tests/fibers/gh10496.phpt @@ -0,0 +1,33 @@ +--TEST-- +Bug GH-10496 (Segfault when garbage collector is invoked inside of fiber) +--FILE-- +start(); +unset($f); +gc_collect_cycles(); +print "Collected\n"; + +?> +--EXPECT-- +Cleaned +Dtor x() +Collected diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index 49af6fc7695..848d479651f 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -4459,7 +4459,24 @@ ZEND_API void zend_cleanup_unfinished_execution(zend_execute_data *execute_data, cleanup_live_vars(execute_data, op_num, catch_op_num); } -ZEND_API HashTable *zend_unfinished_execution_gc(zend_execute_data *execute_data, zend_execute_data *call, zend_get_gc_buffer *gc_buffer) +ZEND_API ZEND_ATTRIBUTE_DEPRECATED HashTable *zend_unfinished_execution_gc(zend_execute_data *execute_data, zend_execute_data *call, zend_get_gc_buffer *gc_buffer) +{ + bool suspended_by_yield = false; + + if (Z_TYPE_INFO(EX(This)) & ZEND_CALL_GENERATOR) { + ZEND_ASSERT(EX(return_value)); + + /* The generator object is stored in EX(return_value) */ + zend_generator *generator = (zend_generator*) EX(return_value); + ZEND_ASSERT(execute_data == generator->execute_data); + + suspended_by_yield = !(generator->flags & ZEND_GENERATOR_CURRENTLY_RUNNING); + } + + return zend_unfinished_execution_gc_ex(execute_data, call, gc_buffer, suspended_by_yield); +} + +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)) { return NULL; @@ -4495,8 +4512,15 @@ ZEND_API HashTable *zend_unfinished_execution_gc(zend_execute_data *execute_data } if (call) { - /* -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; + uint32_t op_num = execute_data->opline - op_array->opcodes; + if (suspended_by_yield) { + /* When the execution was suspended by yield, EX(opline) points to + * next opline to execute. Otherwise, it points to the opline that + * suspended execution. */ + op_num--; + ZEND_ASSERT(EX(func)->op_array.opcodes[op_num].opcode == ZEND_YIELD + || EX(func)->op_array.opcodes[op_num].opcode == ZEND_YIELD_FROM); + } zend_unfinished_calls_gc(execute_data, call, op_num, gc_buffer); } diff --git a/Zend/zend_execute.h b/Zend/zend_execute.h index 1091b1ebe65..eaad706cee3 100644 --- a/Zend/zend_execute.h +++ b/Zend/zend_execute.h @@ -410,7 +410,8 @@ ZEND_API void zend_clean_and_cache_symbol_table(zend_array *symbol_table); ZEND_API void ZEND_FASTCALL 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); -ZEND_API HashTable *zend_unfinished_execution_gc(zend_execute_data *execute_data, zend_execute_data *call, zend_get_gc_buffer *gc_buffer); +ZEND_API ZEND_ATTRIBUTE_DEPRECATED HashTable *zend_unfinished_execution_gc(zend_execute_data *execute_data, zend_execute_data *call, zend_get_gc_buffer *gc_buffer); +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); zval * ZEND_FASTCALL zend_handle_named_arg( zend_execute_data **call_ptr, zend_string *arg_name, diff --git a/Zend/zend_fibers.c b/Zend/zend_fibers.c index 051a9dceaea..a0892effdf2 100644 --- a/Zend/zend_fibers.c +++ b/Zend/zend_fibers.c @@ -652,7 +652,7 @@ 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->call, buf); + HashTable *symTable = zend_unfinished_execution_gc_ex(ex, ex->call, buf, false); if (symTable) { if (lastSymTable) { zval *val; diff --git a/Zend/zend_generators.c b/Zend/zend_generators.c index a192170cae3..5d7bef3854f 100644 --- a/Zend/zend_generators.c +++ b/Zend/zend_generators.c @@ -372,7 +372,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(execute_data, call, gc_buffer); + zend_unfinished_execution_gc_ex(execute_data, call, gc_buffer, true); if (UNEXPECTED(generator->frozen_call_stack)) { zend_generator_revert_call_stack(call);