From b2e7b73b0a213c5db8083d597da7825830945602 Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Sat, 20 Apr 2024 18:00:42 +0200 Subject: [PATCH 1/2] Fix GH-13817: Segmentation fault for enabled observers after pass 4 Instead of fixing up temporaries count in between observer steps, just take ZEND_ACC_DONE_PASS_TWO into account during stack_size calculation. Introducing zend_vm_calc_ct_used_stack for that use case. This should be much less susceptible to forgetting to handle the ZEND_OBSERVER_ENABLED temporary explicitly. --- NEWS | 2 ++ Zend/Optimizer/optimize_func_calls.c | 4 +-- Zend/Optimizer/zend_optimizer.c | 14 +++----- Zend/zend_execute.c | 5 +++ Zend/zend_execute.h | 3 ++ ext/opcache/tests/gh13817.phpt | 51 ++++++++++++++++++++++++++++ 6 files changed, 67 insertions(+), 12 deletions(-) create mode 100644 ext/opcache/tests/gh13817.phpt diff --git a/NEWS b/NEWS index aaf02438081a1..5453ec0d5a2e0 100644 --- a/NEWS +++ b/NEWS @@ -142,6 +142,8 @@ PHP NEWS - Opcache: . Fixed incorrect assumptions across compilation units for static calls. (ilutov) + . Fixed bug GH-13817 (Segmentation fault for enabled observers after pass 4). + (Bob) - OpenSSL: . Fixed bug GH-10495 (feof on OpenSSL stream hangs indefinitely). diff --git a/Zend/Optimizer/optimize_func_calls.c b/Zend/Optimizer/optimize_func_calls.c index bcddacc4e43fc..4ee582f44d6ac 100644 --- a/Zend/Optimizer/optimize_func_calls.c +++ b/Zend/Optimizer/optimize_func_calls.c @@ -195,7 +195,7 @@ void zend_optimize_func_calls(zend_op_array *op_array, zend_optimizer_ctx *ctx) /* nothing to do */ } else if (fcall->opcode == ZEND_INIT_FCALL_BY_NAME) { fcall->opcode = ZEND_INIT_FCALL; - fcall->op1.num = zend_vm_calc_used_stack(fcall->extended_value, call_stack[call].func); + fcall->op1.num = zend_vm_calc_ct_used_stack(fcall->extended_value, call_stack[call].func); literal_dtor(&ZEND_OP2_LITERAL(fcall)); fcall->op2.constant = fcall->op2.constant + 1; if (opline->opcode != ZEND_CALLABLE_CONVERT) { @@ -203,7 +203,7 @@ void zend_optimize_func_calls(zend_op_array *op_array, zend_optimizer_ctx *ctx) } } else if (fcall->opcode == ZEND_INIT_NS_FCALL_BY_NAME) { fcall->opcode = ZEND_INIT_FCALL; - fcall->op1.num = zend_vm_calc_used_stack(fcall->extended_value, call_stack[call].func); + fcall->op1.num = zend_vm_calc_ct_used_stack(fcall->extended_value, call_stack[call].func); literal_dtor(&op_array->literals[fcall->op2.constant]); literal_dtor(&op_array->literals[fcall->op2.constant + 2]); fcall->op2.constant = fcall->op2.constant + 1; diff --git a/Zend/Optimizer/zend_optimizer.c b/Zend/Optimizer/zend_optimizer.c index f274edb039c17..346ba078e6195 100644 --- a/Zend/Optimizer/zend_optimizer.c +++ b/Zend/Optimizer/zend_optimizer.c @@ -1238,6 +1238,8 @@ static void zend_redo_pass_two_ex(zend_op_array *op_array, zend_ssa *ssa) } #endif + op_array->T += ZEND_OBSERVER_ENABLED; // reserve last temporary for observers if enabled + opline = op_array->opcodes; end = opline + op_array->last; while (opline < end) { @@ -1362,7 +1364,7 @@ static void zend_adjust_fcall_stack_size(zend_op_array *op_array, zend_optimizer &ctx->script->function_table, Z_STR_P(RT_CONSTANT(opline, opline->op2))); if (func) { - opline->op1.num = zend_vm_calc_used_stack(opline->extended_value, func); + opline->op1.num = zend_vm_calc_ct_used_stack(opline->extended_value, func); } } opline++; @@ -1381,7 +1383,7 @@ static void zend_adjust_fcall_stack_size_graph(zend_op_array *op_array) if (opline && call_info->callee_func && opline->opcode == ZEND_INIT_FCALL) { ZEND_ASSERT(!call_info->is_prototype); - opline->op1.num = zend_vm_calc_used_stack(opline->extended_value, call_info->callee_func); + opline->op1.num = zend_vm_calc_ct_used_stack(opline->extended_value, call_info->callee_func); } call_info = call_info->next_callee; } @@ -1557,12 +1559,6 @@ ZEND_API void zend_optimize_script(zend_script *script, zend_long optimization_l } } - if (ZEND_OBSERVER_ENABLED) { - for (i = 0; i < call_graph.op_arrays_count; i++) { - ++call_graph.op_arrays[i]->T; // ensure accurate temporary count for stack size precalculation - } - } - if (ZEND_OPTIMIZER_PASS_12 & optimization_level) { for (i = 0; i < call_graph.op_arrays_count; i++) { zend_adjust_fcall_stack_size_graph(call_graph.op_arrays[i]); @@ -1578,8 +1574,6 @@ ZEND_API void zend_optimize_script(zend_script *script, zend_long optimization_l zend_recalc_live_ranges(op_array, needs_live_range); } } else { - op_array->T -= ZEND_OBSERVER_ENABLED; // redo_pass_two will re-increment it - zend_redo_pass_two(op_array); if (op_array->live_range) { zend_recalc_live_ranges(op_array, NULL); diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index 5a060540db015..898886f1cbefb 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -226,6 +226,11 @@ ZEND_API void* zend_vm_stack_extend(size_t size) return ptr; } +ZEND_API uint32_t zend_vm_calc_ct_used_stack(uint32_t num_args, zend_function *func) +{ + return zend_vm_calc_used_stack(num_args, func) + ((func->common.fn_flags & ZEND_ACC_DONE_PASS_TWO) == 0 && ZEND_USER_CODE(func->type) ? ZEND_OBSERVER_ENABLED : 0) * sizeof(zval); +} + ZEND_API zval* zend_get_compiled_variable_value(const zend_execute_data *execute_data, uint32_t var) { return EX_VAR(var); diff --git a/Zend/zend_execute.h b/Zend/zend_execute.h index 2a61f4bff711e..da08bad1d29ae 100644 --- a/Zend/zend_execute.h +++ b/Zend/zend_execute.h @@ -255,6 +255,9 @@ static zend_always_inline uint32_t zend_vm_calc_used_stack(uint32_t num_args, ze return used_stack * sizeof(zval); } +// Handle a possibly currently not applied pass_two +ZEND_API uint32_t zend_vm_calc_ct_used_stack(uint32_t num_args, zend_function *func); + static zend_always_inline zend_execute_data *zend_vm_stack_push_call_frame(uint32_t call_info, zend_function *func, uint32_t num_args, void *object_or_called_scope) { uint32_t used_stack = zend_vm_calc_used_stack(num_args, func); diff --git a/ext/opcache/tests/gh13817.phpt b/ext/opcache/tests/gh13817.phpt new file mode 100644 index 0000000000000..0e5eb560d46f3 --- /dev/null +++ b/ext/opcache/tests/gh13817.phpt @@ -0,0 +1,51 @@ +--TEST-- +GH-13712 (Segmentation fault for enabled observers after pass 4) +--EXTENSIONS-- +opcache +zend_test +--INI-- +zend_test.observer.enabled=1 +zend_test.observer.show_output=1 +zend_test.observer.observe_all=1 +opcache.enable=1 +opcache.enable_cli=1 +opcache.optimization_level=0x4069 +--FILE-- + +--EXPECTF-- + + + + + + + + +Ok + + + + + From c1f8f84469218ad13966b8f69106232de22f26f7 Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Sun, 2 Jun 2024 00:12:30 +0200 Subject: [PATCH 2/2] Apply obvious simplification --- NEWS | 4 ++-- Zend/Optimizer/compact_vars.c | 3 ++- Zend/Optimizer/optimize_func_calls.c | 4 ++-- Zend/Optimizer/optimize_temp_vars_5.c | 3 ++- Zend/Optimizer/zend_optimizer.c | 11 ++--------- Zend/zend_execute.c | 5 ----- Zend/zend_execute.h | 3 --- 7 files changed, 10 insertions(+), 23 deletions(-) diff --git a/NEWS b/NEWS index 5453ec0d5a2e0..072445c5efb58 100644 --- a/NEWS +++ b/NEWS @@ -30,6 +30,8 @@ PHP NEWS . Fixed bug GH-14267 (opcache.jit=off does not allow enabling JIT at runtime). (ilutov) . Fixed TLS access in JIT on FreeBSD/amd64. (Arnaud) + . Fixed bug GH-13817 (Segmentation fault for enabled observers after pass 4). + (Bob) - Soap: . Fixed bug #47925 (PHPClient can't decompress response). (nielsdos) @@ -142,8 +144,6 @@ PHP NEWS - Opcache: . Fixed incorrect assumptions across compilation units for static calls. (ilutov) - . Fixed bug GH-13817 (Segmentation fault for enabled observers after pass 4). - (Bob) - OpenSSL: . Fixed bug GH-10495 (feof on OpenSSL stream hangs indefinitely). diff --git a/Zend/Optimizer/compact_vars.c b/Zend/Optimizer/compact_vars.c index a8ef8846deca2..9898714a17c5e 100644 --- a/Zend/Optimizer/compact_vars.c +++ b/Zend/Optimizer/compact_vars.c @@ -18,6 +18,7 @@ #include "Optimizer/zend_optimizer_internal.h" #include "zend_bitset.h" +#include "zend_observer.h" /* This pass removes all CVs and temporaries that are completely unused. It does *not* merge any CVs or TMPs. * This pass does not operate on SSA form anymore. */ @@ -117,7 +118,7 @@ void zend_optimizer_compact_vars(zend_op_array *op_array) { op_array->last_var = num_cvs; } - op_array->T = num_tmps; + op_array->T = num_tmps + ZEND_OBSERVER_ENABLED; // reserve last temporary for observers if enabled free_alloca(vars_map, use_heap2); } diff --git a/Zend/Optimizer/optimize_func_calls.c b/Zend/Optimizer/optimize_func_calls.c index 4ee582f44d6ac..bcddacc4e43fc 100644 --- a/Zend/Optimizer/optimize_func_calls.c +++ b/Zend/Optimizer/optimize_func_calls.c @@ -195,7 +195,7 @@ void zend_optimize_func_calls(zend_op_array *op_array, zend_optimizer_ctx *ctx) /* nothing to do */ } else if (fcall->opcode == ZEND_INIT_FCALL_BY_NAME) { fcall->opcode = ZEND_INIT_FCALL; - fcall->op1.num = zend_vm_calc_ct_used_stack(fcall->extended_value, call_stack[call].func); + fcall->op1.num = zend_vm_calc_used_stack(fcall->extended_value, call_stack[call].func); literal_dtor(&ZEND_OP2_LITERAL(fcall)); fcall->op2.constant = fcall->op2.constant + 1; if (opline->opcode != ZEND_CALLABLE_CONVERT) { @@ -203,7 +203,7 @@ void zend_optimize_func_calls(zend_op_array *op_array, zend_optimizer_ctx *ctx) } } else if (fcall->opcode == ZEND_INIT_NS_FCALL_BY_NAME) { fcall->opcode = ZEND_INIT_FCALL; - fcall->op1.num = zend_vm_calc_ct_used_stack(fcall->extended_value, call_stack[call].func); + fcall->op1.num = zend_vm_calc_used_stack(fcall->extended_value, call_stack[call].func); literal_dtor(&op_array->literals[fcall->op2.constant]); literal_dtor(&op_array->literals[fcall->op2.constant + 2]); fcall->op2.constant = fcall->op2.constant + 1; diff --git a/Zend/Optimizer/optimize_temp_vars_5.c b/Zend/Optimizer/optimize_temp_vars_5.c index 33d418ffbd99c..de0b189d5dca1 100644 --- a/Zend/Optimizer/optimize_temp_vars_5.c +++ b/Zend/Optimizer/optimize_temp_vars_5.c @@ -26,6 +26,7 @@ #include "zend_execute.h" #include "zend_vm.h" #include "zend_bitset.h" +#include "zend_observer.h" #define INVALID_VAR ((uint32_t)-1) #define GET_AVAILABLE_T() \ @@ -173,5 +174,5 @@ void zend_optimize_temporary_variables(zend_op_array *op_array, zend_optimizer_c } zend_arena_release(&ctx->arena, checkpoint); - op_array->T = max + 1; + op_array->T = max + 1 + ZEND_OBSERVER_ENABLED; // reserve last temporary for observers if enabled } diff --git a/Zend/Optimizer/zend_optimizer.c b/Zend/Optimizer/zend_optimizer.c index 346ba078e6195..8a029c8007b85 100644 --- a/Zend/Optimizer/zend_optimizer.c +++ b/Zend/Optimizer/zend_optimizer.c @@ -31,7 +31,6 @@ #include "zend_inference.h" #include "zend_dump.h" #include "php.h" -#include "zend_observer.h" #ifndef ZEND_OPTIMIZER_MAX_REGISTERED_PASSES # define ZEND_OPTIMIZER_MAX_REGISTERED_PASSES 32 @@ -1097,8 +1096,6 @@ static void zend_revert_pass_two(zend_op_array *op_array) } #endif - op_array->T -= ZEND_OBSERVER_ENABLED; - op_array->fn_flags &= ~ZEND_ACC_DONE_PASS_TWO; } @@ -1128,8 +1125,6 @@ static void zend_redo_pass_two(zend_op_array *op_array) } #endif - op_array->T += ZEND_OBSERVER_ENABLED; // reserve last temporary for observers if enabled - opline = op_array->opcodes; end = opline + op_array->last; while (opline < end) { @@ -1238,8 +1233,6 @@ static void zend_redo_pass_two_ex(zend_op_array *op_array, zend_ssa *ssa) } #endif - op_array->T += ZEND_OBSERVER_ENABLED; // reserve last temporary for observers if enabled - opline = op_array->opcodes; end = opline + op_array->last; while (opline < end) { @@ -1364,7 +1357,7 @@ static void zend_adjust_fcall_stack_size(zend_op_array *op_array, zend_optimizer &ctx->script->function_table, Z_STR_P(RT_CONSTANT(opline, opline->op2))); if (func) { - opline->op1.num = zend_vm_calc_ct_used_stack(opline->extended_value, func); + opline->op1.num = zend_vm_calc_used_stack(opline->extended_value, func); } } opline++; @@ -1383,7 +1376,7 @@ static void zend_adjust_fcall_stack_size_graph(zend_op_array *op_array) if (opline && call_info->callee_func && opline->opcode == ZEND_INIT_FCALL) { ZEND_ASSERT(!call_info->is_prototype); - opline->op1.num = zend_vm_calc_ct_used_stack(opline->extended_value, call_info->callee_func); + opline->op1.num = zend_vm_calc_used_stack(opline->extended_value, call_info->callee_func); } call_info = call_info->next_callee; } diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index 898886f1cbefb..5a060540db015 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -226,11 +226,6 @@ ZEND_API void* zend_vm_stack_extend(size_t size) return ptr; } -ZEND_API uint32_t zend_vm_calc_ct_used_stack(uint32_t num_args, zend_function *func) -{ - return zend_vm_calc_used_stack(num_args, func) + ((func->common.fn_flags & ZEND_ACC_DONE_PASS_TWO) == 0 && ZEND_USER_CODE(func->type) ? ZEND_OBSERVER_ENABLED : 0) * sizeof(zval); -} - ZEND_API zval* zend_get_compiled_variable_value(const zend_execute_data *execute_data, uint32_t var) { return EX_VAR(var); diff --git a/Zend/zend_execute.h b/Zend/zend_execute.h index da08bad1d29ae..2a61f4bff711e 100644 --- a/Zend/zend_execute.h +++ b/Zend/zend_execute.h @@ -255,9 +255,6 @@ static zend_always_inline uint32_t zend_vm_calc_used_stack(uint32_t num_args, ze return used_stack * sizeof(zval); } -// Handle a possibly currently not applied pass_two -ZEND_API uint32_t zend_vm_calc_ct_used_stack(uint32_t num_args, zend_function *func); - static zend_always_inline zend_execute_data *zend_vm_stack_push_call_frame(uint32_t call_info, zend_function *func, uint32_t num_args, void *object_or_called_scope) { uint32_t used_stack = zend_vm_calc_used_stack(num_args, func);