ext/pcntl: do not drop queued signals when an exception is pending (#23624)

* ext/pcntl: run signal handlers when dispatch happens with an exception pending

ZEND_DO_FCALL runs its interrupt check right after an internal function returns,
before the pending exception is handled, and zend_call_function() does the same
for the calls it makes, so pcntl_interrupt_function() can reach 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 whole queue without a single
handler having run. The signal is destroyed rather than delayed: a later
pcntl_signal_dispatch() finds nothing left.

Set the exception aside while the handlers run and chain it back afterwards.
The frame is left as found, EG(opline_before_exception) included: 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 finishes the job once we return.

zend_test_raise_and_throw() and the user frame test are from Arnaud Le Blanc.

Any long blocking internal call that throws on timeout reaches this. pecl/amqp
throws "Consumer timeout exceed" out of AMQPQueue::consume(), which makes a
Symfony messenger worker miss every SIGTERM whatever the timeout is. PDO/SQLite
throws "database is locked" once busy_timeout expires, which kills a keepalive
SIGALRM for the rest of the process's life.

* ext/pcntl: keep the signals a throwing handler left in the queue

When a handler threw, the signals queued behind it were recycled without ever
being delivered. Put them back on the queue instead, and re-arm the interrupt so
that the engine dispatches them once the exception has been handled, rather than
leaving them to wait for another signal to come in.

Not calling further handlers while the exception propagates is unchanged.

Author: nicolas-grekas

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 */