Interrupt while internal frame is on the stack (#14627)

* Check VM interrupt while internal frame is on top

* Use tab instead of spaces

* fix frame used in interrupt and refactor

* remove unused failures for zend_jit_check_timeout

* Fix JIT support

Co-authored-by: Bob Weinand <[email protected]>

* Fix the missing store to vm_interrupt

* Rename new functions

* Special case zend_interrupt_function in JIT code

* refactor to use ZEND_VM_SET_OPCODE_NO_INTERRUPT

* Split atomic exchange into load + store

It is difficult to determine performance of atomics sometimes. In this
case, the separate load+store is still correct, and a load does not
cause a modification, and might be faster for some platforms than an
exchange. A load+store is slower than an exchange, but we're fine
trading the penalty to the slow path and keeping the happy path faster.

---------

Co-authored-by: Bob Weinand <[email protected]>

Author: morrisonlevi

UPGRADING.INTERNALS

@@ -377,6 +377,11 @@ PHP 8.4 INTERNALS UPGRADE NOTES
 4. OpCode changes
 ========================
 
+* DO_ICALL, DO_FCALL, and DO_FCALL_BY_NAME now call zend_interrupt_function
+  while the internal frame is still on the stack. This means interrupt handlers
+  will now see the internal call. If your interrupt handler does something like
+  switching EG(current_execute_data), it should not do so if an internal func
+  is on top.
 * New FRAMELESS_ICALL_[0,3] opcodes for faster internal function calls have been
   added. These opcodes don't create a stack frame, but pass arguments via opcode
   operands. They only work for functions that are known at compile-time, and

Zend/tests/fibers/signal-async.phpt

@@ -17,7 +17,7 @@ pcntl_signal(SIGUSR1, function (): void {
 $fiber = new Fiber(function (): void {
     echo "Fiber start\n";
     posix_kill(posix_getpid(), SIGUSR1);
-    time_nanosleep(1);
+    time_nanosleep(1, 0);
     echo "Fiber end\n";
 });
 
@@ -30,8 +30,9 @@ Fiber start
 Fatal error: Uncaught FiberError: Cannot switch fibers in current execution context in %ssignal-async.php:%d
 Stack trace:
 #0 %ssignal-async.php(%d): Fiber::suspend()
-#1 %ssignal-async.php(%d): {closure:%s:%d}(%d, Array)
-#2 [internal function]: {closure:%s:%d}()
-#3 %ssignal-async.php(%d): Fiber->start()
-#4 {main}
+#1 [internal function]: {closure:%s:%d}(%d, Array)
+#2 %ssignal-async.php(%d): posix_kill(%d, %d)
+#3 [internal function]: {closure:%s:%d}()
+#4 %ssignal-async.php(%d): Fiber->start()
+#5 {main}
   thrown in %ssignal-async.php on line %d

Zend/zend.h

@@ -341,7 +341,22 @@ extern ZEND_API size_t (*zend_printf)(const char *format, ...) ZEND_ATTRIBUTE_PT
 extern ZEND_API zend_write_func_t zend_write;
 extern ZEND_API FILE *(*zend_fopen)(zend_string *filename, zend_string **opened_path);
 extern ZEND_API void (*zend_ticks_function)(int ticks);
+
+/* Called by the VM in certain places like at the loop header, user function
+ * entry, and after internal function calls, if EG(vm_interrupt) has been set.
+ *
+ * If this is used to switch the EG(current_execute_data), such as implementing
+ * a coroutine scheduler, then it needs to check the top frame to see if it's
+ * an internal function. If an internal function is on top, then the frame
+ * shouldn't be switched away.
+ *
+ * Prior to PHP 8.0, this check was not necessary. In PHP 8.0,
+ * zend_call_function started calling zend_interrupt_function, and in 8.4 the
+ * DO_*CALL* opcodes started calling the zend_interrupt_function while the
+ * internal frame is still on top.
+ */
 extern ZEND_API void (*zend_interrupt_function)(zend_execute_data *execute_data);
+
 extern ZEND_API void (*zend_error_cb)(int type, zend_string *error_filename, const uint32_t error_lineno, zend_string *message);
 extern ZEND_API void (*zend_on_timeout)(int seconds);
 extern ZEND_API zend_result (*zend_stream_open_function)(zend_file_handle *handle);

Zend/zend_execute.c

@@ -4095,6 +4095,16 @@ ZEND_API void ZEND_FASTCALL zend_free_compiled_variables(zend_execute_data *exec
 }
 /* }}} */
 
+ZEND_API ZEND_COLD void ZEND_FASTCALL zend_fcall_interrupt(zend_execute_data *call)
+{
+	zend_atomic_bool_store_ex(&EG(vm_interrupt), false);
+	if (zend_atomic_bool_load_ex(&EG(timed_out))) {
+		zend_timeout();
+	} else if (zend_interrupt_function) {
+		zend_interrupt_function(call);
+	}
+}
+
 #define ZEND_VM_INTERRUPT_CHECK() do { \
 		if (UNEXPECTED(zend_atomic_bool_load_ex(&EG(vm_interrupt)))) { \
 			ZEND_VM_INTERRUPT(); \
@@ -4107,6 +4117,12 @@ ZEND_API void ZEND_FASTCALL zend_free_compiled_variables(zend_execute_data *exec
 		} \
 	} while (0)
 
+#define ZEND_VM_FCALL_INTERRUPT_CHECK(call) do { \
+		if (UNEXPECTED(zend_atomic_bool_load_ex(&EG(vm_interrupt)))) { \
+			zend_fcall_interrupt(call); \
+		} \
+	} while (0)
+
 /*
  * Stack Frame Layout (the whole stack frame is allocated at once)
  * ==================
@@ -5541,9 +5557,12 @@ static zend_always_inline zend_execute_data *_zend_vm_stack_push_call_frame(uint
 	CHECK_SYMBOL_TABLES() \
 	OPLINE = new_op
 
-#define ZEND_VM_SET_OPCODE(new_op) \
+#define ZEND_VM_SET_OPCODE_NO_INTERRUPT(new_op) \
 	CHECK_SYMBOL_TABLES() \
-	OPLINE = new_op; \
+	OPLINE = new_op
+
+#define ZEND_VM_SET_OPCODE(new_op) \
+	ZEND_VM_SET_OPCODE_NO_INTERRUPT(new_op); \
 	ZEND_VM_INTERRUPT_CHECK()
 
 #define ZEND_VM_SET_RELATIVE_OPCODE(opline, offset) \

Zend/zend_execute.h

@@ -542,6 +542,11 @@ ZEND_COLD void zend_magic_get_property_type_inconsistency_error(const zend_prope
 
 ZEND_COLD void zend_match_unhandled_error(const zval *value);
 
+/* Call this to handle the timeout or the interrupt function. It will set
+ * EG(vm_interrupt) to false.
+ */
+ZEND_API ZEND_COLD void ZEND_FASTCALL zend_fcall_interrupt(zend_execute_data *call);
+
 static zend_always_inline void *zend_get_bad_ptr(void)
 {
 	ZEND_UNREACHABLE();

Zend/zend_system_id.c

@@ -55,10 +55,11 @@ void zend_startup_system_id(void)
 	zend_system_id[0] = '\0';
 }
 
-#define ZEND_HOOK_AST_PROCESS      (1 << 0)
-#define ZEND_HOOK_COMPILE_FILE     (1 << 1)
-#define ZEND_HOOK_EXECUTE_EX       (1 << 2)
-#define ZEND_HOOK_EXECUTE_INTERNAL (1 << 3)
+#define ZEND_HOOK_AST_PROCESS        (1 << 0)
+#define ZEND_HOOK_COMPILE_FILE       (1 << 1)
+#define ZEND_HOOK_EXECUTE_EX         (1 << 2)
+#define ZEND_HOOK_EXECUTE_INTERNAL   (1 << 3)
+#define ZEND_HOOK_INTERRUPT_FUNCTION (1 << 4)
 
 void zend_finalize_system_id(void)
 {
@@ -77,6 +78,9 @@ void zend_finalize_system_id(void)
 	if (zend_execute_internal) {
 		hooks |= ZEND_HOOK_EXECUTE_INTERNAL;
 	}
+	if (zend_interrupt_function) {
+		hooks |= ZEND_HOOK_INTERRUPT_FUNCTION;
+	}
 	PHP_MD5Update(&context, &hooks, sizeof hooks);
 
 	for (int16_t i = 0; i < 256; i++) {

Zend/zend_vm_def.h

@@ -4084,6 +4084,7 @@ ZEND_VM_HOT_HANDLER(129, ZEND_DO_ICALL, ANY, ANY, SPEC(RETVAL,OBSERVER))
 	}
 #endif
 	ZEND_OBSERVER_FCALL_END(call, EG(exception) ? NULL : ret);
+	ZEND_VM_FCALL_INTERRUPT_CHECK(call);
 
 	EG(current_execute_data) = execute_data;
 	zend_vm_stack_free_args(call);
@@ -4107,7 +4108,7 @@ ZEND_VM_HOT_HANDLER(129, ZEND_DO_ICALL, ANY, ANY, SPEC(RETVAL,OBSERVER))
 		HANDLE_EXCEPTION();
 	}
 
-	ZEND_VM_SET_OPCODE(opline + 1);
+	ZEND_VM_SET_OPCODE_NO_INTERRUPT(opline + 1);
 	ZEND_VM_CONTINUE();
 }
 
@@ -4205,6 +4206,7 @@ ZEND_VM_HOT_HANDLER(131, ZEND_DO_FCALL_BY_NAME, ANY, ANY, SPEC(RETVAL,OBSERVER))
 		}
 #endif
 		ZEND_OBSERVER_FCALL_END(call, EG(exception) ? NULL : ret);
+		ZEND_VM_FCALL_INTERRUPT_CHECK(call);
 
 		EG(current_execute_data) = execute_data;
 
@@ -4235,7 +4237,7 @@ ZEND_VM_C_LABEL(fcall_by_name_end):
 		zend_rethrow_exception(execute_data);
 		HANDLE_EXCEPTION();
 	}
-	ZEND_VM_SET_OPCODE(opline + 1);
+	ZEND_VM_SET_OPCODE_NO_INTERRUPT(opline + 1);
 	ZEND_VM_CONTINUE();
 }
 
@@ -4325,6 +4327,7 @@ ZEND_VM_HOT_HANDLER(60, ZEND_DO_FCALL, ANY, ANY, SPEC(RETVAL,OBSERVER))
 		}
 #endif
 		ZEND_OBSERVER_FCALL_END(call, EG(exception) ? NULL : ret);
+		ZEND_VM_FCALL_INTERRUPT_CHECK(call);
 
 		EG(current_execute_data) = execute_data;
 
@@ -4353,8 +4356,7 @@ ZEND_VM_C_LABEL(fcall_end):
 		zend_rethrow_exception(execute_data);
 		HANDLE_EXCEPTION();
 	}
-
-	ZEND_VM_SET_OPCODE(opline + 1);
+	ZEND_VM_SET_OPCODE_NO_INTERRUPT(opline + 1);
 	ZEND_VM_CONTINUE();
 }
 

Zend/zend_vm_execute.h

@@ -1425,9 +1425,7 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
 			zend_jit_set_last_valid_opline(&ctx, op_array->opcodes + ssa->cfg.blocks[b].start);
 		}
 		if (ssa->cfg.blocks[b].flags & ZEND_BB_LOOP_HEADER) {
-			if (!zend_jit_check_timeout(&ctx, op_array->opcodes + ssa->cfg.blocks[b].start, NULL)) {
-				goto jit_failure;
-			}
+			zend_jit_check_timeout(&ctx, op_array->opcodes + ssa->cfg.blocks[b].start, NULL);
 		}
 		if (!ssa->cfg.blocks[b].len) {
 			zend_jit_bb_end(&ctx, b);