Fix GH-16041: Support stack limit in phpdbg SAPI #16055
Conversation
|
Are you going to backport this to 8.4? |
|
I've just changed the target to 8.4, but actually I'm not sure. Should I target 8.4? |
Zend/zend_vm_execute.h
Outdated
|
|
||
| #ifdef ZEND_CHECK_STACK_LIMIT | ||
| if (UNEXPECTED(zend_call_stack_overflowed(EG(stack_limit)))) { | ||
| zend_call_stack_size_error(); | ||
| /* No opline was executed before exception */ | ||
| EG(opline_before_exception) = NULL; | ||
| LOAD_OPLINE(); | ||
| /* Fall through to handle exception below. */ | ||
| } | ||
| #endif /* ZEND_CHECK_STACK_LIMIT */ | ||
|
|
There was a problem hiding this comment.
May be it's better to make this check in phpdbg_execute_ex()?
There was a problem hiding this comment.
Yes, this would also work. I've made the change.
arnaud-lb
changed the title
Hum, as you are exposing a new function this might be an ABI problem, but I don't remember the rules about this. Maybe @php/release-managers-84 could chime in here. |
|
(Quick observation of the issue, don't take this as consensus) My understanding is adding a function doesn't constitute an ABI break in the traditional sense (that is, things will keep working when linked; it's not a SOVER bump), but it might constitute bumping the Zend ABI version, since extensions might try to rely on it and it might not be there. |
|
Thank you! In this case it should be ok then, as the function is not exported (not |
|
If it's not supposed to be API surface and extensions shouldn't use it, then it should be fine? We could mark it with |
|
I confirm that it's not supposed to be API surface, so it should be fine. Regardless of ABI breaks, can this change land in 8.4? This fixes GH-16041, but it can be seen as a new feature since this adds support for stack limit to phpdbg. |
|
@Girgias We've been routinely adding new API functions to existing PHP versions, so that's a non-problem. You as an extension author just have to make sure that you either declare some specific PHP versions unsupported or guard against it with e.g. a weak symbol, if you use them. |
|
Based on that, the fact it fixes a segfault, and the fact it's really just making things more consistent by reusing existing functionality elsewhere, I think targeting 8.4 makes sense. I'll let the other RMs chime in if they disagree and/or have more to add though. |
* PHP-8.4: [ci skip] NEWS for GH-16055 Support stack limit in phpdbg SAPI
Add a stack limit check in
zend_vm_call_opcode_handler()for SAPIs that do not useexecute_ex(), like phpdbg