From a905731b235e8215a0b8df4dfae7a2673c89df75 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 16 May 2023 18:03:47 +0200 Subject: [PATCH 1/4] Fix GH-11245 (In some specific cases SWITCH with one default statement will cause segfault) The block optimizer pass allows the use of sources of the preceding block if the block is a follower and not a target. This causes issues when trying to remove FREE instructions: if the source is not in the block of the FREE, then the FREE and source are still removed. Therefore the other successor blocks, which must consume or FREE the temporary, will still contain the FREE opline. This opline will now refer to a temporary that doesn't exist anymore, which most of the time results in a crash. For these kind of non-local scenarios, we'll let the SSA based optimizations handle those cases. --- Zend/Optimizer/block_pass.c | 78 +++++++++++++++------------- ext/opcache/tests/opt/gh11245_1.phpt | 33 ++++++++++++ ext/opcache/tests/opt/gh11245_2.phpt | 35 +++++++++++++ 3 files changed, 111 insertions(+), 35 deletions(-) create mode 100644 ext/opcache/tests/opt/gh11245_1.phpt create mode 100644 ext/opcache/tests/opt/gh11245_2.phpt diff --git a/Zend/Optimizer/block_pass.c b/Zend/Optimizer/block_pass.c index 72ae01206609..9139991f59b5 100644 --- a/Zend/Optimizer/block_pass.c +++ b/Zend/Optimizer/block_pass.c @@ -257,46 +257,54 @@ static void zend_optimize_block(zend_basic_block *block, zend_op_array *op_array break; case ZEND_FREE: + src = VAR_SOURCE(opline->op1); + if (!src) { + break; + } + /* Note: Only remove the source if the source is local to this block. + * If it's not local, then the other blocks successors must also eventually either FREE or consume the temporary, + * hence removing the temporary is not safe in the general case unless specified otherwise. + * The source's result can also not be removed in such a case because that would cause a memory leak. */ if (opline->op1_type == IS_TMP_VAR) { - src = VAR_SOURCE(opline->op1); - if (src) { - switch (src->opcode) { - case ZEND_BOOL: - case ZEND_BOOL_NOT: - /* T = BOOL(X), FREE(T) => T = BOOL(X) */ - /* The remaining BOOL is removed by a separate optimization */ - VAR_SOURCE(opline->op1) = NULL; - MAKE_NOP(opline); - ++(*opt_count); - break; - case ZEND_ASSIGN: - case ZEND_ASSIGN_DIM: - case ZEND_ASSIGN_OBJ: - case ZEND_ASSIGN_STATIC_PROP: - case ZEND_ASSIGN_OP: - case ZEND_ASSIGN_DIM_OP: - case ZEND_ASSIGN_OBJ_OP: - case ZEND_ASSIGN_STATIC_PROP_OP: - case ZEND_PRE_INC: - case ZEND_PRE_DEC: - case ZEND_PRE_INC_OBJ: - case ZEND_PRE_DEC_OBJ: - case ZEND_PRE_INC_STATIC_PROP: - case ZEND_PRE_DEC_STATIC_PROP: - src->result_type = IS_UNUSED; - VAR_SOURCE(opline->op1) = NULL; - MAKE_NOP(opline); - ++(*opt_count); - break; - default: + switch (src->opcode) { + case ZEND_BOOL: + case ZEND_BOOL_NOT: + /* T = BOOL(X), FREE(T) => T = BOOL(X) */ + /* The remaining BOOL is removed by a separate optimization */ + /* The source is a bool, no source removals take place, so can be done non-locally. */ + VAR_SOURCE(opline->op1) = NULL; + MAKE_NOP(opline); + ++(*opt_count); + break; + case ZEND_ASSIGN: + case ZEND_ASSIGN_DIM: + case ZEND_ASSIGN_OBJ: + case ZEND_ASSIGN_STATIC_PROP: + case ZEND_ASSIGN_OP: + case ZEND_ASSIGN_DIM_OP: + case ZEND_ASSIGN_OBJ_OP: + case ZEND_ASSIGN_STATIC_PROP_OP: + case ZEND_PRE_INC: + case ZEND_PRE_DEC: + case ZEND_PRE_INC_OBJ: + case ZEND_PRE_DEC_OBJ: + case ZEND_PRE_INC_STATIC_PROP: + case ZEND_PRE_DEC_STATIC_PROP: + if (src < op_array->opcodes + block->start || src > op_array->opcodes + block->len) { break; - } + } + src->result_type = IS_UNUSED; + VAR_SOURCE(opline->op1) = NULL; + MAKE_NOP(opline); + ++(*opt_count); + break; + default: + break; } } else if (opline->op1_type == IS_VAR) { - src = VAR_SOURCE(opline->op1); /* V = OP, FREE(V) => OP. NOP */ - if (src && - src->opcode != ZEND_FETCH_R && + if (src >= op_array->opcodes + block->start && src <= op_array->opcodes + block->len && + src->opcode != ZEND_FETCH_R && src->opcode != ZEND_FETCH_STATIC_PROP_R && src->opcode != ZEND_FETCH_DIM_R && src->opcode != ZEND_FETCH_OBJ_R && diff --git a/ext/opcache/tests/opt/gh11245_1.phpt b/ext/opcache/tests/opt/gh11245_1.phpt new file mode 100644 index 000000000000..eac085ac4402 --- /dev/null +++ b/ext/opcache/tests/opt/gh11245_1.phpt @@ -0,0 +1,33 @@ +--TEST-- +GH-11245: In some specific cases SWITCH with one default statement will cause segfault (VAR variation) +--INI-- +opcache.enable=1 +opcache.enable_cli=1 +opcache.optimization_level=0x7FFFBFFF +opcache.opt_debug_level=0x20000 +opcache.preload= +--EXTENSIONS-- +opcache +--FILE-- + +--EXPECTF-- +$_main: + ; (lines=4, args=0, vars=1, tmps=1) + ; (after optimizer) + ; %s +0000 T1 = ISSET_ISEMPTY_CV (empty) CV0($xx) +0001 JMPNZ T1 0003 +0002 RETURN null +0003 RETURN int(1) + +xx: + ; (lines=1, args=0, vars=0, tmps=0) + ; (after optimizer) + ; %s +0000 RETURN string("somegarbage") diff --git a/ext/opcache/tests/opt/gh11245_2.phpt b/ext/opcache/tests/opt/gh11245_2.phpt new file mode 100644 index 000000000000..8e967bf9f41b --- /dev/null +++ b/ext/opcache/tests/opt/gh11245_2.phpt @@ -0,0 +1,35 @@ +--TEST-- +GH-11245: In some specific cases SWITCH with one default statement will cause segfault (TMP variation) +--INI-- +opcache.enable=1 +opcache.enable_cli=1 +opcache.optimization_level=0x7FFFBFFF +opcache.opt_debug_level=0x20000 +opcache.preload= +--EXTENSIONS-- +opcache +--FILE-- + +--EXPECTF-- +$_main: + ; (lines=7, args=0, vars=1, tmps=2) + ; (after optimizer) + ; %s +0000 T1 = PRE_INC_STATIC_PROP string("prop") string("X") +0001 T2 = ISSET_ISEMPTY_CV (empty) CV0($xx) +0002 JMPZ T2 0005 +0003 FREE T1 +0004 RETURN null +0005 FREE T1 +0006 RETURN int(1) +LIVE RANGES: + 1: 0001 - 0005 (tmp/var) From 6c10e4b16f4b6d89c95fba887fb80c1c195af8fb Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 16 May 2023 19:40:41 +0200 Subject: [PATCH 2/4] Whitespace changes --- Zend/Optimizer/block_pass.c | 76 ++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/Zend/Optimizer/block_pass.c b/Zend/Optimizer/block_pass.c index 9139991f59b5..035c98deb4e2 100644 --- a/Zend/Optimizer/block_pass.c +++ b/Zend/Optimizer/block_pass.c @@ -257,54 +257,54 @@ static void zend_optimize_block(zend_basic_block *block, zend_op_array *op_array break; case ZEND_FREE: - src = VAR_SOURCE(opline->op1); - if (!src) { - break; - } /* Note: Only remove the source if the source is local to this block. * If it's not local, then the other blocks successors must also eventually either FREE or consume the temporary, * hence removing the temporary is not safe in the general case unless specified otherwise. * The source's result can also not be removed in such a case because that would cause a memory leak. */ if (opline->op1_type == IS_TMP_VAR) { - switch (src->opcode) { - case ZEND_BOOL: - case ZEND_BOOL_NOT: - /* T = BOOL(X), FREE(T) => T = BOOL(X) */ - /* The remaining BOOL is removed by a separate optimization */ - /* The source is a bool, no source removals take place, so can be done non-locally. */ - VAR_SOURCE(opline->op1) = NULL; - MAKE_NOP(opline); - ++(*opt_count); - break; - case ZEND_ASSIGN: - case ZEND_ASSIGN_DIM: - case ZEND_ASSIGN_OBJ: - case ZEND_ASSIGN_STATIC_PROP: - case ZEND_ASSIGN_OP: - case ZEND_ASSIGN_DIM_OP: - case ZEND_ASSIGN_OBJ_OP: - case ZEND_ASSIGN_STATIC_PROP_OP: - case ZEND_PRE_INC: - case ZEND_PRE_DEC: - case ZEND_PRE_INC_OBJ: - case ZEND_PRE_DEC_OBJ: - case ZEND_PRE_INC_STATIC_PROP: - case ZEND_PRE_DEC_STATIC_PROP: - if (src < op_array->opcodes + block->start || src > op_array->opcodes + block->len) { + src = VAR_SOURCE(opline->op1); + if (src) { + switch (src->opcode) { + case ZEND_BOOL: + case ZEND_BOOL_NOT: + /* T = BOOL(X), FREE(T) => T = BOOL(X) */ + /* The remaining BOOL is removed by a separate optimization */ + /* The source is a bool, no source removals take place, so can be done non-locally. */ + VAR_SOURCE(opline->op1) = NULL; + MAKE_NOP(opline); + ++(*opt_count); break; - } - src->result_type = IS_UNUSED; - VAR_SOURCE(opline->op1) = NULL; - MAKE_NOP(opline); - ++(*opt_count); - break; - default: - break; + case ZEND_ASSIGN: + case ZEND_ASSIGN_DIM: + case ZEND_ASSIGN_OBJ: + case ZEND_ASSIGN_STATIC_PROP: + case ZEND_ASSIGN_OP: + case ZEND_ASSIGN_DIM_OP: + case ZEND_ASSIGN_OBJ_OP: + case ZEND_ASSIGN_STATIC_PROP_OP: + case ZEND_PRE_INC: + case ZEND_PRE_DEC: + case ZEND_PRE_INC_OBJ: + case ZEND_PRE_DEC_OBJ: + case ZEND_PRE_INC_STATIC_PROP: + case ZEND_PRE_DEC_STATIC_PROP: + if (src < op_array->opcodes + block->start || src > op_array->opcodes + block->len) { + break; + } + src->result_type = IS_UNUSED; + VAR_SOURCE(opline->op1) = NULL; + MAKE_NOP(opline); + ++(*opt_count); + break; + default: + break; + } } } else if (opline->op1_type == IS_VAR) { + src = VAR_SOURCE(opline->op1); /* V = OP, FREE(V) => OP. NOP */ if (src >= op_array->opcodes + block->start && src <= op_array->opcodes + block->len && - src->opcode != ZEND_FETCH_R && + src->opcode != ZEND_FETCH_R && src->opcode != ZEND_FETCH_STATIC_PROP_R && src->opcode != ZEND_FETCH_DIM_R && src->opcode != ZEND_FETCH_OBJ_R && From cbaa8e42a84f9c325bd0ab6fb1991ee019fb2885 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 20 May 2023 01:36:25 +0200 Subject: [PATCH 3/4] Clarify comments --- Zend/Optimizer/block_pass.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Zend/Optimizer/block_pass.c b/Zend/Optimizer/block_pass.c index 035c98deb4e2..283fc21614d1 100644 --- a/Zend/Optimizer/block_pass.c +++ b/Zend/Optimizer/block_pass.c @@ -259,8 +259,8 @@ static void zend_optimize_block(zend_basic_block *block, zend_op_array *op_array case ZEND_FREE: /* Note: Only remove the source if the source is local to this block. * If it's not local, then the other blocks successors must also eventually either FREE or consume the temporary, - * hence removing the temporary is not safe in the general case unless specified otherwise. - * The source's result can also not be removed in such a case because that would cause a memory leak. */ + * hence removing the temporary is not safe in the general case, especially when other consumers are not FREE. + * A FREE may not be removed without also removing the source's result, because otherwise that would cause a memory leak. */ if (opline->op1_type == IS_TMP_VAR) { src = VAR_SOURCE(opline->op1); if (src) { @@ -269,7 +269,7 @@ static void zend_optimize_block(zend_basic_block *block, zend_op_array *op_array case ZEND_BOOL_NOT: /* T = BOOL(X), FREE(T) => T = BOOL(X) */ /* The remaining BOOL is removed by a separate optimization */ - /* The source is a bool, no source removals take place, so can be done non-locally. */ + /* The source is a bool, no source removals take place, so this may be done non-locally. */ VAR_SOURCE(opline->op1) = NULL; MAKE_NOP(opline); ++(*opt_count); From d60523146a386653a5ac0ff6fb824cbf6afafc32 Mon Sep 17 00:00:00 2001 From: nielsdos <7771979+nielsdos@users.noreply.github.com> Date: Tue, 23 May 2023 00:17:25 +0200 Subject: [PATCH 4/4] Remove useless condition --- Zend/Optimizer/block_pass.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Zend/Optimizer/block_pass.c b/Zend/Optimizer/block_pass.c index 283fc21614d1..b17fcde4bdf8 100644 --- a/Zend/Optimizer/block_pass.c +++ b/Zend/Optimizer/block_pass.c @@ -288,7 +288,7 @@ static void zend_optimize_block(zend_basic_block *block, zend_op_array *op_array case ZEND_PRE_DEC_OBJ: case ZEND_PRE_INC_STATIC_PROP: case ZEND_PRE_DEC_STATIC_PROP: - if (src < op_array->opcodes + block->start || src > op_array->opcodes + block->len) { + if (src < op_array->opcodes + block->start) { break; } src->result_type = IS_UNUSED; @@ -303,7 +303,7 @@ static void zend_optimize_block(zend_basic_block *block, zend_op_array *op_array } else if (opline->op1_type == IS_VAR) { src = VAR_SOURCE(opline->op1); /* V = OP, FREE(V) => OP. NOP */ - if (src >= op_array->opcodes + block->start && src <= op_array->opcodes + block->len && + if (src >= op_array->opcodes + block->start && src->opcode != ZEND_FETCH_R && src->opcode != ZEND_FETCH_STATIC_PROP_R && src->opcode != ZEND_FETCH_DIM_R &&