Skip to content

ext/opcache/jit: handle zend_jit_find_trace() failures #10153

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

Closed
wants to merge 1 commit into from

Conversation

MaxKellermann
Copy link
Contributor

Commit 6c25413 added the flag ZEND_JIT_EXIT_INVALIDATE which resets the trace handlers in zend_jit_trace_exit(), but forgot to consider that on ZEND_JIT_TRACE_STOP_LINK, this changed handler gets passed to zend_jit_find_trace(), causing it to fail, either by returning 0 (results in bogus data) or by aborting due to ZEND_UNREACHABLE(). In either case, this crashes the PHP process.

I'm not quite sure how to fix this multi-threading problem properly; my suggestion is to just fail the zend_jit_trace() call. After all, the whole ZEND_JIT_EXIT_INVALIDATE fix was about reloading modified scripts, so there's probably no point in this pending zend_jit_trace() call.

Commit 6c25413 added the flag ZEND_JIT_EXIT_INVALIDATE which resets
the trace handlers in zend_jit_trace_exit(), but forgot to consider
that on ZEND_JIT_TRACE_STOP_LINK, this changed handler gets passed to
zend_jit_find_trace(), causing it to fail, either by returning 0
(results in bogus data) or by aborting due to ZEND_UNREACHABLE().  In
either case, this crashes the PHP process.

I'm not quite sure how to fix this multi-threading problem properly;
my suggestion is to just fail the zend_jit_trace() call.  After all,
the whole ZEND_JIT_EXIT_INVALIDATE fix was about reloading modified
scripts, so there's probably no point in this pending zend_jit_trace()
call.
@MaxKellermann
Copy link
Contributor Author

@dstogov, please review.
This fix is related to #10138 and I think both are necessary.

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

The patch looks fine. Thanks.

@devnexen
Copy link
Member

Merged with b26b7589 thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants