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

[nicolas-grekas]

pcntl_signal_dispatch() loses queued signals in two situations, one per commit.

When dispatch is reached with an exception pending, e.g. from the interrupt check that runs right after an internal function threw, call_user_function() does nothing and the queue is then drained. The first commit sets the exception aside while the handlers run and leaves the frame as found.

When a handler throws, the signals queued behind it were dropped. The second commit puts them back and re-arms the interrupt, so they're dispatched once the exception has been handled.

Any internal function that blocks and then throws on timeout reaches the first case: a worker blocked in pecl/amqp's AMQPQueue::consume() misses every SIGTERM, and PDO/SQLite hitting busy_timeout loses the SIGALRM its keepalive depends on.

[devnexen]

Thanks I ll have a look some time in the following days, I would say tough after a quick look there are incoming changes needed.

[TimWolla]

Is #22538 related?

[nicolas-grekas]

@TimWolla Related in that #22538 rewrites the same function, but it neither causes nor fixes this, and it does not help the case that made me look. I built it (master, 8.6.0-dev) with pecl/amqp 2.2.0 on top (plus php-amqp#638 to get that compiling) and ran the same worker loop:

read_timeout=0.20s, SIGTERM at 1.70s: 31 rounds, never stopped
read_timeout=0.20s, SIGTERM at 2.31s: 31 rounds, never stopped
control on that build: stream_socket_accept / stream_select / sleep all interrupted at 0.50s

Two reasons. #22538 runs handlers on EINTR inside php_sockop_*, but librabbitmq owns that socket and retries EINTR itself, so PHP never sees it and the only way out of consume() is still the read timeout, which throws. And the rewritten dispatcher keeps both losses: call_user_function() is still called with EG(exception) possibly set (so it is a no-op), if (EG(exception)) { interrupt = true; break; } then fires on the first entry, and /* drain the remaining in case of exception thrown */ if (EG(exception)) { while (pcntl_signal_dequeue(&sig)) {} } empties the ring buffer. The second loss arguably matters more there, since #22538 makes "a handler throws" a supported way to interrupt a syscall: whatever is queued behind that handler is then silently gone.

So whichever lands first, the other needs the same two guards, setting the exception aside while handlers run and keeping what a throwing handler leaves behind. I can port them onto #22538 once it settles.

@devnexen Thanks. Meanwhile I found a problem in my own change and reworked it in 17ff74f: zend_exception_save() goes through the single EG(prev_exception) slot, so a dispatch happening inside an autoloader called with an exception set aside would have handed that exception back too early. It now uses a local, the way zend_objects_destroy_object() does. The other change is that when a throwing handler leaves signals behind, the interrupt is re-armed so the engine delivers them on its own once the exception is handled, rather than waiting for another signal; that turned out to be deterministic to test, hence pcntl_signal_dispatch_exception_3.phpt. If what you saw is something else, a pointer would help.

[ineersa]

Hitting same issue with Symfony Messenger and keepalive.
Using Doctrine with SQLite as a transport.

What happens, step by step:

1. The worker's receive loop (normal code, not a signal handler) polls the transport DB and calls BEGIN IMMEDIATE.
2. Another process holds the SQLite write lock, so that call blocks inside native code, up to busy_timeout (5 s in your config).
3. While it's blocked, the 2 s SIGALRM arrives. The kernel marks it pending; PHP can't run the handler until the native call returns.
4. The call gives up and throws database is locked — so PHP now has an exception pending and a queued signal.
5. PHP's bug (ext/pcntl: do not drop queued signals when an exception is pending #23624): when a native call returns with an exception pending, it drops the queued signal. The handler never runs, the one-shot alarm is never re-armed, and the heartbeat is dead for the rest of the process's life.

[devnexen]

You also need to save (and restore) EG(opline_before_exception) as zend_objects_destroy_object does.

[nicolas-grekas]

thanks, fixed (and sorry I messed up with the rebase so the labels are wrong now...)

[arnaud-lb]

There is an edge case where EG(current_execute_data) belongs to a user function frame, but the exception hasn't been throw on that frame yet. This causes EG(opline_before_exception) to be NULL / invalid for that frame.

This can happen at least when zend_call_function() is called from a user frame, as it handles interrupts before calling zend_rethrow_exception().

The fix is to use zend_rethrow_exception() like zend_objects_destroy_object() does.

Test case:

--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();

?>

[nicolas-grekas]

good catch, your test reproduces it. zend_rethrow_exception() breaks pcntl_signal_dispatch_exception_3.phpt though: there the re-armed dispatch runs after ZEND_HANDLE_EXCEPTION already jumped to the catch, so re-registering makes the catch op the throwing op and the exception escapes it. The frame is now left as found instead, opline and opline_before_exception both restored as they were, and the caller takes it from there.

[arnaud-lb]

I'm not a fan of letting signal handlers execute after one of them throws. Throwing should mean "unwind the call stack, execute only catch/finally blocks". But here we are letting adjacent signal handlers execute.

This problem may be fixed in the same way as GH-22538: Execute signal handlers on EINTR.

In the case of pecl/amqp, since amqp_consume_message() always retries on EINTR without returning to the caller, AMQPQueue::consume() should invoke signal handlers just after amqp_consume_message() returns.

[nicolas-grekas]

@arnaud-lb I split the patch in two: the 1st is the minimum fix. It cannot be tested with a .phpt because it needs an internal function that has a signal delivered while it runs and then throws.

The 2nd commit is the part you object to. It isn't unprecedented: destructors already behave that way. If one destructor throws during unwinding, the remaining ones still run, immediately, with the exceptions chained:

• dtor A runs, throws
• dtor B runs (after A threw)
• dtor C runs
• caught Exception(from A) previous=outer

The 2nd commit is more conservative than that: it stops calling handlers as soon as one throws, and delivers what is left only once the exception has been handled.

There has to be a fix in php-src: PDO/SQLite hits the same defect (comment above), so a third extension is already queued up for the same workaround, etc. That's what tells me where the fix belongs.

On GH-22538 and my 2nd commit: it strengthens the case rather than replacing it. GH-22538 makes throwing from a handler the documented way to interrupt a syscall, so "a handler threw while other signals were queued" becomes routine, and its dispatcher still drains those (if (EG(exception)) { while (pcntl_signal_dequeue(&sig)) {} }). A worker that breaks out of a read by throwing from its SIGALRM handler would lose a SIGTERM that arrived in the same window.

[arnaud-lb]

Thinking more about it, I agree that the fixed behavior makes sense. This also doesn't conflict with #22538.

the 1st is the minimum fix. It cannot be tested with a .phpt because it needs an internal function that has a signal delivered while it runs and then throws

We can add a helper function in ext/zend_test for this purpose:

static PHP_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);
}

[nicolas-grekas]

Thank you @arnaud-lb, PR updated: your helper is in ext/zend_test, so both commits have tests now.

[arnaud-lb]

Thank you!