Skip to content

Commit 6c25413

Browse files
dstogovdstogov
committed
Add JIT guards for INIT_FCALL instructions and functions that may be modified
For methods we reuse mechanism of polymorphic calls. For regular function we invalidate the whole root trace. This fixes #8461
1 parent 84c1e99 commit 6c25413

19 files changed

+543
-12
lines changed

‎ext/opcache/jit/zend_jit_arm64.dasc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8811,7 +8811,8 @@ static int zend_jit_init_fcall(dasm_State **Dst, const zend_op *opline, uint32_t
88118811
| // Get the return value of function zend_jit_find_func_helper/zend_jit_find_ns_func_helper
88128812
| mov REG0, RETVALx
88138813
if (JIT_G(trigger) == ZEND_JIT_ON_HOT_TRACE) {
8814-
int32_t exit_point = zend_jit_trace_get_exit_point(opline, 0);
8814+
int32_t exit_point = zend_jit_trace_get_exit_point(opline,
8815+
func && (func->common.fn_flags & ZEND_ACC_IMMUTABLE) ? ZEND_JIT_EXIT_INVALIDATE : 0);
88158816
const void *exit_addr = zend_jit_trace_get_exit_addr(exit_point);
88168817

88178818
if (!exit_addr) {

‎ext/opcache/jit/zend_jit_internal.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,7 @@ typedef enum _zend_jit_trace_stop {
412412
#define ZEND_JIT_EXIT_PACKED_GUARD (1<<7)
413413
#define ZEND_JIT_EXIT_CLOSURE_CALL (1<<8) /* exit because of polymorphic INIT_DYNAMIC_CALL call */
414414
#define ZEND_JIT_EXIT_METHOD_CALL (1<<9) /* exit because of polymorphic INIT_METHOD_CALL call */
415+
#define ZEND_JIT_EXIT_INVALIDATE (1<<10) /* invalidate current trace */
415416

416417
typedef union _zend_op_trace_info {
417418
zend_op dummy; /* the size of this structure must be the same as zend_op */
@@ -584,6 +585,7 @@ typedef struct _zend_jit_trace_info {
584585
uint32_t flags; /* See ZEND_JIT_TRACE_... defines above */
585586
uint32_t polymorphism; /* Counter of polymorphic calls */
586587
uint32_t jmp_table_size;/* number of jmp_table slots */
588+
const zend_op_array *op_array; /* function */
587589
const zend_op *opline; /* first opline */
588590
const void *code_start; /* address of native code */
589591
zend_jit_trace_exit_info *exit_info; /* info about side exits */

‎ext/opcache/jit/zend_jit_trace.c

Lines changed: 62 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,26 @@ static int zend_jit_trace_may_exit(const zend_op_array *op_array, const zend_op
366366
return 0;
367367
}
368368

369+
static bool zend_jit_may_be_modified(const zend_function *func, const zend_op_array *called_from)
370+
{
371+
if (func->type == ZEND_INTERNAL_FUNCTION) {
372+
#ifdef _WIN32
373+
/* ASLR */
374+
return 1;
375+
#else
376+
return 0;
377+
#endif
378+
} else if (func->type == ZEND_USER_FUNCTION) {
379+
if (func->common.fn_flags & ZEND_ACC_PRELOADED) {
380+
return 0;
381+
}
382+
if (func->op_array.filename == called_from->filename && !func->op_array.scope) {
383+
return 0;
384+
}
385+
}
386+
return 1;
387+
}
388+
369389
static zend_always_inline uint32_t zend_jit_trace_type_to_info_ex(zend_uchar type, uint32_t info)
370390
{
371391
if (type == IS_UNKNOWN) {
@@ -6081,10 +6101,11 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
60816101
if (!zend_jit_trace_handler(&dasm_state, op_array, opline, zend_may_throw(opline, ssa_op, op_array, ssa), p + 1)) {
60826102
goto jit_failure;
60836103
}
6084-
if ((opline->opcode != ZEND_INIT_STATIC_METHOD_CALL
6104+
if ((p+1)->op == ZEND_JIT_TRACE_INIT_CALL && (p+1)->func
6105+
&& (opline->opcode != ZEND_INIT_STATIC_METHOD_CALL
60856106
|| opline->op1_type != IS_CONST
6086-
|| opline->op2_type != IS_CONST)
6087-
&& (p+1)->op == ZEND_JIT_TRACE_INIT_CALL && (p+1)->func) {
6107+
|| opline->op2_type != IS_CONST
6108+
|| zend_jit_may_be_modified((p+1)->func, op_array))) {
60886109
if (!zend_jit_init_fcall_guard(&dasm_state, 0, (p+1)->func, opline+1)) {
60896110
goto jit_failure;
60906111
}
@@ -6094,8 +6115,9 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
60946115
if (!zend_jit_trace_handler(&dasm_state, op_array, opline, zend_may_throw(opline, ssa_op, op_array, ssa), p + 1)) {
60956116
goto jit_failure;
60966117
}
6097-
if (opline->op2_type != IS_CONST
6098-
&& (p+1)->op == ZEND_JIT_TRACE_INIT_CALL && (p+1)->func) {
6118+
if ((p+1)->op == ZEND_JIT_TRACE_INIT_CALL && (p+1)->func
6119+
&& (opline->op2_type != IS_CONST
6120+
|| zend_jit_may_be_modified((p+1)->func, op_array))) {
60996121
if (!zend_jit_init_fcall_guard(&dasm_state, 0, (p+1)->func, opline+1)) {
61006122
goto jit_failure;
61016123
}
@@ -6105,8 +6127,9 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
61056127
if (!zend_jit_trace_handler(&dasm_state, op_array, opline, zend_may_throw(opline, ssa_op, op_array, ssa), p + 1)) {
61066128
goto jit_failure;
61076129
}
6108-
if (opline->op1_type != IS_CONST
6109-
&& (p+1)->op == ZEND_JIT_TRACE_INIT_CALL && (p+1)->func) {
6130+
if ((p+1)->op == ZEND_JIT_TRACE_INIT_CALL && (p+1)->func
6131+
&& (opline->op1_type != IS_CONST
6132+
|| zend_jit_may_be_modified((p+1)->func, op_array))) {
61106133
SET_STACK_TYPE(stack, EX_VAR_TO_NUM(opline->result.var), IS_OBJECT, 1);
61116134
if (!zend_jit_init_fcall_guard(&dasm_state, 0, (p+1)->func, opline+1)) {
61126135
goto jit_failure;
@@ -6672,8 +6695,8 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
66726695
call_info = call_info->next_callee;
66736696
}
66746697
if (!skip_guard
6675-
&& !zend_jit_may_be_polymorphic_call(init_opline)) {
6676-
// TODO: recompilation may change target ???
6698+
&& !zend_jit_may_be_polymorphic_call(init_opline)
6699+
&& !zend_jit_may_be_modified(p->func, op_array)) {
66776700
skip_guard = 1;
66786701
}
66796702
}
@@ -7006,6 +7029,7 @@ static zend_jit_trace_stop zend_jit_compile_root_trace(zend_jit_trace_rec *trace
70067029
t->flags = 0;
70077030
t->polymorphism = 0;
70087031
t->jmp_table_size = 0;
7032+
t->op_array = trace_buffer[0].op_array;
70097033
t->opline = trace_buffer[1].opline;
70107034
t->exit_info = exit_info;
70117035
t->stack_map = NULL;
@@ -8034,6 +8058,35 @@ int ZEND_FASTCALL zend_jit_trace_exit(uint32_t exit_num, zend_jit_registers_buf
80348058
EX(opline)->lineno);
80358059
}
80368060

8061+
if (t->exit_info[exit_num].flags & ZEND_JIT_EXIT_INVALIDATE) {
8062+
zend_jit_op_array_trace_extension *jit_extension;
8063+
uint32_t num = trace_num;
8064+
8065+
while (t->root != num) {
8066+
num = t->root;
8067+
t = &zend_jit_traces[num];
8068+
}
8069+
8070+
SHM_UNPROTECT();
8071+
zend_jit_unprotect();
8072+
8073+
jit_extension = (zend_jit_op_array_trace_extension*)ZEND_FUNC_INFO(t->op_array);
8074+
if (ZEND_OP_TRACE_INFO(t->opline, jit_extension->offset)->trace_flags & ZEND_JIT_TRACE_START_LOOP) {
8075+
((zend_op*)(t->opline))->handler = (const void*)zend_jit_loop_trace_counter_handler;
8076+
} else if (ZEND_OP_TRACE_INFO(t->opline, jit_extension->offset)->trace_flags & ZEND_JIT_TRACE_START_ENTER) {
8077+
((zend_op*)(t->opline))->handler = (const void*)zend_jit_func_trace_counter_handler;
8078+
} else if (ZEND_OP_TRACE_INFO(t->opline, jit_extension->offset)->trace_flags & ZEND_JIT_TRACE_START_RETURN) {
8079+
((zend_op*)(t->opline))->handler = (const void*)zend_jit_ret_trace_counter_handler;
8080+
}
8081+
ZEND_OP_TRACE_INFO(t->opline, jit_extension->offset)->trace_flags &=
8082+
ZEND_JIT_TRACE_START_LOOP|ZEND_JIT_TRACE_START_ENTER|ZEND_JIT_TRACE_START_RETURN;
8083+
8084+
zend_jit_protect();
8085+
SHM_PROTECT();
8086+
8087+
return 0;
8088+
}
8089+
80378090
if (t->exit_info[exit_num].flags & ZEND_JIT_EXIT_TO_VM) {
80388091
if (zend_jit_trace_exit_is_bad(trace_num, exit_num)) {
80398092
zend_jit_blacklist_trace_exit(trace_num, exit_num);

‎ext/opcache/jit/zend_jit_x86.dasc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9422,13 +9422,13 @@ static int zend_jit_init_fcall(dasm_State **Dst, const zend_op *opline, uint32_t
94229422
ZEND_UNREACHABLE();
94239423
}
94249424
if (JIT_G(trigger) == ZEND_JIT_ON_HOT_TRACE) {
9425-
int32_t exit_point = zend_jit_trace_get_exit_point(opline, 0);
9425+
int32_t exit_point = zend_jit_trace_get_exit_point(opline,
9426+
func && (func->common.fn_flags & ZEND_ACC_IMMUTABLE) ? ZEND_JIT_EXIT_INVALIDATE : 0);
94269427
const void *exit_addr = zend_jit_trace_get_exit_addr(exit_point);
94279428

94289429
if (!exit_addr) {
94299430
return 0;
94309431
}
9431-
94329432
if (!func || opline->opcode == ZEND_INIT_FCALL) {
94339433
| test r0, r0
94349434
| jnz >3

‎ext/opcache/tests/jit/gh8461-001.inc

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<?php
2+
3+
$x = 0;
4+
5+
class UniqueList
6+
{
7+
const A = 1;
8+
9+
public static function foo()
10+
{
11+
global $x;
12+
$x += self::A;
13+
}
14+
}

‎ext/opcache/tests/jit/gh8461-001.phpt

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
--TEST--
2+
Bug GH-8461 001 (JIT does not account for class re-compile)
3+
--EXTENSIONS--
4+
opcache
5+
--INI--
6+
opcache.enable=1
7+
opcache.enable_cli=1
8+
opcache.jit_buffer_size=1M
9+
opcache.jit=1255
10+
opcache.file_update_protection=0
11+
opcache.revalidate_freq=0
12+
opcache.protect_memory=1
13+
--FILE--
14+
<?php
15+
16+
// Checks that JITed code does not crash in --repeat 2 after the UniqueList
17+
// class is recompiled.
18+
19+
require __DIR__ . '/gh8461-001.inc';
20+
21+
class UniqueListLast extends UniqueList
22+
{
23+
public static function bar() {
24+
parent::foo();
25+
}
26+
}
27+
28+
for ($i = 0; $i < 10; $i++) {
29+
UniqueListLast::bar();
30+
}
31+
32+
// mark the file as changed (important)
33+
touch(__DIR__ . '/gh8461-001.inc');
34+
35+
print "OK";
36+
--EXPECT--
37+
OK

‎ext/opcache/tests/jit/gh8461-002.inc

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<?php
2+
3+
$x = 0;
4+
5+
class UniqueList
6+
{
7+
const A = 1;
8+
9+
public static function foo()
10+
{
11+
global $x;
12+
$x += self::A;
13+
}
14+
}

‎ext/opcache/tests/jit/gh8461-002.phpt

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
--TEST--
2+
Bug GH-8461 002 (JIT does not account for class re-compile)
3+
--EXTENSIONS--
4+
opcache
5+
--INI--
6+
opcache.enable=1
7+
opcache.enable_cli=1
8+
opcache.jit_buffer_size=1M
9+
opcache.jit=1255
10+
opcache.file_update_protection=0
11+
opcache.revalidate_freq=0
12+
opcache.protect_memory=1
13+
--FILE--
14+
<?php
15+
16+
// Checks that JITed code does not crash in --repeat 2 after the UniqueList
17+
// class is recompiled.
18+
19+
require __DIR__ . '/gh8461-002.inc';
20+
21+
for ($i = 0; $i < 10; $i++) {
22+
UniqueList::foo();
23+
}
24+
25+
// mark the file as changed (important)
26+
touch(__DIR__ . '/gh8461-002.inc');
27+
28+
print "OK";
29+
--EXPECT--
30+
OK

‎ext/opcache/tests/jit/gh8461-003.inc

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
<?php
2+
3+
$x = 0;
4+
5+
class UniqueList
6+
{
7+
public const A = 1;
8+
public const B = 1;
9+
10+
private $foo;
11+
12+
public function __construct($b)
13+
{
14+
global $x;
15+
$x++;
16+
17+
$this->foo = self::A + $b;
18+
}
19+
}

‎ext/opcache/tests/jit/gh8461-003.phpt

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
--TEST--
2+
Bug GH-8461 003 (JIT does not account for class re-compile)
3+
--EXTENSIONS--
4+
opcache
5+
--INI--
6+
opcache.enable=1
7+
opcache.enable_cli=1
8+
opcache.jit_buffer_size=1M
9+
opcache.jit=1255
10+
opcache.file_update_protection=0
11+
opcache.revalidate_freq=0
12+
opcache.protect_memory=1
13+
--FILE--
14+
<?php
15+
16+
// Checks that JITed code does not crash in --repeat 2 after the UniqueList
17+
// class is recompiled.
18+
19+
require __DIR__ . '/gh8461-003.inc';
20+
21+
class UniqueListLast extends UniqueList
22+
{
23+
public function __construct()
24+
{
25+
parent::__construct(self::B);
26+
}
27+
}
28+
29+
for ($i = 0; $i < 10; $i++) {
30+
new UniqueListLast();
31+
}
32+
33+
// mark the file as changed (important)
34+
touch(__DIR__ . '/gh8461-003.inc');
35+
36+
print "OK";
37+
--EXPECT--
38+
OK

0 commit comments

Comments
 (0)