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.

Author: bwoebi

NEWS

@@ -29,6 +29,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).

Zend/Optimizer/optimize_func_calls.c

@@ -195,15 +195,15 @@ 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) {
 							opline->opcode = zend_get_call_op(fcall, call_stack[call].func);
 						}
 					} 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;

Zend/Optimizer/zend_optimizer.c

@@ -1362,7 +1362,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 +1381,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 +1557,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 +1572,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);

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_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);

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);

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--
+<?php
+
+function inner() {
+	echo "Ok\n";
+}
+
+function foo() {
+	// If stack size is wrong, inner() will corrupt the previous observed frame
+	inner();
+}
+
+// After foo() def so that we land here, with step_two undone for foo() first
+function outer() {
+	// Pass 15 does constant string propagation, which gives a ZEND_INIT_DYNAMIC_FCALL on a const which Pass 4 will optimize
+	// Pass 4 must calc the right stack size here
+	(NAME)();
+}
+
+const NAME = "foo";
+
+outer();
+
+?>
+--EXPECTF--
+<!-- init '%s' -->
+<file '%s'>
+  <!-- init outer() -->
+  <outer>
+    <!-- init foo() -->
+    <foo>
+      <!-- init inner() -->
+      <inner>
+Ok
+      </inner>
+    </foo>
+  </outer>
+</file '%s'>
+