GeezerVison is showing github.com — Fix GH-17990: Tracing JIT uses the SSA of a dead process by kamil-tekiela · Pull Request #25399 · php/php-src
Conversation
The tracing JIT stores the SSA of each function of a trace in the func_info of the op_array, which is in shared memory. The SSA itself is in the arena of the compiling process, and zend_jit_trace() resets the func_info when the compilation ends.
A process that dies during the compilation leaves the func_info behind. The next process that compiles a trace through the same function finds a non-zero ssa->cfg.blocks_count and reads the pointers of the dead process.
Reuse an SSA only if the current compilation built it, and block interruptions during the compilation, as the rest of opcache does while it changes shared memory.
Fixes #17990 Can you check if this fixes the problem? @YuanchengJiang
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The tracing JIT stores the SSA of each function of a trace in the
func_info of the op_array, which is in shared memory. The SSA itself is
in the arena of the compiling process, and zend_jit_trace() resets the
func_info when the compilation ends.
A process that dies during the compilation leaves the func_info behind.
The next process that compiles a trace through the same function finds
a non-zero ssa->cfg.blocks_count and reads the pointers of the dead
process.
Reuse an SSA only if the current compilation built it, and block
interruptions during the compilation, as the rest of opcache does while
it changes shared memory.
Co-Authored-By: Claude Fable 5.2 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The zend_jit_trace_has_ssa() check is what fixes this: on its own it makes both your SIGKILL test and a SIGTERM variant pass, while the interruption blocking on its own only fixes SIGTERM. It's worth saying that in the commit message.
Not for this PR, but both compile functions install the new code (opline->handler = handler, zend_jit_link_side_trace()) before ZEND_JIT_TRACE_NUM++, so a SIGKILL between those stores leaves live code pointing at a trace slot the next compilation reuses. The window is only a few stores.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
zend_jit_trace_exit_info exit_info[ZEND_JIT_TRACE_MAX_EXITS];
|
|
|
bool do_bailout = 0;
|
|
|
|
|
|
/* Don't let a signal kill the process while func_info in SHM refers to our arena */
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only defers the signals zend_signal handles (SIGTERM, SIGINT, SIGHUP...). SIGKILL, a segfault or the OOM killer still end the process here, and that is the case your test covers. Maybe /* Defer signals while func_info in SHM refers to our arena */?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
zend_jit_trace_exit_info exit_info[ZEND_JIT_TRACE_MAX_EXITS];
|
|
|
bool do_bailout = 0;
|
|
|
|
|
|
/* Don't let a signal kill the process while func_info in SHM refers to our arena */
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
iliaal
iliaal approved these changes
dstogov
Awaiting requested review from dstogov
dstogov is a code owner
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.