pcntl_signal_dispatch() undoing the signal mask changes of its handlers

[EdmondDantes]

Issue

pcntl_signal_dispatch() blocks every signal while the handlers run, then restores the mask it found with SIG_SETMASK. This has two effects:

1. Any change a handler makes to the signal mask is undone after the dispatch.
2. A signal that a handler unblocks while it runs is lost.

Reproducer: a mask change is undone

<?php
pcntl_sigprocmask(SIG_BLOCK, [SIGHUP]);
 
pcntl_signal(SIGUSR1, function () {
    pcntl_sigprocmask(SIG_UNBLOCK, [SIGHUP]);
});
posix_kill(posix_getpid(), SIGUSR1);
pcntl_signal_dispatch();
 
pcntl_sigprocmask(SIG_BLOCK, [SIGUSR2], $mask);
var_dump(in_array(SIGHUP, $mask));

Expected bool(false), actual bool(true): SIGHUP is blocked again.

The same happens to the unblock that pcntl_signal() performs (zend_sigaction() unblocks the signal it installs a handler for), and to an unblock done by an extension a handler calls into. Nothing unblocks that signal afterwards, so the process can stay deaf to it, SIGTERM included.

Reproducer: a signal arriving during a handler is lost

<?php
pcntl_signal(SIGHUP, function () { echo "SIGHUP\n"; });
pcntl_sigprocmask(SIG_BLOCK, [SIGHUP]);
posix_kill(posix_getpid(), SIGHUP);
 
pcntl_signal(SIGUSR1, function () {
    pcntl_sigprocmask(SIG_UNBLOCK, [SIGHUP]);
});
posix_kill(posix_getpid(), SIGUSR1);
pcntl_signal_dispatch();
pcntl_signal_dispatch();

Expected SIGHUP, actual: nothing. The pending SIGHUP is delivered and queued as soon as the handler unblocks it, but the end of the dispatch clears pending_signals. The signal is lost in two more ways:

• after a throwing handler, the queue head is overwritten with the signals that handler left (the new node also leaks);
• with pcntl_async_signals(true), the engine interrupt the signal raises is spent on a nested dispatch, which returns at once because the queue is being processed.
  Both reproducers give the actual output above on PHP 8.3.6; PHP-8.4 and master have the same code.

Fix

Each handler, together with the destructor of its return value, runs under the thread's own mask. Signals are blocked only while the queue and the spare list change, and the dispatch ends with the mask the handlers left. A signal that arrives meanwhile stays queued, behind what a throwing handler left; pending_signals stays set, and with async signals the engine interrupt is raised again.

Behaviour changes

• A signal that arrives while a handler runs is dispatched after it (with async signals, at the next interrupt check) instead of being dropped.
• Handlers no longer run with every signal blocked, so inside a handler signals behave as in the rest of the script: a blocking call can be interrupted, a signal left at SIG_DFL takes its default action at once, and the kernel no longer merges repeats of a signal: each one queued during a long handler takes a node from the spare pool, so a burst larger than the pool loses the rest, as between two pcntl_signal_dispatch() calls.
• Two sigprocmask() calls per handler.

Tests

Each fails without the fix:

• ext/pcntl/tests/pcntl_signal_dispatch_keeps_handler_sigprocmask.phpt: a block and an unblock from a handler, a signal blocked before and left alone, a save and restore inside a handler, a change made by the destructor of a handler's return value;
• ext/pcntl/tests/pcntl_signal_dispatch_keeps_signal_arriving_in_handler.phpt: the second reproducer;
• ext/pcntl/tests/pcntl_async_signals_keeps_signal_arriving_in_handler.phpt: the same with async signals;
• ext/pcntl/tests/pcntl_signal_dispatch_exception_keeps_signal_arriving_in_handler.phpt: what a throwing handler left goes ahead of the signal that arrived in it, and a signal sent later is queued behind both.

NEWS

- PCNTL:
  . Fixed pcntl_signal_dispatch() undoing the signal mask changes made by
    signal handlers, and losing a signal that arrives while they run.
    (Edmond Dantes)

[bukka]

This is not something that should go to PHP-8.4 as it has got a BC impact. It should be for master only but we actually plan to go with #22538 and it seems to cover this as far as I understand it. It would still make sense to then add the tests as they seem useful and not present in #22538. CC @arnaud-lb