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

@@ -68,6 +68,8 @@ PHP                                                                        NEWS
 - 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

ext/pcntl/pcntl.c

@@ -1365,6 +1365,8 @@ void pcntl_signal_dispatch(void)
 
 	/* 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);
@@ -1383,24 +1385,19 @@ 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;
-	}
 
-	/* drain the remaining in case of exception thrown */
-	while (queue) {
-		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;
+		}
 	}
 
 	if (old_exception) {
@@ -1415,7 +1412,25 @@ void pcntl_signal_dispatch(void)
 		}
 	}
 
-	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