Fix bug #81577: Execute interrupt handler on original opline

[nikic]

The interrupt handler should be executed while the original opline
is set, so that exceptions are thrown from the original opline,
rather than the target opline. We should only set the target
opline after the interrupt function has run.

Fixes https://bugs.php.net/bug.php?id=81577.

[dstogov]

This breaks an ability to use an interrupt handler for coroutine scheduling. By design, interrupt handler might transfer control to arbitrary stack frame and opline, but now the target opline is always the same.

This won't work with CALL VM, because VM_ENTER won't return.

This also probably needs special support for JIT.

[nikic]

This breaks an ability to use an interrupt handler for coroutine scheduling. By design, interrupt handler might transfer control to arbitrary stack frame and opline, but now the target opline is always the same.

This is why the code checks whether the execute_data/opline has been modified and uses VM_ENTER in that case. We only switch to target opline if they are not changed.

Or is the concern here what happens if the coroutine tries to switch back, in which case it will execute the previous opline again, rather than the target?

I don't really see how to address this. I think we can just drop the VM_ENTER functionality, it's an unnecessary generalization.

[dstogov]

Or is the concern here what happens if the coroutine tries to switch back, in which case it will execute the previous opline again, rather than the target?

yes. and this will lead to crash.

I don't really see how to address this. I think we can just drop the VM_ENTER functionality, it's an unnecessary generalization.

I don't think so, and I think, exception in interrupt handlers don't have to be thrown from the original opline.

The original bug may be easily fixed by UNDEF_RESULT() without any behaviour breaks

[dstogov]

I can't reproduce the failure, but I suppose, it should be fixed by this patch

diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h
index 9979a9b2fb..df756146e4 100644
--- a/Zend/zend_vm_def.h
+++ b/Zend/zend_vm_def.h
@@ -9856,6 +9856,7 @@ ZEND_VM_HELPER(zend_interrupt_helper, ANY, ANY)
 	if (EG(timed_out)) {
 		zend_timeout();
 	} else if (zend_interrupt_function) {
+		UNDEF_RESULT();
 		zend_interrupt_function(execute_data);
 		ZEND_VM_ENTER();
 	}
diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h
index 4a26e0c252..83e31f289c 100644
--- a/Zend/zend_vm_execute.h
+++ b/Zend/zend_vm_execute.h
@@ -3527,6 +3527,7 @@ static zend_never_inline ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL zend_interrupt_he
 	if (EG(timed_out)) {
 		zend_timeout();
 	} else if (zend_interrupt_function) {
+		UNDEF_RESULT();
 		zend_interrupt_function(execute_data);
 		ZEND_VM_ENTER();
 	}

[nikic]

@dstogov Using UNDEF_RESULT() would only work around this specific case. This affects all other unwinding cleanups as well. For example, this one will leak the live var for array_merge() result:

pcntl_async_signals(true);
pcntl_signal(SIGTERM, function ($signo) { throw new Exception("Signal"); });
array_merge([1], [2]) + posix_kill(posix_getpid(), SIGTERM);

Probably it's possible to construct similar examples where live var is incorrect freed or incorrect unfinished call cleanup happens.

[dstogov]

The bug is partially fixed in PHP-8.0 and above by fa0b84a and 5380b41

[codecov-commenter]

Codecov Report

Merging #7624 (5380b41) into master (ce62a98) will decrease coverage by 0.54%.
The diff coverage is 63.75%.

❗ Current head 5380b41 differs from pull request most recent head 703d713. Consider uploading reports for the commit 703d713 to get more accurate results

@@            Coverage Diff             @@
##           master    #7624      +/-   ##
==========================================
- Coverage   67.28%   66.73%   -0.55%     
==========================================
  Files         802      755      -47     
  Lines      301692   293056    -8636     
==========================================
- Hits       202997   195576    -7421     
+ Misses      98695    97480    -1215     

Impacted Files	Coverage Δ
Zend/zend_opcode.c	88.96% <0.00%> (+0.28%)	⬆️
ext/opcache/jit/zend_jit_disasm_x86.c	0.59% <0.00%> (ø)
ext/opcache/jit/zend_jit_helpers.c	9.97% <0.00%> (-2.00%)	⬇️
ext/opcache/jit/zend_jit_x86.dasc	11.03% <0.00%> (+2.60%)	⬆️
ext/opcache/jit/zend_jit_trace.c	70.46% <80.95%> (-0.83%)	⬇️
Zend/zend_API.c	81.04% <92.30%> (+0.35%)	⬆️
Zend/zend_compile.c	94.56% <100.00%> (-0.14%)	⬇️
Zend/zend_inheritance.c	90.00% <100.00%> (+0.40%)	⬆️
Zend/zend_vm_execute.h	63.14% <100.00%> (-1.27%)	⬇️
ext/bz2/bz2.c	79.91% <100.00%> (+0.52%)	⬆️
... and 595 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b302bfa...703d713. Read the comment docs.

[nikic]

Closing as a partial fix for the main issue landed. We couldn't figure out a good way to handle the general issue.