Fixed GH-16233: Observer segfault when calling user function in internal function via trampoline #16252
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ternal function via trampoline In the test, I have an internal `__call` function for `_ZendTestMagicCallForward` that calls the global function with name `$name` via `call_user_function`. Note that observer writes the pointer to the previously observed frame in the last temporary of the new call frame (`*prev_observed_frame`). The following happens: First, we call `$test->callee`, this will be handled via a trampoline with T=2 for the two arguments. The call frame is allocated at this point. This call frame is not observed because it has `ZEND_ACC_CALL_VIA_TRAMPOLINE` set. Next we use `ZEND_CALL_TRAMPOLINE` to call the trampoline, this reuses the stack frame allocated earlier with T=2, but this time it is observed. The pointer to the previous frame is written outside of the call frame because `T` is too small (should be 3). We are now in the internal function `_ZendTestMagicCallForward::__call` where we call the global function `callee`. This will push a new call frame which will overlap `*prev_observed_frame`. This value gets overwritten by `zend_init_func_execute_data` when `EX(opline)` is set because `*prev_observed_frame` overlaps with `EX(opline)`. From now on, `*prev_observed_frame` is corrupted. When `zend_observer_fcall_end` is called this will result in reading wrong value `*prev_observed_frame` into `current_observed_frame`. This causes issues in `zend_observer_fcall_end_all` leading to the segfault we observe. Despite function with `ZEND_ACC_CALL_VIA_TRAMPOLINE` not being observed, the reuse of call frames makes problems when `T` is not large enough. To fix this, we make sure to add 1 to `T` if `ZEND_OBSERVER_ENABLED` is true.
|
Probably, this is right. cc: @bwoebi |
|
As a side-note: while looking at this I found that EG(trampoline) T and such isn't reset at all for property hooks trampoline. It's not a problem because it's an upper bound and prop hooks don't need temporaries on their own, it'll just alloc more slots than needed. |
|
I'll have a look at the property hook thing. |
nielsdos
added a commit
to nielsdos/php-src
that referenced
this pull request
Oct 7, 2024
…polines As pointed out in php#16252 (comment)
nielsdos
added a commit
that referenced
this pull request
Mar 25, 2025
…polines (#16287) As pointed out in #16252 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the test, I have an internal
__callfunction for_ZendTestMagicCallForwardthat calls the global function with name$nameviacall_user_function. Note that observer writes the pointer to the previously observed frame in the last temporary of the new call frame (*prev_observed_frame).The following happens:
First, we call
$test->callee, this will be handled via a trampoline with T=2 for the two arguments. The call frame is allocated at this point. This call frame is not observed because it hasZEND_ACC_CALL_VIA_TRAMPOLINEset. Next we useZEND_CALL_TRAMPOLINEto call the trampoline, this reuses the stack frame allocated earlier with T=2, but this time it is observed. The pointer to the previous frame is written outside of the call frame becauseTis too small (should be 3). We are now in the internal function_ZendTestMagicCallForward::__callwhere we call the global functioncallee. This will push a new call frame which will overlap*prev_observed_frame. This value gets overwritten byzend_init_func_execute_datawhenEX(opline)is set because*prev_observed_frameoverlaps withEX(opline). From now on,*prev_observed_frameis corrupted. Whenzend_observer_fcall_endis called this will result in reading wrong value*prev_observed_frameintocurrent_observed_frame. This causes issues inzend_observer_fcall_end_allleading to the segfault we observe.Despite function with
ZEND_ACC_CALL_VIA_TRAMPOLINEnot being observed, the reuse of call frames makes problems whenTis not large enough. To fix this, we make sure to add 1 toTifZEND_OBSERVER_ENABLEDis true.