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 GH-17246: Nested shm protections cause segfault #17249

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

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:

php-src/Zend/Optimizer/sccp.c

Lines 2387 to 2389 in 4e9cde7

if (!Z_DELREF(ctx->values[i])) {
zend_array_destroy(Z_ARR(ctx->values[i]));
}

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_memory is set, but production configuration usually don't have this enabled. It may be possible to combine the protect_memory and shm_nesting_level field in some way, but that will make more complicated code.

@nielsdos nielsdos linked an issue Dec 23, 2024 that may be closed by this pull request
@nielsdos nielsdos marked this pull request as ready for review December 23, 2024 19:06
@nielsdos nielsdos requested a review from dstogov as a code owner December 23, 2024 19:06
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.

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]>
@nielsdos
Copy link
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.

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.

Nested shm protections cause segfault
2 participants