Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GH-12837: Combination of phpdbg and Generator method causes memory leak #17303

Open
wants to merge 2 commits into
base: PHP-8.3
Choose a base branch
from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Dec 30, 2024

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:

php-src/Zend/zend_vm_def.h

Lines 4294 to 4299 in ecb90c1

SAVE_OPLINE_EX();
ZEND_OBSERVER_FCALL_BEGIN(execute_data);
execute_data = EX(prev_execute_data);
LOAD_OPLINE();
ZEND_ADD_CALL_FLAG(call, ZEND_CALL_TOP);
zend_execute_ex(call);

and then cleans up This here:

php-src/Zend/zend_vm_def.h

Lines 4358 to 4360 in ecb90c1

if (UNEXPECTED(ZEND_CALL_INFO(call) & ZEND_CALL_RELEASE_THIS)) {
OBJ_RELEASE(Z_OBJ(call->This));
}

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 phpdbg temporarily changes zend_execute_ex to the default here for ZEND_DO_FCALL:

if ((execute_data->opline->opcode == ZEND_DO_FCALL ||
execute_data->opline->opcode == ZEND_DO_UCALL ||
execute_data->opline->opcode == ZEND_DO_FCALL_BY_NAME) &&
execute_data->call->func->type == ZEND_USER_FUNCTION) {
zend_execute_ex = execute_ex;
}
PHPDBG_G(vmret) = zend_vm_call_opcode_handler(execute_data);
zend_execute_ex = phpdbg_execute_ex;
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 between ZEND_DO_FCALL and ZEND_GENERATOR_CREATE by checking the ZEND_CALL_TOP flag instead of relying solely on execute_ex pointer, because this flag is set when zend_execute_ex was changed:

ZEND_ADD_CALL_FLAG(call, ZEND_CALL_TOP);

It may over-approximate if called via zend_call_function, as that sets the ZEND_CALL_TOP flag too, but we could filter that by checking for ZEND_CALL_DYNAMIC too.

Alternatively, we can fix this solely in phpdbg by also executing GENERATOR_CREATE with the normal execution handler, but I think the real problem is in the VM inconsistency.

…mory 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
@nielsdos nielsdos marked this pull request as ready for review December 30, 2024 16:30
@nielsdos nielsdos requested a review from dstogov as a code owner December 30, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combination of phpdbg and Generator method causes memory leak
1 participant