Hi Carlo
On 26/06/2025 19:24, Carlo Marcelo Arenas Belón wrote:
There has always been a small race condition between the time a
children status is checked, and the arrival of a SIGCHLD that
would alert us of their demise, hopefully interrupt poll().
The race was introduced by 695605b5080 (git-daemon: Simplify
dead-children reaping logic, 2008-08-14). Before that children were
reaped inside the signal handler so there was no race.
To close the gap, add the reading side of a pipe to the poll and
use the signal handler to write to it for each child.
As well as closing the race this fixes the issue with poll() not being
interrupted when a child exits. It also allows us to move the re-arming
of the handler into the event loop which would fix the infinite
recursion on AIX.
Suggested=by: Phillip Wood <phillip.wood123@xxxxxxxxx>
s/=/-/
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx>
---
Implements the "self pipe" trick Hillip suggested.
I've never been called that before :-/
I tried to make self healing and optional, as I also presume the race
condition it is meant to address is very unlikely.
I see this as an alternative fix for the other problems you've been
working on as well.
An obvious disadvantage (at least of this implementation), is that it
actually doubles the number of events that need to be handled for each
children process on most cases (ex: when `poll()` gets interrupted)
I suspect that if fixing that last race condition is so important with
the current foundation, it might be better to reintroduce some sort of
timeout to poll(), so that they will be cleared periodically.
I had a prototype (only the bare minimum) that I thought was more
efficient and that would instead remove completely the need for a
signal handler which I would post (only for RFC) later.
I'm not sure injecting an fd into each child process is a good direction.
daemon.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 46 insertions(+), 9 deletions(-)
diff --git a/daemon.c b/daemon.c
index d1be61fd57..d3b9421575 100644
--- a/daemon.c
+++ b/daemon.c
@@ -912,14 +912,17 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
add_child(&cld, addr, addrlen);
}
-static void child_handler(int signo UNUSED)
+int poll_pipe[2] = { -1, -1 };
Maybe call this signal_pipe? I'm not sure what poll_pipe means.
+
+static void child_handler(int signo)
{
/*
- * Otherwise empty handler because systemcalls will get interrupted
- * upon signal receipt
* SysV needs the handler to be rearmed
*/
signal(SIGCHLD, child_handler);
I think from Chris' email that it is conventional to do this at the end
of the handler. As I said above we could add an additional commit that
moves this into the event loop to fix the infinite recursion on AIX.
+
+ if (poll_pipe[1] >= 0)
+ write(poll_pipe[1], &signo, 1);
write() might fail so we should save errno around it. Conventionally one
would re-try on EINTR as well though in this case the most likely reason
for that is another child exiting which means the pipe would be written
to anyway.
}
static int set_reuse_addr(int sockfd)
@@ -1121,20 +1124,43 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
static int service_loop(struct socketlist *socklist)
{
struct pollfd *pfd;
+ unsigned long nfds = 1 + socklist->nr;
+
+ ALLOC_ARRAY(pfd, nfds);
+ if (!pipe(poll_pipe)) {
If we cannot create a pipe here then things have gone pretty badly wrong
and I think it is unlikely we're going to be able to accept incoming
connections so it would be best to die(). Relying on the signal pipe
would solve the problem of children not being collected until we receive
an new connection as well.
+ for (int i = 0; i < 2; i++) {
The body of this loop is quite indented - I think it would be better to
turn it into a function.
+ int flags;
+
+ flags = fcntl(poll_pipe[i], F_GETFD, 0);
+ if (flags >= 0)
+ fcntl(poll_pipe[i], F_SETFD, flags | FD_CLOEXEC);
I think we should probably close the pipes if we do not set FD_CLOEXEC.
+
+ flags = fcntl(poll_pipe[i], F_GETFL, 0);
+ if (flags < 0 || fcntl(poll_pipe[i], F_SETFL,
+ flags | O_NONBLOCK) == -1) {
+ close(poll_pipe[0]);
+ close(poll_pipe[1]);
+ poll_pipe[0] = poll_pipe[1] = -1;
+ break;
+ }
+ }
+ }
+ pfd[0].fd = poll_pipe[0];
+ pfd[0].events = POLLIN;
- CALLOC_ARRAY(pfd, socklist->nr);
-
- for (size_t i = 0; i < socklist->nr; i++) {
- pfd[i].fd = socklist->list[i];
+ for (size_t i = 1; i < nfds; i++) {
+ pfd[i].fd = socklist->list[i - 1];
pfd[i].events = POLLIN;
}
signal(SIGCHLD, child_handler);
for (;;) {
+ int nevents, scratch;
+
check_dead_children();
- if (poll(pfd, socklist->nr, -1) < 0) {
+ if ((nevents = poll(pfd, nfds, -1)) <= 0) {
if (errno != EINTR) {
logerror("Poll failed, resuming: %s",
strerror(errno));
sleep(1);
}
continue;
We need to drain the signal pipe on EINTR. I think it would be best to
do that just before calling check_dead_childern() above. We can avoid
calling check_dead_children() if we don't read anything.
}
@@ -1143,7 +1169,17 @@ static int service_loop(struct socketlist *socklist)
continue;
}
- for (size_t i = 0; i < socklist->nr; i++) {
+ if ((pfd[0].revents & POLLIN) && pfd[0].fd != -1) {
+ if (nevents == 1 && read(pfd[0].fd, &scratch, 1) > 0)
There can be more than one byte to read from the signal_pipe - we should
drain it until we see EAGAIN. As I said above I think it would be better
to do that just before calling check_dead_children(). Here we can just
decrement nevents if poll tells us the signal_pipe is readable.
Thanks
Phillip
+ continue;
+ else if (!read(pfd[0].fd, &scratch, 1)) {
+ close(pfd[0].fd);
+ pfd[0].fd = -1;
+ }
+ nevents--;
+ }
+
+ for (size_t i = 1; nevents && i < nfds; i++) {
if (pfd[i].revents & POLLIN) {
union {
struct sockaddr sa;
@@ -1165,6 +1201,7 @@ static int service_loop(struct socketlist *socklist)
}
}
handle(incoming, &ss.sa, sslen);
+ nevents--;
}
}
}
