From 4d72bd73040447f7621db65030b54a42830ebb53 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 30 Dec 2024 16:13:40 +0100 Subject: [PATCH 1/2] Fix GH-12837: Combination of phpdbg and Generator method causes memory leak The fix for bug #72523 (d1dd474) added a check on `zend_execute_ex` to add an extra refcount to `This`. This is necessary because the VM does a re-entry here: https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4294-L4299 and then cleans up `This` here: https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4358-L4360 So if we don't add the refcount, we destroy `This` both in the VM and in `zend_generator_close`. The reason this causes a leak in phpdbg is because it changes the `zend_execute_ex` temporarily back to the default here for `ZEND_DO_FCALL`: https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/sapi/phpdbg/phpdbg_prompt.c#L1820-L1827 which means that we only execute `OBJ_RELEASE` on the `This` object once in `zend_generator_close` instead of also doing it in the `ZEND_DO_FCALL` handler. To solve this, we make sure the check for the re-entrancy is consistently done by checking the `ZEND_CALL_TOP` flag instead of relying solely on `execute_ex` pointer: https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4298 --- Zend/zend_vm_def.h | 2 +- Zend/zend_vm_execute.h | 2 +- sapi/phpdbg/tests/gh12837.phpt | 44 ++++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) create mode 100644 sapi/phpdbg/tests/gh12837.phpt diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h index 53aa7a821f69..5ee3cda890f3 100644 --- a/Zend/zend_vm_def.h +++ b/Zend/zend_vm_def.h @@ -4534,7 +4534,7 @@ ZEND_VM_HANDLER(139, ZEND_GENERATOR_CREATE, ANY, ANY) if ((call_info & Z_TYPE_MASK) == IS_OBJECT && (!(call_info & (ZEND_CALL_CLOSURE|ZEND_CALL_RELEASE_THIS)) /* Bug #72523 */ - || UNEXPECTED(zend_execute_ex != execute_ex))) { + || (call_info & ZEND_CALL_TOP))) { ZEND_ADD_CALL_FLAG_EX(call_info, ZEND_CALL_RELEASE_THIS); Z_ADDREF(gen_execute_data->This); } diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h index 2c86a94134c0..b2008afb1b32 100644 --- a/Zend/zend_vm_execute.h +++ b/Zend/zend_vm_execute.h @@ -2177,7 +2177,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_GENERATOR_CREATE_SPEC_HANDLER( if ((call_info & Z_TYPE_MASK) == IS_OBJECT && (!(call_info & (ZEND_CALL_CLOSURE|ZEND_CALL_RELEASE_THIS)) /* Bug #72523 */ - || UNEXPECTED(zend_execute_ex != execute_ex))) { + || (call_info & ZEND_CALL_TOP))) { ZEND_ADD_CALL_FLAG_EX(call_info, ZEND_CALL_RELEASE_THIS); Z_ADDREF(gen_execute_data->This); } diff --git a/sapi/phpdbg/tests/gh12837.phpt b/sapi/phpdbg/tests/gh12837.phpt new file mode 100644 index 000000000000..57a631c8849c --- /dev/null +++ b/sapi/phpdbg/tests/gh12837.phpt @@ -0,0 +1,44 @@ +--TEST-- +GH-12837 (Combination of phpdbg and Generator method causes memory leak) +--CREDITS-- +ngyuki +--FILE-- +iterate()); gc_collect_cycles(); +$arr = iterator_to_array((new A())->iterate()); gc_collect_cycles(); +$arr = iterator_to_array((new A())->iterate()); gc_collect_cycles(); + +var_dump('exit'); +?> +--PHPDBG-- +r +q +--EXPECTF-- +[Successful compilation of %s] +prompt> string(11) "__construct" +string(10) "__destruct" +string(11) "__construct" +string(10) "__destruct" +string(11) "__construct" +string(10) "__destruct" +string(4) "exit" +[Script ended normally] +prompt> From 38c1e971d322178fb8f732e9d7cc05d8c637dbcb Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 30 Dec 2024 19:55:03 +0100 Subject: [PATCH 2/2] Not necessary for dynamic calls --- Zend/zend_vm_def.h | 2 +- Zend/zend_vm_execute.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h index 5ee3cda890f3..20891c8cb68f 100644 --- a/Zend/zend_vm_def.h +++ b/Zend/zend_vm_def.h @@ -4534,7 +4534,7 @@ ZEND_VM_HANDLER(139, ZEND_GENERATOR_CREATE, ANY, ANY) if ((call_info & Z_TYPE_MASK) == IS_OBJECT && (!(call_info & (ZEND_CALL_CLOSURE|ZEND_CALL_RELEASE_THIS)) /* Bug #72523 */ - || (call_info & ZEND_CALL_TOP))) { + || (call_info & (ZEND_CALL_DYNAMIC|ZEND_CALL_TOP)) == ZEND_CALL_TOP)) { ZEND_ADD_CALL_FLAG_EX(call_info, ZEND_CALL_RELEASE_THIS); Z_ADDREF(gen_execute_data->This); } diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h index b2008afb1b32..89be7af7040c 100644 --- a/Zend/zend_vm_execute.h +++ b/Zend/zend_vm_execute.h @@ -2177,7 +2177,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_GENERATOR_CREATE_SPEC_HANDLER( if ((call_info & Z_TYPE_MASK) == IS_OBJECT && (!(call_info & (ZEND_CALL_CLOSURE|ZEND_CALL_RELEASE_THIS)) /* Bug #72523 */ - || (call_info & ZEND_CALL_TOP))) { + || (call_info & (ZEND_CALL_DYNAMIC|ZEND_CALL_TOP)) == ZEND_CALL_TOP)) { ZEND_ADD_CALL_FLAG_EX(call_info, ZEND_CALL_RELEASE_THIS); Z_ADDREF(gen_execute_data->This); }