Repository navigation
Conversation
dstogov
reviewed
Dec 24, 2024
dstogov
left a comment
Member
There was a problem hiding this comment.
I think, you are trying to fix the side effect but not the real problem.
We must not allow execution of PHP user code while SHM is unprotected.
I'm not completely sure if optimizer may invoke some user code directly (it shouldn't do this).
The test case starts user code from SCCP because of GC.
To fix this, I would propose to temporary disable GC .
diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c
index 3bb69eb107e..78a4b1710d6 100644
--- a/ext/opcache/ZendAccelerator.c
+++ b/ext/opcache/ZendAccelerator.c
@@ -2165,7 +2165,9 @@ zend_op_array *persistent_compile_file(zend_file_handle *file_handle, int type)
*/
from_shared_memory = false;
if (persistent_script) {
+ bool orig_gc_state = gc_enable(false);
persistent_script = cache_script_in_shared_memory(persistent_script, key, &from_shared_memory);
+ gc_enable(orig_gc_state);
}
/* Caching is disabled, returning op_array;This also may be not a complete solution. Please take care.
This bug happens because of a nested `SHM_UNPROTECT()` sequence. In particular: ``` unprotect memory at ext/opcache/ZendAccelerator.c:2127 protect memory at ext/opcache/ZendAccelerator.c:2160 unprotect memory at ext/opcache/ZendAccelerator.c:2164 unprotect memory at ext/opcache/jit/zend_jit_trace.c:7464 ^^^ Nested protect memory at ext/opcache/jit/zend_jit_trace.c:7591 ^^^ Problem is here: it should not protect again due to the nested unprotect protect memory at ext/opcache/ZendAccelerator.c:2191 ^^^ This one should actually protect, not the previous one ``` The reason this nesting happen is because: 1. We try to include the script, this eventually calls `cache_script_in_shared_memory` 2. `zend_optimize_script` will eventually run SCCP as part of the DFA pass. 3. SCCP will try to replace constants, but can also run destructors when a partial array is destructed here: https://github.com/php/php-src/blob/4e9cde758eadf30cc4d596d6398c2c34c64197b4/Zend/Optimizer/sccp.c#L2387-L2389 In this case, this destruction invokes the GC which invokes the tracing JIT, leading to the nested unprotects. This patch disables the GC to prevent invoking user code, as user code is not supposed to run during the optimizer pipeline. Co-authored-by: Dmitry Stogov <[email protected]>
Member
Author
|
Your suggestion makes sense, I forgot that we shouldn't execute user code during optimization pipeline. I tried to cheat it but couldn't find a way, so I'll commit your patch. |
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
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.
This bug happens because of a nested
SHM_UNPROTECT()sequence. In particular:The reason this nesting happen is because:
cache_script_in_shared_memoryzend_optimize_scriptwill eventually run SCCP as part of the DFA pass.php-src/Zend/Optimizer/sccp.c
Lines 2387 to 2389 in 4e9cde7
In this case, this destruction invokes the tracing JIT, leading to the nested unprotects.
This patch adds a counter to track the nesting level of shm protections. This is a simple solution to the problem. An alternative is not JITting during SCCP, but that would be a leaky abstraction. I also think this solution is more general.
One downside is an extra check in case
protect_memoryis set, but production configuration usually don't have this enabled. It may be possible to combine theprotect_memoryandshm_nesting_levelfield in some way, but that will make more complicated code.