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-16041: Support stack limit in phpdbg SAPI #16055

Open
wants to merge 1 commit into
base: PHP-8.4
Choose a base branch
from

Conversation

arnaud-lb
Copy link
Member

Add a stack limit check in zend_vm_call_opcode_handler() for SAPIs that do not use execute_ex(), like phpdbg

@Girgias
Copy link
Member

Girgias commented Sep 25, 2024

Are you going to backport this to 8.4?

@arnaud-lb arnaud-lb changed the base branch from master to PHP-8.4 September 25, 2024 17:35
@arnaud-lb
Copy link
Member Author

I've just changed the target to 8.4, but actually I'm not sure. Should I target 8.4?

@arnaud-lb arnaud-lb marked this pull request as ready for review September 25, 2024 17:36
Comment on lines 68347 to 68357

#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 */

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be it's better to make this check in phpdbg_execute_ex()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this would also work. I've made the change.

@arnaud-lb arnaud-lb changed the title Support stack limit in phpdbg SAPI Fix GH-16041: Support stack limit in phpdbg SAPI Sep 25, 2024
@arnaud-lb arnaud-lb linked an issue Sep 25, 2024 that may be closed by this pull request
@Girgias
Copy link
Member

Girgias commented Sep 25, 2024

I've just changed the target to 8.4, but actually I'm not sure. Should I target 8.4?

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.

@NattyNarwhal
Copy link
Member

NattyNarwhal commented Sep 25, 2024

(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.

@arnaud-lb
Copy link
Member Author

Thank you! In this case it should be ok then, as the function is not exported (not ZEND_API)?

@NattyNarwhal
Copy link
Member

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 ZEND_API if we want in 8.5. (Would like to hear other RMs opinion on this, mine isn't too strong here.)

@arnaud-lb
Copy link
Member Author

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.

@bwoebi
Copy link
Member

bwoebi commented Sep 27, 2024

@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.

@NattyNarwhal
Copy link
Member

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.

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.

Segmentation fault (stack overflow in phpdbg) in main/spprintf.c
5 participants