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 [off-list ref]

s/=/-/

Signed-off-by: Carlo Marcelo Arenas Belón <redacted>
---
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.

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.

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.

		}

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