Skip to content

Commit

Permalink
Fix GH-11498: SIGCHLD is not always returned from proc_open
Browse files Browse the repository at this point in the history
Linux, and maybe other unixes, may merge multiple standard signals into
a single one. This causes issues when keeping track of process IDs.
Solve this by manually checking which children are dead using waitpid().

Test case is based on taka-oyama's test code.

Closes GH-11509.
  • Loading branch information
nielsdos committed Jun 23, 2023
1 parent 1111a95 commit f39b513
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 6 deletions.
4 changes: 4 additions & 0 deletions NEWS
Expand Up @@ -8,6 +8,10 @@ PHP NEWS
- Date:
. Fixed bug GH-11368 (Date modify returns invalid datetime). (Derick)

- PCNTL:
. Fixed bug GH-11498 (SIGCHLD is not always returned from proc_open).
(nielsdos)

- PCRE:
. Mangle PCRE regex cache key with JIT option. (mvorisek)

Expand Down
55 changes: 49 additions & 6 deletions ext/pcntl/pcntl.c
Expand Up @@ -1345,21 +1345,64 @@ static void pcntl_signal_handler(int signo)
/* oops, too many signals for us to track, so we'll forget about this one */
return;
}
PCNTL_G(spares) = psig->next;

psig->signo = signo;
psig->next = NULL;
struct php_pcntl_pending_signal *psig_first = psig;

/* Standard signals may be merged into a single one.
* POSIX specifies that SIGCHLD has the si_pid field (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html),
* so we'll handle the merging for that signal.
* See also: https://www.gnu.org/software/libc/manual/html_node/Merged-Signals.html */
if (signo == SIGCHLD) {
/* Note: The first waitpid result is not necessarily the pid that was passed above!
* We therefore cannot avoid the first waitpid() call. */
int status;
pid_t pid;
while (true) {
do {
errno = 0;
/* Although Linux specifies that WNOHANG will never result in EINTR, POSIX doesn't say so:
* https://pubs.opengroup.org/onlinepubs/9699919799/functions/waitpid.html */
pid = waitpid(WAIT_ANY, &status, WNOHANG);
} while (pid <= 0 && errno == EINTR);
if (pid <= 0) {
if (UNEXPECTED(psig == psig_first)) {
/* Don't handle multiple, revert back to the single signal handling. */
goto single_signal;
}
break;
}

psig->signo = signo;

#ifdef HAVE_STRUCT_SIGINFO_T
psig->siginfo = *siginfo;
psig->siginfo.si_pid = pid;
#endif

if (EXPECTED(psig->next)) {
psig = psig->next;
} else {
break;
}
}
} else {
single_signal:;
psig->signo = signo;

#ifdef HAVE_STRUCT_SIGINFO_T
psig->siginfo = *siginfo;
psig->siginfo = *siginfo;
#endif
}

PCNTL_G(spares) = psig->next;
psig->next = NULL;

/* the head check is important, as the tick handler cannot atomically clear both
* the head and tail */
if (PCNTL_G(head) && PCNTL_G(tail)) {
PCNTL_G(tail)->next = psig;
PCNTL_G(tail)->next = psig_first;
} else {
PCNTL_G(head) = psig;
PCNTL_G(head) = psig_first;
}
PCNTL_G(tail) = psig;
PCNTL_G(pending_signals) = 1;
Expand Down
35 changes: 35 additions & 0 deletions ext/pcntl/tests/gh11498.phpt
@@ -0,0 +1,35 @@
--TEST--
GH-11498 (SIGCHLD is not always returned from proc_open)
--EXTENSIONS--
pcntl
--SKIPIF--
<?php
if (PHP_OS != 'Linux') {
die('skip Linux only');
}
?>
--FILE--
<?php
$processes = [];

pcntl_async_signals(true);
pcntl_signal(SIGCHLD, function($sig, $info) use (&$processes) {
unset($processes[$info['pid']]);
}, false);

foreach (range(0, 5) as $i) {
$process = proc_open('echo $$ > /dev/null', [], $pipes);
$pid = proc_get_status($process)['pid'];
$processes[$pid] = $process;
}

$iters = 50;
while (!empty($processes) && $iters > 0) {
usleep(100_000);
$iters--;
}

var_dump(empty($processes));
?>
--EXPECT--
bool(true)

0 comments on commit f39b513

Please sign in to comment.