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

pcntl_signal_dispatch() takes the whole queue out of PCNTL_G(head) before it
starts calling handlers, and recycles every entry it walks over. Two paths let
signals disappear that way.

The first one is the interrupt handler. ZEND_VM_FCALL_INTERRUPT_CHECK() runs
right after an internal function returns, before the pending exception is
handled, so pcntl_interrupt_function() reaches the dispatcher with EG(exception)
set. call_user_function() returns without calling anything in that state, the
"if (EG(exception)) break" added by 296fad1 fires on the first entry, and
the drain loop then recycles the entire queue without a single handler having
run. Setting the exception aside for the duration of the dispatch, the way
destructors are called during unwinding, lets the handlers run.
zend_exception_save() is not usable for that: it goes through the single
EG(prev_exception) slot, so a dispatch happening inside an autoloader called
with an exception set aside would hand that exception back too early.

The second one is a handler that throws while other signals are queued behind
it: those were recycled too. They now go back to the queue, and with
asynchronous signals the interrupt is re-armed, so that the engine delivers
them on its own once the exception is handled instead of waiting for another
signal to come in.

This is reachable from any long blocking internal call that throws on timeout.
pecl/amqp is one: AMQPQueue::consume() throws "Consumer timeout exceed" when
the read timeout expires, which made a Symfony messenger worker built on it
miss every SIGTERM, whatever the timeout was.

Author: nicolas-grekas

NEWS

@@ -11,6 +11,10 @@ PHP                                                                        NEWS
     registrations are freed while still reachable from the cycle collector.
     (Ilia Alshanetsky)
 
+- PCNTL:
+  . Fixed pcntl_signal_dispatch() dropping queued signals when it runs while an
+    exception is pending. (nicolas-grekas)
+
 - SPL:
   . Fixed bug GH-23385 (SplDoublyLinkedList::serialize() use-after-free when
     __serialize() removes an element). (David Carlier)

ext/pcntl/pcntl.c

@@ -29,6 +29,7 @@
 #include "ext/standard/info.h"
 #include "php_signal.h"
 #include "php_ticks.h"
+#include "zend_exceptions.h"
 #include "zend_fibers.h"
 #include "main/php_main.h"
 
@@ -1352,6 +1353,8 @@ 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;
 	sigset_t mask;
 	sigset_t old_mask;
 
@@ -1379,8 +1382,21 @@ void pcntl_signal_dispatch(void)
 	PCNTL_G(head) = NULL; /* simple stores are atomic */
 	PCNTL_G(tail) = NULL;
 
+	/* Dispatching can happen while an exception is propagating, typically from the interrupt
+	 * check that runs right after an internal function returned with an exception pending.
+	 * Handlers cannot be called in that state, so set the exception aside while they run,
+	 * the way destructors are called during unwinding. */
+	old_exception = EG(exception);
+	if (old_exception && EG(current_execute_data)) {
+		EG(current_execute_data)->opline = EG(opline_before_exception);
+		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_LONG(&params[0], queue->signo);
@@ -1391,28 +1407,52 @@ void pcntl_signal_dispatch(void)
 				call_user_function(NULL, NULL, handle, &retval, 2, params);
 				zval_ptr_dtor(&retval);
 				zval_ptr_dtor(&params[1]);
-
-				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 = EG(exception_op);
+			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) = false;
+	if (UNEXPECTED(queue)) {
+		/* The signals a throwing handler left behind go back to the queue instead of being
+		 * dropped, and the engine is asked to dispatch again once that exception is handled.
+		 * Signals are still blocked here, so PCNTL_G(head) cannot have been repopulated. */
+		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) = false;
+	}
 
 	/* Re-enable queue */
 	PCNTL_G(processing_signal_queue) = false;

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