Skip to content

Commit

Permalink
Fix phpGH-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.
  • Loading branch information
nielsdos committed Jun 22, 2023
1 parent 039dd0b commit 5e64d53
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 6 deletions.
47 changes: 41 additions & 6 deletions ext/pcntl/pcntl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1345,21 +1345,56 @@ 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. */
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;
do {
do {
errno = 0;
pid = waitpid(WAIT_ANY, &status, WNOHANG);
} while (pid <= 0 && errno == EINTR);
if (pid <= 0) {
if (UNEXPECTED(psig == psig_first)) {
/* Couldn't handle even a single signal, forget about them. */
return;
}
break;
}

psig->signo = signo;

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

psig = psig->next;
} while (pid);
} else {
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
Original file line number Diff line number Diff line change
@@ -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 5e64d53

Please sign in to comment.