Fix pcntl_signal_dispatch() undoing the signal mask changes of its handlers

The dispatch blocked every signal while its handlers ran and then restored the
mask it found. A handler's pcntl_sigprocmask() was undone, and so was an
unblock done by pcntl_signal() (zend_sigaction()) or by an extension a handler
calls into, after which nothing unblocked that signal again. A signal such an
unblock let in during the handlers was lost: the end of the dispatch cleared
pending_signals, or overwrote the queue's head after a throwing handler, and
the engine interrupt it raised was spent on a nested dispatch that returned at
once.

Each handler now runs under the thread's own mask, and signals are blocked only
while the queue changes. A signal that arrives meanwhile stays queued, after
what a throwing handler left, and with async signals the engine is asked to
come back for it. A block an extension takes in a handler on a signal that was
not blocked before the dispatch now stays too.

Author: EdmondDantes

ext/pcntl/pcntl.c

@@ -1326,6 +1326,7 @@ void pcntl_signal_dispatch(void)
 	const zend_op *old_opline = NULL;
 	sigset_t mask;
 	sigset_t old_mask;
+	sigset_t handler_mask;
 
 	if(!PCNTL_G(pending_signals)) {
 		return;
@@ -1351,6 +1352,10 @@ void pcntl_signal_dispatch(void)
 	PCNTL_G(head) = NULL; /* simple stores are atomic */
 	PCNTL_G(tail) = NULL;
 
+	/* Handlers run under the thread's own mask, and the dispatch ends with the mask they left; what
+	 * arrives meanwhile is queued for the next dispatch. */
+	handler_mask = old_mask;
+
 	/* 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
@@ -1382,7 +1387,9 @@ void pcntl_signal_dispatch(void)
 
 				/* Call php signal handler - Note that we do not report errors, and we ignore the return value */
 				/* FIXME: this is probably broken when multiple signals are handled in this while loop (retval) */
+				sigprocmask(SIG_SETMASK, &handler_mask, NULL);
 				call_user_function(NULL, NULL, handle, &retval, 2, params);
+				sigprocmask(SIG_BLOCK, &mask, &handler_mask);
 				zval_ptr_dtor(&retval);
 #ifdef HAVE_STRUCT_SIGINFO_T
 				zval_ptr_dtor(&params[1]);
@@ -1417,21 +1424,26 @@ void pcntl_signal_dispatch(void)
 	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. */
+		 * blocked here, but PCNTL_G(head) may hold what arrived while the handlers ran: it goes
+		 * after these. */
 		next = queue;
 
 		while (next->next) {
 			next = next->next;
 		}
 
+		next->next = PCNTL_G(head);
+		if (!PCNTL_G(head)) {
+			PCNTL_G(tail) = 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;
+	/* What arrived while the handlers ran is still queued, and a nested dispatch may have spent its
+	 * interrupt. */
+	PCNTL_G(pending_signals) = PCNTL_G(head) != NULL;
+	if (PCNTL_G(head) && PCNTL_G(async_signals)) {
+		zend_atomic_bool_store_ex(&EG(vm_interrupt), true);
 	}
 
 	/* Re-enable queue */
@@ -1441,7 +1453,7 @@ void pcntl_signal_dispatch(void)
 	zend_fiber_switch_unblock();
 
 	/* return signal mask to previous state */
-	sigprocmask(SIG_SETMASK, &old_mask, NULL);
+	sigprocmask(SIG_SETMASK, &handler_mask, NULL);
 }
 
 static void pcntl_signal_dispatch_tick_function(int dummy_int, void *dummy_pointer)

ext/pcntl/tests/pcntl_async_signals_keeps_signal_arriving_in_handler.phpt

@@ -0,0 +1,33 @@
+--TEST--
+pcntl_async_signals(): a signal that arrives while a handler runs is dispatched after it
+--EXTENSIONS--
+pcntl
+posix
+--FILE--
+<?php
+
+pcntl_async_signals(true);
+
+pcntl_signal(SIGHUP, function () {
+    echo "HUP\n";
+});
+pcntl_sigprocmask(SIG_BLOCK, [SIGHUP]);
+posix_kill(posix_getpid(), SIGHUP);
+
+pcntl_signal(SIGUSR1, function () {
+    echo "USR1\n";
+    pcntl_sigprocmask(SIG_UNBLOCK, [SIGHUP]);
+    echo "Unblocked\n";
+});
+
+posix_kill(posix_getpid(), SIGUSR1);
+// A check point for the interrupt the dispatch raised again: the return of an internal call.
+posix_getpid();
+echo "Done\n";
+
+?>
+--EXPECT--
+USR1
+Unblocked
+HUP
+Done

ext/pcntl/tests/pcntl_signal_dispatch_exception_keeps_signal_arriving_in_handler.phpt

@@ -0,0 +1,39 @@
+--TEST--
+pcntl_signal_dispatch() keeps a signal that arrives in a throwing handler after the ones that handler left
+--EXTENSIONS--
+pcntl
+posix
+--FILE--
+<?php
+
+pcntl_signal(SIGHUP, function () {
+    echo "HUP\n";
+});
+pcntl_sigprocmask(SIG_BLOCK, [SIGHUP]);
+posix_kill(posix_getpid(), SIGHUP);
+
+pcntl_signal(SIGUSR1, function () {
+    pcntl_sigprocmask(SIG_UNBLOCK, [SIGHUP]);
+    throw new Exception("USR1");
+});
+pcntl_signal(SIGUSR2, function () {
+    echo "USR2\n";
+});
+
+posix_kill(posix_getpid(), SIGUSR1);
+posix_kill(posix_getpid(), SIGUSR2);
+
+try {
+    pcntl_signal_dispatch();
+} catch (Exception $e) {
+    echo $e->getMessage(), "\n";
+}
+posix_kill(posix_getpid(), SIGUSR2);
+pcntl_signal_dispatch();
+
+?>
+--EXPECT--
+USR1
+USR2
+HUP
+USR2

ext/pcntl/tests/pcntl_signal_dispatch_keeps_handler_sigprocmask.phpt

@@ -0,0 +1,54 @@
+--TEST--
+pcntl_signal_dispatch() runs its handlers under the thread's mask and keeps the changes they made
+--EXTENSIONS--
+pcntl
+posix
+--FILE--
+<?php
+
+function blocked(): array {
+    // SIGCHLD, unused here: the call refuses an empty list.
+    pcntl_sigprocmask(SIG_BLOCK, [SIGCHLD], $mask);
+    pcntl_sigprocmask(SIG_SETMASK, $mask);
+    return $mask;
+}
+
+function is_blocked(int $signo): bool {
+    return in_array($signo, blocked(), true);
+}
+
+pcntl_sigprocmask(SIG_BLOCK, [SIGHUP, SIGALRM]);
+
+pcntl_signal(SIGUSR1, function () {
+    pcntl_sigprocmask(SIG_BLOCK, [SIGUSR2]);
+    pcntl_sigprocmask(SIG_UNBLOCK, [SIGHUP]);
+});
+
+posix_kill(posix_getpid(), SIGUSR1);
+pcntl_signal_dispatch();
+
+var_dump(is_blocked(SIGUSR2));
+var_dump(is_blocked(SIGHUP));
+var_dump(is_blocked(SIGALRM));
+
+echo "Saved and restored in a handler\n";
+
+$before = blocked();
+
+pcntl_signal(SIGUSR1, function () {
+    pcntl_sigprocmask(SIG_BLOCK, [SIGTERM], $old);
+    pcntl_sigprocmask(SIG_SETMASK, $old);
+});
+
+posix_kill(posix_getpid(), SIGUSR1);
+pcntl_signal_dispatch();
+
+var_dump(blocked() === $before);
+
+?>
+--EXPECT--
+bool(true)
+bool(false)
+bool(true)
+Saved and restored in a handler
+bool(true)

ext/pcntl/tests/pcntl_signal_dispatch_keeps_signal_arriving_in_handler.phpt

@@ -0,0 +1,29 @@
+--TEST--
+pcntl_signal_dispatch() keeps a signal that arrives while its handlers run
+--EXTENSIONS--
+pcntl
+posix
+--FILE--
+<?php
+
+pcntl_signal(SIGHUP, function () {
+    echo "HUP\n";
+});
+pcntl_sigprocmask(SIG_BLOCK, [SIGHUP]);
+posix_kill(posix_getpid(), SIGHUP);
+
+pcntl_signal(SIGUSR1, function () {
+    echo "USR1\n";
+    pcntl_sigprocmask(SIG_UNBLOCK, [SIGHUP]);
+});
+
+posix_kill(posix_getpid(), SIGUSR1);
+pcntl_signal_dispatch();
+echo "Dispatched\n";
+pcntl_signal_dispatch();
+
+?>
+--EXPECT--
+USR1
+Dispatched
+HUP