fix(profiling): use-after-free with Closure trampolines - #2280
Merged
Conversation
bwoebi
reviewed
Sep 22, 2023
|
|
||
| Our zend_execute_internal hook inspected the func after the call has been made, | ||
| potentially triggering the issue. This test will likely only fail under asan. | ||
| It's unclear how the customer also got a crash out of it. |
Collaborator
There was a problem hiding this comment.
It basically crashes if the 18th (ZEND_ACC_CALL_VIA_TRAMPOLINE) bit of the address is not set.
This may be different on different executions, depending on base address (depending on OS), amount of memory allocated before (that's just 256 KB), address randomization etc.
bwoebi
approved these changes
Sep 22, 2023
bwoebi
left a comment
bwoebi
left a comment
Collaborator
There was a problem hiding this comment.
I wouldn't say that it's unclear how it crashed :-D
But the fix looks good and correct to me :-)
Collaborator
Author
Well sure, use-after-frees are obviously wrong even if they don't lead to crashes ^_^ I just couldn't ever get it to crash myself, hence the notes about it only failing in ASAN. I suppose that's clear enough. |
realFlowControl
approved these changes
Sep 23, 2023
Comment on lines
+1031
to
+1037
| /// Returns true if the func tied to the execute_data is a trampoline. | ||
| /// # Safety | ||
| /// This is only safe to execute _before_ executing the trampoline, because the trampoline may | ||
| /// free the `execute_data.func` _without_ setting it to NULL: | ||
| /// https://heap.space/xref/PHP-8.2/Zend/zend_closures.c?r=af2110e6#60-63 | ||
| /// So no code can inspect the func after the call has been made, which is why you would call this function: find out before you | ||
| /// call the function if indeed you need to skip certain code after it has been executed. |
4 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
A customer reported a crash which lead to the discovery of this use-after-free. Note that this is limited to Closure trampolines, and only when the profiler is enabled. It has been a potential issue for some time, but seems to be more likely to cause a crash since #1998 was merged.
Readiness checklist
Reviewer checklist