Skip to content

Commit f0600f5

Browse files
committed
Fix GH-17246: Nested shm protections cause segfault
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 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.
1 parent fcbfd5a commit f0600f5

File tree

3 files changed

+50
-2
lines changed

3 files changed

+50
-2
lines changed

ext/opcache/ZendAccelerator.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,7 @@ typedef struct _zend_accel_globals {
210210
int cwd_key_len;
211211
bool cwd_check;
212212
int auto_globals_mask;
213+
uint32_t shm_nesting_level;
213214
time_t request_time;
214215
time_t last_restart_time; /* used to synchronize SHM and in-process caches */
215216
HashTable xlat_table;
@@ -332,14 +333,14 @@ END_EXTERN_C()
332333
/* memory write protection */
333334
#define SHM_PROTECT() \
334335
do { \
335-
if (ZCG(accel_directives).protect_memory) { \
336+
if (ZCG(accel_directives).protect_memory && ZCG(shm_nesting_level)++ == 0) { \
336337
zend_accel_shared_protect(true); \
337338
} \
338339
} while (0)
339340

340341
#define SHM_UNPROTECT() \
341342
do { \
342-
if (ZCG(accel_directives).protect_memory) { \
343+
if (ZCG(accel_directives).protect_memory && --ZCG(shm_nesting_level) == 0) { \
343344
zend_accel_shared_protect(false); \
344345
} \
345346
} while (0)

ext/opcache/tests/jit/gh17246.inc

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?php
2+
3+
// Need to cause a trace exit, so extend non existent class
4+
class MyXSLTProcessor extends NonExistentClass {
5+
public function registerCycle() {
6+
[[$this]]; // Non trivial array
7+
}
8+
}

ext/opcache/tests/jit/gh17246.phpt

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
--TEST--
2+
GH-17246 (Nested shm protections cause segfault)
3+
--EXTENSIONS--
4+
opcache
5+
--INI--
6+
opcache.protect_memory=1
7+
opcache.jit_buffer_size=32M
8+
opcache.jit=1254
9+
--FILE--
10+
<?php
11+
12+
class Test
13+
{
14+
private $field;
15+
16+
public function __construct()
17+
{
18+
$this->field = function() {};
19+
}
20+
21+
public function __destruct()
22+
{
23+
// Necessary because we need to invoke tracing JIT during destruction
24+
}
25+
}
26+
27+
for ($i = 0; $i < 10000; ++$i) {
28+
$obj = new Test();
29+
}
30+
31+
require __DIR__.'/gh17246.inc';
32+
33+
?>
34+
--EXPECTF--
35+
Fatal error: Uncaught Error: Class "NonExistentClass" not found in %s:%d
36+
Stack trace:
37+
#0 %s(%d): require()
38+
#1 {main}
39+
thrown in %s on line %d

0 commit comments

Comments
 (0)