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 failing VirtualProtect() calls #6470

Closed
wants to merge 2 commits into from
Closed

Conversation

cmb69
Copy link
Contributor

@cmb69 cmb69 commented Nov 30, 2020

If JIT is disabled due to incompatible zend_execute_ex, only
JIT_G(enabled) is set to zero, but JIT_G(on) keeps its value.
However, calling VirtualProtect() with NULL fails with
ERROR_INVALID_PARAMETER. Thus, we check for JIT_G(enabled) as
well, before (un)protecting the SHM.


For reference: the diff of the failing test case.

However, with this patch, the test is still marked as borked:

=====================================================================
BORKED TEST SUMMARY
---------------------------------------------------------------------
Warning: JIT is incompatible with third party extensions that override zend_execute_ex(). JIT disabled. in Unknown on line 0 [%s\ext\opcache\tests\jit\bug426.phpt]
=====================================================================

If JIT is disabled due to incompatible `zend_execute_ex`, only
`JIT_G(enabled)` is set to zero, but `JIT_G(on)` keeps its value.
However, calling `VirtualProtect()` with `NULL` fails with
`ERROR_INVALID_PARAMETER`.  Thus, we check for `JIT_G(enabled)` as
well, before (un)protecting the SHM.
@cmb69
Copy link
Contributor Author

cmb69 commented Nov 30, 2020

Maybe the test is borked only in debug mode; apparently it succeeds now in CI.

@cmb69 cmb69 requested a review from dstogov November 30, 2020 21:18
@nikic
Copy link
Member

nikic commented Dec 2, 2020

I think instead we should change

if (JIT_G(buffer_size) == 0
to check for jit_size instead of JIT_G(buffer_size), and let it set enabled and on to 0 there.

@cmb69
Copy link
Contributor Author

cmb69 commented Dec 2, 2020

The problem is that this code is not exectuted, because JIT_G(enabled) is already false in that case. When looking at d5a82e2, I wonder whether JIT_G(on) should also be set to 0 when JIT_G(enabled) is set to 0. Frankly, I don't understand the difference between these globals.

@nikic
Copy link
Member

nikic commented Dec 2, 2020

The problem is that this code is not exectuted, because JIT_G(enabled) is already false in that case. When looking at d5a82e2, I wonder whether JIT_G(on) should also be set to 0 when JIT_G(enabled) is set to 0. Frankly, I don't understand the difference between these globals.

Ah, good point! Yes, I think that code should set both enabled and on to 0.

The difference between the two options is (I believe) that one determines whether the JIT is enabled at all, while the other determines whether it's currently active.

Whenever JIT is disabled due to incompatibilities, we also need to set
`JIT_G(on)` to zero.
@cmb69
Copy link
Contributor Author

cmb69 commented Dec 2, 2020

Thanks!

@php-pulls php-pulls closed this in 72cd579 Dec 2, 2020
@cmb69 cmb69 deleted the cmb/virtual-protect branch December 13, 2020 22:57
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.

None yet

2 participants