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 15981: Segfault with frameless jumps and minimal JIT #17329

Closed
wants to merge 2 commits into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Jan 2, 2025

Minimal JIT shouldn't generate a call to the complex handler, but instead rely on the VM and then check for a two-way jump. This moves the frameless codegen under the check JIT_G(opt_level) >= ZEND_JIT_LEVEL_INLINE. (See issue for more discussion)

I'm fixing this finally because some fuzzer constantly hit this.

Minimal JIT shouldn't generate a call to the complex handler, but
instead rely on the VM and then check for a two-way jump.
This moves the frameless codegen under the check `JIT_G(opt_level) >=
ZEND_JIT_LEVEL_INLINE`.
@nielsdos nielsdos requested a review from dstogov as a code owner January 2, 2025 22:49
@nielsdos nielsdos requested a review from iluuu1994 January 2, 2025 22:49
@nielsdos nielsdos linked an issue Jan 2, 2025 that may be closed by this pull request
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

@nielsdos Thank you very much for taking care of this. With my limited understanding, this looks correct. I don't know exactly what rules should be used to decide if something needs to be compiled for ZEND_JIT_LEVEL_MINIMAL. In that case, maybe ZEND_FRAMELESS_ICALL_n itself also shouldn't? @dstogov can maybe confirm (he's on holiday until next week). For now, this looks harmless as it only changes the rarely used minimal mode, and is broken as-is anyway.

@nielsdos
Copy link
Member Author

nielsdos commented Jan 6, 2025

Right, I suppose the frameless icalss should also go via the VM because the optimization level is below inline VM handlers. Fortunately that handler doesn't seem to cause issues.

@iluuu1994
Copy link
Member

Ok. It's probably fine to do this here as well then, I think.

@nielsdos
Copy link
Member Author

nielsdos commented Jan 6, 2025

Ok. I moved those too, in a new commit such that the actual fix is still in the first commit separately. We can wait for Dmitry to confirm.

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.

So simple :), Thanks!

@nielsdos
Copy link
Member Author

nielsdos commented Jan 9, 2025

Merged via 72184ab

@nielsdos nielsdos closed this Jan 9, 2025
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.

Segfault with frameless jumps and minimal JIT
3 participants