Skip to content

Fix GH-11498: SIGCHLD is not always returned from proc_open #11509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 46 additions & 6 deletions ext/pcntl/pcntl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1345,21 +1345,61 @@ 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;
while (true) {
do {
errno = 0;
pid = waitpid(WAIT_ANY, &status, WNOHANG);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linux docs say that waitpid with WNOHANG can't EINTR, but then again https://www.gnu.org/software/libc/manual/html_node/Merged-Signals.html looks like the code this is proposing, so it's probably better to keep it. Also not sure about other Unix systems.

Btw, might be good to link the above somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About Linux docs: yes, but POSIX doesn't specify that so we should indeed keep it :) https://pubs.opengroup.org/onlinepubs/9699919799/functions/waitpid.html
And yes, forgot to link that, my mistake.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll actually add that as a note too; otherwise we risk someone in the future going like "well this cannot happen, yeet"

} 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
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)