ext/pcntl: do not drop queued signals when an exception is pending

NEWS

@@ -65,6 +65,12 @@ PHP                                                                        NEWS
     is_cacheable_stream_path()). (ndossche)
   . Fix zend_analyze_calls() call_stack buffer overrun. (Mrmaxmeier)
 
+- PCNTL:
+  . Fixed pcntl_signal_dispatch() dropping the queued signals when it runs while
+    an exception is pending. (nicolas-grekas)
+  . Fixed pcntl_signal_dispatch() dropping the signals queued behind a handler
+    that throws. (nicolas-grekas)
+
 - PDO:
   . Fixed PDOStatement::getColumnMeta() reading out of bounds for an invalid
     column index. (Ilia Alshanetsky)

ext/pcntl/pcntl.c

@@ -31,6 +31,7 @@
 #include "ext/standard/info.h"
 #include "php_signal.h"
 #include "php_ticks.h"
+#include "zend_exceptions.h"
 #include "zend_fibers.h"
 
 #if defined(HAVE_GETPRIORITY) || defined(HAVE_SETPRIORITY) || defined(HAVE_WAIT3)
@@ -1318,6 +1319,9 @@ void pcntl_signal_dispatch(void)
 {
 	zval params[2], *handle, retval;
 	struct php_pcntl_pending_signal *queue, *next;
+	zend_object *old_exception;
+	const zend_op *old_opline_before_exception = NULL;
+	const zend_op *old_opline = NULL;
 	sigset_t mask;
 	sigset_t old_mask;
 
@@ -1345,8 +1349,24 @@ void pcntl_signal_dispatch(void)
 	PCNTL_G(head) = NULL; /* simple stores are atomic */
 	PCNTL_G(tail) = NULL;
 
+	/* Dispatching can happen with an exception pending, e.g. from the interrupt check that runs
+	 * right after an internal function threw. call_user_function() does nothing in that state,
+	 * so set the exception aside while the handlers run. The frame is left as found: depending
+	 * on the caller, the exception may not be registered on it yet, or may already be on its
+	 * way to a catch block, and the caller takes it from there once we return. */
+	old_exception = EG(exception);
+	if (old_exception) {
+		if (EG(current_execute_data)) {
+			old_opline = EG(current_execute_data)->opline;
+		}
+		old_opline_before_exception = EG(opline_before_exception);
+		EG(exception) = NULL;
+	}
+
 	/* Allocate */
 	while (queue) {
+		bool handler_threw = false;
+
 		if ((handle = zend_hash_index_find(&PCNTL_G(php_signal_table), queue->signo)) != NULL) {
 			if (Z_TYPE_P(handle) != IS_LONG) {
 				ZVAL_NULL(&retval);
@@ -1365,27 +1385,52 @@ void pcntl_signal_dispatch(void)
 #ifdef HAVE_STRUCT_SIGINFO_T
 				zval_ptr_dtor(&params[1]);
 #endif
-				if (EG(exception)) {
-					break;
-				}
+				handler_threw = NULL != EG(exception);
 			}
 		}
 
 		next = queue->next;
 		queue->next = PCNTL_G(spares);
 		PCNTL_G(spares) = queue;
 		queue = next;
+
+		/* No other handler can be called while the exception propagates */
+		if (handler_threw) {
+			break;
+		}
 	}
 
-	/* drain the remaining in case of exception thrown */
-	while (queue) {
-		next = queue->next;
-		queue->next = PCNTL_G(spares);
-		PCNTL_G(spares) = queue;
-		queue = next;
+	if (old_exception) {
+		if (EG(current_execute_data)) {
+			EG(current_execute_data)->opline = old_opline;
+		}
+		EG(opline_before_exception) = old_opline_before_exception;
+		if (EG(exception)) {
+			zend_exception_set_previous(EG(exception), old_exception);
+		} else {
+			EG(exception) = old_exception;
+		}
 	}
 
-	PCNTL_G(pending_signals) = 0;
+	if (UNEXPECTED(queue)) {
+		/* Put back what the throwing handler did not get to, instead of dropping it, and ask
+		 * the engine to come back once the exception has been handled. Signals are still
+		 * blocked here, so PCNTL_G(head) cannot have been repopulated in the meantime. */
+		next = queue;
+
+		while (next->next) {
+			next = next->next;
+		}
+
+		PCNTL_G(head) = queue;
+		PCNTL_G(tail) = next;
+
+		if (PCNTL_G(async_signals)) {
+			zend_atomic_bool_store_ex(&EG(vm_interrupt), true);
+		}
+	} else {
+		PCNTL_G(pending_signals) = 0;
+	}
 
 	/* Re-enable queue */
 	PCNTL_G(processing_signal_queue) = 0;

ext/pcntl/tests/pcntl_signal_dispatch_exception_2.phpt

@@ -0,0 +1,44 @@
+--TEST--
+pcntl_signal_dispatch() keeps the signals left in the queue by a throwing handler
+--EXTENSIONS--
+pcntl
+posix
+--FILE--
+<?php
+
+$called = [];
+
+pcntl_signal(SIGUSR1, function ($signo) use (&$called) {
+    $called[] = 'SIGUSR1';
+    throw new \Exception('Exception in signal handler');
+});
+
+pcntl_signal(SIGUSR2, function ($signo) use (&$called) {
+    $called[] = 'SIGUSR2';
+});
+
+pcntl_signal(SIGHUP, function ($signo) use (&$called) {
+    $called[] = 'SIGHUP';
+});
+
+posix_kill(posix_getpid(), SIGUSR1);
+posix_kill(posix_getpid(), SIGUSR2);
+posix_kill(posix_getpid(), SIGHUP);
+
+try {
+    pcntl_signal_dispatch();
+} catch (\Exception $e) {
+    echo $e->getMessage() . "\n";
+}
+
+echo "Handlers called: " . implode(', ', $called) . "\n";
+
+pcntl_signal_dispatch();
+
+echo "Handlers called: " . implode(', ', $called) . "\n";
+
+?>
+--EXPECT--
+Exception in signal handler
+Handlers called: SIGUSR1
+Handlers called: SIGUSR1, SIGUSR2, SIGHUP

ext/pcntl/tests/pcntl_signal_dispatch_exception_3.phpt

@@ -0,0 +1,47 @@
+--TEST--
+pcntl_signal_dispatch() delivers the signals a throwing handler left behind once its exception is handled
+--EXTENSIONS--
+pcntl
+posix
+--FILE--
+<?php
+
+$called = [];
+
+pcntl_signal(SIGUSR1, function ($signo) use (&$called) {
+    $called[] = 'SIGUSR1';
+    throw new \Exception('Exception in signal handler');
+});
+
+pcntl_signal(SIGUSR2, function ($signo) use (&$called) {
+    $called[] = 'SIGUSR2';
+});
+
+pcntl_signal(SIGHUP, function ($signo) use (&$called) {
+    $called[] = 'SIGHUP';
+});
+
+// Queued, not dispatched: asynchronous signals are off
+posix_kill(posix_getpid(), SIGUSR1);
+posix_kill(posix_getpid(), SIGUSR2);
+
+pcntl_async_signals(true);
+
+try {
+    // Delivered asynchronously, so the whole queue is dispatched
+    posix_kill(posix_getpid(), SIGHUP);
+    echo "Not reached\n";
+} catch (\Exception $e) {
+    echo $e->getMessage() . "\n";
+}
+
+// No explicit dispatch: the engine delivers what the throwing handler left behind
+// on its own, as soon as the exception has been handled
+usleep(1000);
+
+echo "Handlers called: " . implode(', ', $called) . "\n";
+
+?>
+--EXPECT--
+Exception in signal handler
+Handlers called: SIGUSR1, SIGUSR2, SIGHUP

ext/pcntl/tests/pcntl_signal_dispatch_exception_pending.phpt

@@ -0,0 +1,24 @@
+--TEST--
+pcntl_signal_dispatch() runs the handlers of the signals raised while an internal function ran and then threw
+--EXTENSIONS--
+pcntl
+zend_test
+--FILE--
+<?php
+
+pcntl_async_signals(true);
+
+pcntl_signal(SIGUSR1, function ($signo) {
+    echo "Handler called\n";
+});
+
+try {
+    zend_test_raise_and_throw(SIGUSR1);
+} catch (\Exception $e) {
+    echo $e->getMessage(), "\n";
+}
+
+?>
+--EXPECT--
+Handler called
+Exception after raise()

ext/pcntl/tests/pcntl_signal_dispatch_exception_pending_user_frame.phpt

@@ -0,0 +1,33 @@
+--TEST--
+pcntl_signal_dispatch() with an exception pending after an internal function called from a user frame
+--EXTENSIONS--
+pcntl
+zend_test
+--FILE--
+<?php
+
+pcntl_signal(SIGUSR1, function ($signo) {
+    echo "Handler called from ", debug_backtrace()[1]['function'], "()\n";
+});
+
+pcntl_async_signals(true);
+
+function test() {
+    declare(ticks=1) {
+        register_tick_function('zend_test_raise_and_throw', SIGUSR1);
+    }
+    unregister_tick_function('zend_test_raise_and_throw');
+}
+
+test();
+
+?>
+--EXPECTF--
+Handler called from test()
+
+Fatal error: Uncaught Exception: Exception after raise() in %s:%d
+Stack trace:
+#0 %s(%d): zend_test_raise_and_throw(%d)
+#1 %s(%d): test()
+#2 {main}
+  thrown in %s on line %d

ext/zend_test/test.c

@@ -37,6 +37,7 @@
 #include "zend_call_stack.h"
 #include "zend_exceptions.h"
 #include "zend_mm_custom_handlers.h"
+#include <signal.h>
 
 // `php.h` sets `NDEBUG` when not `PHP_DEBUG` which will make `assert()` from
 // assert.h a no-op. In order to have `assert()` working on NDEBUG builds, we
@@ -672,6 +673,22 @@ static ZEND_FUNCTION(zend_test_crash)
 	php_printf("%s", invalid);
 }
 
+static ZEND_FUNCTION(zend_test_raise_and_throw)
+{
+	zend_long signo;
+
+	ZEND_PARSE_PARAMETERS_START(1, 1)
+		Z_PARAM_LONG(signo)
+	ZEND_PARSE_PARAMETERS_END();
+
+	if (raise((int) signo) != 0) {
+		zend_throw_error(NULL, "raise() failed");
+		RETURN_THROWS();
+	}
+
+	zend_throw_exception(NULL, "Exception after raise()", 0);
+}
+
 static bool has_opline(zend_execute_data *execute_data)
 {
 	return execute_data

ext/zend_test/test.stub.php

@@ -298,6 +298,8 @@ function zend_get_map_ptr_last(): int {}
 
     function zend_test_crash(?string $message = null): void {}
 
+    function zend_test_raise_and_throw(int $signal): void {}
+
     function zend_test_fill_packed_array(array &$array): void {}
 
     /** @return resource */