daemon: explicitly allow EINTR during poll()

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

Systems without SA_RESTART where using custom CFLAGS instead of the standard header file.

Consolidate that, so it will be easier to use in a future commit.

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
 compat/posix.h   | 7 +++++++
 config.mak.uname | 3 ---
 2 files changed, 7 insertions(+), 3 deletions(-)

compat/posix.h +0 -0

index 067a00f33b83..2612a8515897 100644

compat/posix.h +0 -0

 typedef uintmax_t timestamp_t;
 #define PRItime PRIuMAX
 #define parse_timestamp strtoumax

config.mak.uname +0 -3

index b1c5c4d5e8ed..52160ef5cb07 100644
@@ -654,8 +654,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 
 	# Not detected (nor checked for) by './configure'.
-	# We don't have SA_RESTART on NonStop, unfortunalety.
-	COMPAT_CFLAGS += -DSA_RESTART=0
 	# Apparently needed in compat/fnmatch/fnmatch.c.
 	COMPAT_CFLAGS += -DHAVE_STRING_H=1
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease

@@ -782,7 +780,6 @@ ifeq ($(uname_S),MINGW)
         endif
 endif
 ifeq ($(uname_S),QNX)
-	COMPAT_CFLAGS += -DSA_RESTART=0
 	EXPAT_NEEDS_XMLPARSE_H = YesPlease
 	HAVE_STRINGS_H = YesPlease
 	NEEDS_SOCKET = YesPlease

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

In a future change, the flags used for processing SIGCHLD will need to be updated, which is only possible by using sigaction().

Replace the call, which hs the added benefit of using BSD semantics reliably and therefore not needing the rearming call.

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
 daemon.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

daemon.c +5 -3

index d1be61fd5789..d870ad2f63c1 100644
@@ -917,9 +917,7 @@ static void child_handler(int signo UNUSED)
 	/*
 	 * Otherwise empty handler because systemcalls will get interrupted
 	 * upon signal receipt
-	 * SysV needs the handler to be rearmed
 	 */
-	signal(SIGCHLD, child_handler);
 }
 
 static int set_reuse_addr(int sockfd)

@@ -1121,6 +1119,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
 static int service_loop(struct socketlist *socklist)
 {
 	struct pollfd *pfd;
+	struct sigaction sa;
 
 	CALLOC_ARRAY(pfd, socklist->nr);
 

@@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)
 		pfd[i].events = POLLIN;
 	}
 
-	signal(SIGCHLD, child_handler);
+	sigemptyset(&sa.sa_mask);
+	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
+	sa.sa_handler = child_handler;
+	sigaction(SIGCHLD, &sa, NULL);
 
 	for (;;) {
 		check_dead_children();

daemon.c +1 -1

-static void child_handler(int signo UNUSED)
+static void child_handler(int signo)
 {
 	/*

static int set_reuse_addr(int sockfd)

daemon.c +15 -1

@@ -1118,8 +1122,10 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
 
 static int service_loop(struct socketlist *socklist)
 {
-	struct pollfd *pfd;
+#ifndef NO_SIGINTERRUPT
 	struct sigaction sa;
+#endif
+	struct pollfd *pfd;
 	CALLOC_ARRAY(pfd, socklist->nr);
 

@@ -1128,14 +1134,22 @@ static int service_loop(struct socketlist *socklist)
 		pfd[i].events = POLLIN;
 	}
 
+#ifdef NO_SIGINTERRUPT
+	signal(SIGCHLD, child_handler);
+#else
 	sigemptyset(&sa.sa_mask);
 	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
 	sa.sa_handler = child_handler;
 	sigaction(SIGCHLD, &sa, NULL);
+#endif
 
 	for (;;) {
 		check_dead_children();
 
+#ifndef NO_SIGINTERRUPT
+		sa.sa_flags &= ~SA_RESTART;
+		sigaction(SIGCHLD, &sa, NULL);
+#endif
 		if (poll(pfd, socklist->nr, -1) < 0) {
 			if (errno != EINTR) {
 				logerror("Poll failed, resuming: %s",

@@ -1144,6 +1158,10 @@ static int service_loop(struct socketlist *socklist)
 			}
 			continue;
 		}
+#ifndef NO_SIGINTERRUPT
+		sa.sa_flags |= SA_RESTART;
+		sigaction(SIGCHLD, &sa, NULL);
+#endif

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

In a future change, the flags used for processing SIGCHLD will need to be updated, which is only possible by using sigaction().

Replace the call, which hs the added benefit of using BSD semantics reliably and therefore not needing the rearming call.

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
 daemon.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

daemon.c +5 -3

index d1be61fd5789..d870ad2f63c1 100644
@@ -917,9 +917,7 @@ static void child_handler(int signo UNUSED)
 	/*
 	 * Otherwise empty handler because systemcalls will get interrupted
 	 * upon signal receipt
-	 * SysV needs the handler to be rearmed
 	 */
-	signal(SIGCHLD, child_handler);
 }
 
 static int set_reuse_addr(int sockfd)

@@ -1121,6 +1119,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
 static int service_loop(struct socketlist *socklist)
 {
 	struct pollfd *pfd;
+	struct sigaction sa;
 
 	CALLOC_ARRAY(pfd, socklist->nr);
 

@@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)
 		pfd[i].events = POLLIN;
 	}
 
-	signal(SIGCHLD, child_handler);
+	sigemptyset(&sa.sa_mask);
+	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
+	sa.sa_handler = child_handler;
+	sigaction(SIGCHLD, &sa, NULL);
 
 	for (;;) {
 		check_dead_children();

+/* On most systems <signal.h> would have given us this, but
+ * not on some systems (e.g. NonStop, QNX).
+ */

"Carlo Marcelo Arenas Belón via GitGitGadget"

Quoted 5 lines · <[email protected]> writes:

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

In a future change, the flags used for processing SIGCHLD will need to be updated, which is only possible by using sigaction().

Quoted 7 lines

Replace the call, which hs the added benefit of using BSD semantics reliably and therefore not needing the rearming call.

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
 daemon.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Hmph. Wouldn't it a much smaller change and fix to discard 2/3 and most of the 3/3 and instead make a siginterrupt() call to tell the system to interrupt us when SIGCHLD is received only on platforms where siginterrupt() is available? Use of sigaction() does not seem to be buying us anything for the purpose of this series.

Quoted 5 lines

Hmph. Wouldn't it a much smaller change and fix to discard 2/3 and most of the 3/3 and instead make a siginterrupt() call to tell the system to interrupt us when SIGCHLD is received only on platforms where siginterrupt() is available? Use of sigaction() does not seem to be buying us anything for the purpose of this series.

Using siginterrupt() would work (at least it did when I tested it in OpenBSD), but its use is discouraged as it has been obsoleted by the last two versions of POSIX (since 2018).

Most systems seem to be implementing signal() with sigaction() nowadays, but in the ones that are not (ex: Solaris) calling the later to get a struct sigaction with the flags being used, doesn't work and therefore it would seem, that the only way to do this reliably is by using sigaction everywhere for this signal, as implemented in 2/3.

"Carlo Marcelo Arenas Belón via GitGitGadget"

Quoted 5 lines · <[email protected]> writes:

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

In a future change, the flags used for processing SIGCHLD will need to be updated, which is only possible by using sigaction().

Quoted 2 lines

Replace the call, which hs the added benefit of using BSD semantics reliably and therefore not needing the rearming call.

"hs" -> "has"

Hmph, if we do not have to rearm, do we even need to have the handler at all, now it is a completely empty function? Presumably we'll see the answer to this question in the next step?

I didn'r address it because I didn't knew where to put it, but removing the signal handler isn't possible, because as soon as we do, EINTR is no longer "returned" by poll() on the systems that allowed that as an exception to SA_RESTART rules ...

This series addresses and ambiguity that is at least visible in OpenBSD, where zombie proceses would only be cleared after a new connection is received.

The underlying problem is that when this code was originally introduced, SA_RESTART was not widely implemented, and the signal() call usually implemented SysV like semantics, at least until it started being reimplemented by calling sigaction() internally.

Changes since v1

* Almost all references to siginterrupt has been removed and a better named variable used instead
* Changes had been anstracted to minimize ifdefs and their introduction staged more naturally

Carlo Marcelo Arenas Belón (3):
compat/posix.h: track SA_RESTART fallback
daemon: use sigaction() to install child_handler()
daemon: explicitly allow EINTR during poll()

  Makefile             |  6 +++++
  compat/mingw-posix.h |  1 -
  compat/posix.h       |  8 +++++++
  config.mak.uname     |  7 +++---
  configure.ac         | 17 +++++++++++++++
  daemon.c             | 52 +++++++++++++++++++++++++++++++++++++++-----
  meson.build          |  4 ++++
  7 files changed, 85 insertions(+), 10 deletions(-)

base-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v2
Pull-Request: https://github.com/git/git/pull/2002

Range-diff vs v1:

1: 2b5a58e53ac ! 1: e82b7425bbc compat/posix.h: track SA_RESTART fallback
@@ Metadata
## Commit message ##
compat/posix.h: track SA_RESTART fallback

- Systems without SA_RESTART where using custom CFLAGS instead of
- the standard header file.
+ Systems without SA_RESTART are using custom CFLAGS or headers
+ instead of the standard header file.

- Consolidate that, so it will be easier to use in a future commit.
+ Correct that, and invent a Makefile variable to track the
+ exceptions which will become handy in the next commits.

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

+ ## Makefile ##
+@@ Makefile: include shared.mak
+ # when attempting to read from an fopen'ed directory (or even to fopen
+ # it at all).
+ #
++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or ++# prefer to use ANSI C signal() over POSIX sigaction()
++#
+ # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR
+ # when a signal is received (as opposed to restarting).
+ #
+@@ Makefile: ifdef FREAD_READS_DIRECTORIES
+ COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
+ COMPAT_OBJS += compat/fopen.o
+ endif
++ifdef USE_NON_POSIX_SIGNAL
++ COMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL
++endif
+ ifdef OPEN_RETURNS_EINTR
+ COMPAT_CFLAGS += -DOPEN_RETURNS_EINTR
+ endif
+
+ ## compat/mingw-posix.h ##
+@@ compat/mingw-posix.h: struct sigaction {
+ sig_handler_t sa_handler;
+ unsigned sa_flags;
+ };
+-#define SA_RESTART 0
+
+ struct itimerval {
+ struct timeval it_value, it_interval;
+
## compat/posix.h ##
@@ compat/posix.h: char *gitdirname(char *);
#define NAME_MAX 255
#endif

-+/* On most systems <signal.h> would have given us this, but ++/*
++ * On most systems <signal.h> would have given us this, but
+ * not on some systems (e.g. NonStop, QNX).
+ */
+#ifndef SA_RESTART
-+#define SA_RESTART 0 /* disabled for sigaction() */
++# define SA_RESTART 0 /* disabled for sigaction() */
+#endif
+
typedef uintmax_t timestamp_t;
@@ compat/posix.h: char *gitdirname(char *);
#define parse_timestamp strtoumax

## config.mak.uname ##
+@@ config.mak.uname: ifeq ($(uname_S),Windows)
+ NO_STRTOUMAX = YesPlease
+ NO_MKDTEMP = YesPlease
+ NO_INTTYPES_H = YesPlease
++ USE_NON_POSIX_SIGNAL = YesPlease
+ CSPRNG_METHOD = rtlgenrandom
+ # VS2015 with UCRT claims that snprintf and friends are C99 compliant,
+ # so we don't need this:
@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
FREAD_READS_DIRECTORIES = UnfortunatelyYes

@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
# Apparently needed in compat/fnmatch/fnmatch.c. COMPAT_CFLAGS += -DHAVE_STRING_H=1 NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
+@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
+ NO_MMAP = YesPlease
+ NO_POLL = YesPlease
+ NO_INTPTR_T = UnfortunatelyYes
++ USE_NON_POSIX_SIGNAL = UnfortunatelyYes
+ CSPRNG_METHOD = openssl
+ SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
+ SHELL_PATH = /usr/coreutils/bin/bash
+@@ config.mak.uname: ifeq ($(uname_S),MINGW)
+ NEEDS_LIBICONV = YesPlease
+ NO_STRTOUMAX = YesPlease
+ NO_MKDTEMP = YesPlease
++ USE_NON_POSIX_SIGNAL = YesPlease
+ NO_SVN_TESTS = YesPlease
+
+ # The builtin FSMonitor requires Named Pipes and Threads on Windows.
@@ config.mak.uname: ifeq ($(uname_S),MINGW)
endif
endif
@@ config.mak.uname: ifeq ($(uname_S),MINGW)
EXPAT_NEEDS_XMLPARSE_H = YesPlease
HAVE_STRINGS_H = YesPlease
NEEDS_SOCKET = YesPlease
+@@ config.mak.uname: ifeq ($(uname_S),QNX)
+ NO_PTHREADS = YesPlease
+ NO_STRCASESTR = YesPlease
+ NO_STRLCPY = YesPlease
++ USE_NON_POSIX_SIGNAL = UnfortunatelyYes
+ endif
+
+ ## configure.ac ##
+@@ configure.ac: fi
+ GIT_CONF_SUBST([ICONV_OMITS_BOM])
+ fi
+
++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or ++# prefer using ANSI C signal() over POSIX sigaction()
++
++AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
++ AC_COMPILE_IFELSE(
++ [AC_LANG_PROGRAM([#include <signal.h>], [[
++ #ifdef SA_RESTART
++ #endif
++ siginterrupt(SIGCHLD, 1)
++ ]])],[ac_cv_siginterrupt=yes],[
++ ac_cv_siginterrupt=no
++ USE_NON_POSIX_SIGNAL=UnfortunatelyYes
++ ]
++ )
++])
++GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])
++
+ ## Checks for typedefs, structures, and compiler characteristics.
+ AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])
+ #
+
+ ## meson.build ##
+@@ meson.build: else
+ build_options_config.set('NO_EXPAT', '1')
+ endif
+
++if compiler.get_define('SA_RESTART', prefix: '#include <signal.h>') == '' ++ libgit_c_args += '-DUSE_NON_POSIX_SIGNAL'
++endif
++
+ if not compiler.has_header('sys/select.h')
+ libgit_c_args += '-DNO_SYS_SELECT_H'
+ endif
2: 2e8c4643a60 ! 2: 05d945aa1e5 daemon: use sigaction() to install child_handler()

      @@ Commit message
           In a future change, the flags used for processing SIGCHLD will need to
           be updated, which is only possible by using sigaction().
       
      -    Replace the call, which hs the added benefit of using BSD semantics
      -    reliably and therefore not needing the rearming call.
      +    Factor out the call to set the signal handler and use sigaction instead
      +    of signal for the systems that allow that, which has the added benefit
      +    of using BSD semantics reliably and therefore not needing the rearming
      +    call.
       
           Signed-off-by: Carlo Marcelo Arenas Belón <[email protected]>
       
        ## daemon.c ##
      -@@ daemon.c: static void child_handler(int signo UNUSED)
      +@@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
      + 		add_child(&cld, addr, addrlen);
      + }
      +
      +-static void child_handler(int signo UNUSED)
      ++static void child_handler(int signo MAYBE_UNUSED)
      + {
        	/*
      - 	 * Otherwise empty handler because systemcalls will get interrupted
      - 	 * upon signal receipt
      +-	 * Otherwise empty handler because systemcalls will get interrupted
      +-	 * upon signal receipt
       -	 * SysV needs the handler to be rearmed
      ++	 * Otherwise empty handler because systemcalls should get interrupted
      ++	 * upon signal receipt.
        	 */
       -	signal(SIGCHLD, child_handler);
      ++#ifdef USE_NON_POSIX_SIGNAL
      ++	/*
      ++	 * SysV needs the handler to be rearmed, but this is known
      ++	 * to trigger infinite recursion crashes at least in AIX.
      ++	 */
      ++	signal(signo, child_handler);
      ++#endif
        }
        
        static int set_reuse_addr(int sockfd)
       @@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s
      + 	}
      + }
      +
      ++#ifndef USE_NON_POSIX_SIGNAL
      ++
      ++static void set_signal_handler(struct sigaction *psa)
      ++{
      ++	sigemptyset(&psa->sa_mask);
      ++	psa->sa_flags = SA_NOCLDSTOP | SA_RESTART;
      ++	psa->sa_handler = child_handler;
      ++	sigaction(SIGCHLD, psa, NULL);
      ++}
      ++
      ++#else
      ++
      ++static void set_signal_handler(struct sigaction *psa UNUSED)
      ++{
      ++	signal(SIGCHLD, child_handler);
      ++}
      ++
        static int service_loop(struct socketlist *socklist)
        {
      - 	struct pollfd *pfd;
       +	struct sigaction sa;
      + 	struct pollfd *pfd;
        
        	CALLOC_ARRAY(pfd, socklist->nr);
      -
       @@ daemon.c: static int service_loop(struct socketlist *socklist)
        		pfd[i].events = POLLIN;
        	}
        
       -	signal(SIGCHLD, child_handler);
      -+	sigemptyset(&sa.sa_mask);
      -+	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
      -+	sa.sa_handler = child_handler;
      -+	sigaction(SIGCHLD, &sa, NULL);
      ++	set_signal_handler(&sa);
        
        	for (;;) {
        		check_dead_children();

3: a450bdb0066 ! 3: b737e0389df daemon: explicitly allow EINTR during poll()
@@ Commit message
might not return with -1 and set errno to EINTR when a signal is received.

- Since the logic to reap zombie childs relies om those interruptions
+ Since the logic to reap zombie childs relies on those interruptions
make sure to explicitly disable SA_RESTART around this function.

- Add a Makefile flag for portability to systems that don't have the
- functionality to change those flags or where it is not needed.
-

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

- ## Makefile ##
-@@ Makefile: include shared.mak
- # Define NO_PREAD if you have a problem with pread() system call (e.g.
- # cygwin1.dll before v1.5.22).
- #
-+# Define NO_SIGINTERRUPT if you don't have siginterrupt() or SA_RESTART -+# or if your signal(SIGCHLD) implementation doesn't set SA_RESTART.
-+#
- # Define NO_SETITIMER if you don't have setitimer()
- #
- # Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval
-@@ Makefile: ifdef NO_PREAD
- COMPAT_CFLAGS += -DNO_PREAD
- COMPAT_OBJS += compat/pread.o
- endif
-+ifdef NO_SIGINTERRUPT
-+ COMPAT_CFLAGS += -DNO_SIGINTERRUPT
-+endif
- ifdef NO_FAST_WORKING_DIRECTORY
- BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
- endif
-
- ## config.mak.uname ##
-@@ config.mak.uname: ifeq ($(uname_S),Windows)
- NO_STRTOUMAX = YesPlease
- NO_MKDTEMP = YesPlease
- NO_INTTYPES_H = YesPlease
-+ NO_SIGINTERRUPT = YesPlease
- CSPRNG_METHOD = rtlgenrandom
- # VS2015 with UCRT claims that snprintf and friends are C99 compliant,
- # so we don't need this:
-@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
- NO_PREAD = YesPlease
- NO_MMAP = YesPlease
- NO_POLL = YesPlease
-+ NO_SIGINTERRUPT = UnfortunatelyYes
- NO_INTPTR_T = UnfortunatelyYes
- CSPRNG_METHOD = openssl
- SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
-@@ config.mak.uname: ifeq ($(uname_S),MINGW)
- NEEDS_LIBICONV = YesPlease
- NO_STRTOUMAX = YesPlease
- NO_MKDTEMP = YesPlease
-+ NO_SIGINTERRUPT = YesPlease
- NO_SVN_TESTS = YesPlease
-
- # The builtin FSMonitor requires Named Pipes and Threads on Windows.
-@@ config.mak.uname: ifeq ($(uname_S),QNX)
- NO_PTHREADS = YesPlease
- NO_STRCASESTR = YesPlease
- NO_STRLCPY = YesPlease
-+ NO_SIGINTERRUPT = UnfortunatelyYes
- endif
-
- ## configure.ac ##
-@@ configure.ac: GIT_CHECK_FUNC(getdelim,
- [HAVE_GETDELIM=])
- GIT_CONF_SUBST([HAVE_GETDELIM])
- #
-+# Define NO_SIGINTERRUPT if you don't have siginterrupt. -+GIT_CHECK_FUNC(siginterrupt,
-+[NO_SIGINTERRUPT=],
-+[NO_SIGINTERRUPT=YesPlease])
-+GIT_CONF_SUBST([NO_SIGINTERRUPT])
- #
- # Define NO_MMAP if you want to avoid mmap.
- #
-
## daemon.c ##
-@@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
- add_child(&cld, addr, addrlen);
+@@ daemon.c: static void set_signal_handler(struct sigaction *psa)
+ sigaction(SIGCHLD, psa, NULL);
}

--static void child_handler(int signo UNUSED)
-+static void child_handler(int signo)
- {
- /*
-- * Otherwise empty handler because systemcalls will get interrupted
-- * upon signal receipt
-+ * Empty handler because systemcalls should get interrupted
-+ * upon signal receipt.
- */
-+#ifdef NO_SIGINTERRUPT
-+ /* SysV needs the handler to be rearmed */
-+ signal(signo, child_handler);
-+#endif
++static void set_sa_restart(struct sigaction *psa, int enable) ++{
++ if (enable)
++ psa->sa_flags |= SA_RESTART;
++ else
++ psa->sa_flags &= ~SA_RESTART;
++ sigaction(SIGCHLD, psa, NULL);
++}
++
+ #else
+
+ static void set_signal_handler(struct sigaction *psa UNUSED) +@@ daemon.c: static void set_signal_handler(struct sigaction *psa UNUSED)
+ signal(SIGCHLD, child_handler);
}

- static int set_reuse_addr(int sockfd)
-@@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s -
++static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED) ++{
++}
++
++#endif
++
static int service_loop(struct socketlist *socklist)
{
-- struct pollfd *pfd;
-+#ifndef NO_SIGINTERRUPT
struct sigaction sa;
-+#endif
-+ struct pollfd *pfd;
-
- CALLOC_ARRAY(pfd, socklist->nr);
-
@@ daemon.c: static int service_loop(struct socketlist *socklist)
- pfd[i].events = POLLIN;
- }
-
-+#ifdef NO_SIGINTERRUPT
-+ signal(SIGCHLD, child_handler);
-+#else
- sigemptyset(&sa.sa_mask);
- sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
- sa.sa_handler = child_handler;
- sigaction(SIGCHLD, &sa, NULL);
-+#endif
-
for (;;) {
check_dead_children();

-+#ifndef NO_SIGINTERRUPT
-+ sa.sa_flags &= ~SA_RESTART;
-+ sigaction(SIGCHLD, &sa, NULL);
-+#endif
++ set_sa_restart(&sa, 0);
if (poll(pfd, socklist->nr, -1) < 0) {
if (errno != EINTR) {
logerror("Poll failed, resuming: %s",
@@ daemon.c: static int service_loop(struct socketlist *socklist)
}
continue;
}
-+#ifndef NO_SIGINTERRUPT
-+ sa.sa_flags |= SA_RESTART;
-+ sigaction(SIGCHLD, &sa, NULL);
-+#endif
++ set_sa_restart(&sa, 1);

for (size_t i = 0; i < socklist->nr; i++) {
if (pfd[i].revents & POLLIN) {
-
- ## meson.build ##
-@@ meson.build: checkfuncs = {
- 'setenv' : ['setenv.c'],
- 'mkdtemp' : ['mkdtemp.c'],
- 'initgroups' : [],
-+ 'siginterrupt' : [],
- 'strtoumax' : ['strtoumax.c', 'strtoimax.c'],
- 'pread' : ['pread.c'],
- }

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

In a future change, the flags used for processing SIGCHLD will need to be updated, which is only possible by using sigaction().

Factor out the call to set the signal handler and use sigaction instead of signal for the systems that allow that, which has the added benefit of using BSD semantics reliably and therefore not needing the rearming call.

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
 daemon.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

daemon.c +10 -5

index d1be61fd5789..8133bd902157 100644
@@ -912,14 +912,19 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
 		add_child(&cld, addr, addrlen);
 }
 
-static void child_handler(int signo UNUSED)
+static void child_handler(int signo MAYBE_UNUSED)
 {
 	/*
-	 * Otherwise empty handler because systemcalls will get interrupted
-	 * upon signal receipt
-	 * SysV needs the handler to be rearmed
+	 * Otherwise empty handler because systemcalls should get interrupted
+	 * upon signal receipt.
 	 */
-	signal(SIGCHLD, child_handler);
+#ifdef USE_NON_POSIX_SIGNAL
+	/*
+	 * SysV needs the handler to be rearmed, but this is known
+	 * to trigger infinite recursion crashes at least in AIX.
+	 */
+	signal(signo, child_handler);
+#endif
 }
 
 static int set_reuse_addr(int sockfd)

daemon.c +4 -0

+static void set_signal_handler(struct sigaction *psa)
+{
+	sigemptyset(&psa->sa_mask);
+	psa->sa_flags = SA_NOCLDSTOP | SA_RESTART;

daemon.c +2 -1

 static int service_loop(struct socketlist *socklist)
 {
+	struct sigaction sa;
 	struct pollfd *pfd;
 
 	CALLOC_ARRAY(pfd, socklist->nr);

@@ -1129,7 +1152,7 @@ static int service_loop(struct socketlist *socklist)
 		pfd[i].events = POLLIN;
 	}
 
-	signal(SIGCHLD, child_handler);
+	set_signal_handler(&sa);
 
 	for (;;) {
 		check_dead_children();

+@@ Makefile: include shared.mak
+ # when attempting to read from an fopen'ed directory (or even to fopen
+ # it at all).
+ #
++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or ++# prefer to use ANSI C signal() over POSIX sigaction()
++#
...

     ++ifdef USE_NON_POSIX_SIGNAL
     ++	COMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL
     ++endif

configure.ac +7 -0

+		]])],[ac_cv_siginterrupt=yes],[
+			ac_cv_siginterrupt=no
+			USE_NON_POSIX_SIGNAL=UnfortunatelyYes
+		]
+	)
+])
+GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

If the setup for the SIGCHLD signal handler sets SA_RESTART, poll() might not return with -1 and set errno to EINTR when a signal is received.

Since the logic to reap zombie childs relies on those interruptions make sure to explicitly disable SA_RESTART around this function.

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
 daemon.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

daemon.c +9 -0

index 8133bd902157..01337fcfedab 100644
@@ -1133,6 +1133,15 @@ static void set_signal_handler(struct sigaction *psa)
 	sigaction(SIGCHLD, psa, NULL);
 }
 
+static void set_sa_restart(struct sigaction *psa, int enable)
+{
+	if (enable)
+		psa->sa_flags |= SA_RESTART;
+	else
+		psa->sa_flags &= ~SA_RESTART;
+	sigaction(SIGCHLD, psa, NULL);
+}
+
 #else
 
 static void set_signal_handler(struct sigaction *psa UNUSED)

Does this #ifndef block with #else lack its #endif probably before the service_loop() is defined ...
...
... around here?

I am wondering if it would make it even cleaner to also define rearm_signal_handler() in this "if sigaction() can be used, do this, otherwise do that" block, and move the whole thing a bit earlier in this file. That way, the primary code paths do not have to see much of the #ifdef conditionals. With IPV6 related #ifdef noise already contaminating the file, it may not be a huge deal, but these crufts tend to build up unless we tightly control them with discipline.

Quoted 3 lines · Carlo Marcelo Arenas Belón via GitGitGadget

This series addresses and ambiguity that is at least visible in OpenBSD, where zombie proceses would only be cleared after a new connection is received.

There is still a race where a child that exits after it has been checked in check_dead_children() but before we call poll() will not be collected until a new connection is received or a child exits while we're polling. If we used the self-pipe trick described on the select(2) man page [1] we would avoid that race and would not need to mess with SA_RESTART and so would not need to introduce USE_NON_POSIX_SIGNAL.

Best Wishes

Phillip

[1] https://www.man7.org/linux/man-pages/man2/select.2.html

Quoted 13 lines · Phillip Wood

Quoted 3 lines · Carlo Marcelo Arenas Belón via GitGitGadget

This series addresses and ambiguity that is at least visible in OpenBSD, where zombie proceses would only be cleared after a new connection is received.

There is still a race where a child that exits after it has been checked in check_dead_children() but before we call poll() will not be collected until a new connection is received or a child exits while we're polling. If we used the self-pipe trick described on the select(2) man page [1] we would avoid that race and would not need to mess with SA_RESTART and so would not need to introduce USE_NON_POSIX_SIGNAL.

Quoted 5 lines

Best Wishes

Phillip

[1] https://www.man7.org/linux/man-pages/man2/select.2.html

The principle should apply equally to poll-based service loop, I presume.

Thanks.

"Carlo Marcelo Arenas Belón via GitGitGadget"

configure.ac fi · line 873

Quoted 9 lines · <[email protected]> writes:

configure.ac +9 -0

+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
+# prefer using ANSI C signal() over POSIX sigaction()
+
+AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
+	AC_COMPILE_IFELSE(
+		[AC_LANG_PROGRAM([#include <signal.h>], [[
+		#ifdef SA_RESTART
+		#endif
+		siginterrupt(SIGCHLD, 1)

This is curious. What is this #ifdef/#endif doing that does not have anything in it?

Quoted 15 lines · Junio C Hamano

"Carlo Marcelo Arenas Belón via GitGitGadget"

configure.ac fi · line 873

Quoted 9 lines · <[email protected]> writes:

configure.ac +9 -0

+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
+# prefer using ANSI C signal() over POSIX sigaction()
+
+AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
+	AC_COMPILE_IFELSE(
+		[AC_LANG_PROGRAM([#include <signal.h>], [[
+		#ifdef SA_RESTART
+		#endif
+		siginterrupt(SIGCHLD, 1)

This is curious. What is this #ifdef/#endif doing that does not have anything in it?

It checks that SA_RESTART is defined in signal.h, which should fail at least in QNX, NonStop and Windows.

Quoted 17 lines · Carlo Marcelo Arenas Belón

Quoted 15 lines · Junio C Hamano

"Carlo Marcelo Arenas Belón via GitGitGadget"

configure.ac fi · line 873

Quoted 9 lines · <[email protected]> writes:

configure.ac +9 -0

+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
+# prefer using ANSI C signal() over POSIX sigaction()
+
+AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
+	AC_COMPILE_IFELSE(
+		[AC_LANG_PROGRAM([#include <signal.h>], [[
+		#ifdef SA_RESTART
+		#endif
+		siginterrupt(SIGCHLD, 1)

This is curious. What is this #ifdef/#endif doing that does not have anything in it?

Quoted 2 lines

It checks that SA_RESTART is defined in signal.h, which should fail at least in QNX, NonStop and Windows.

The above roughly expands to

        #include <signal.h>
        int main(void)
        {
                #ifdef SA_RESTART
                #endif
                siginterrupt(SIGCHLD, 1);
                return 0;
        }

Are you saying that a preprocessor macro SA_RESTART, which may or may not be defined, when asked by "#ifdef", causes what is left in the preprocessed source change in any meaningful way to cause the compilation to fail?

"Carlo Marcelo Arenas Belón via GitGitGadget"

Quoted 11 lines · <[email protected]> writes:

+@@ Makefile: include shared.mak
+ # when attempting to read from an fopen'ed directory (or even to fopen
+ # it at all).
+ #
++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or ++# prefer to use ANSI C signal() over POSIX sigaction()
++#
...

     ++ifdef USE_NON_POSIX_SIGNAL
     ++	COMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL
     ++endif

The new symbol sounds like "POSIX does not have signal(2) but on this platform we have a usable signal(2), so we use it here", but I do not think that it is what we want to say (as POSIX inherits this from ANSI C anyway). More importantly, this "USE_X" sounds as if we allow builders to set it and magically we stop using sigaction(2), which is not what is going on. We have tons of calls to both signal(2) and sigaction(2), and we turn calls to signal(2) we have in daemon.c to sigaction(2) but on some platforms their sigaction(2) cannot do what we ask it to do, so we are stuck with signal(2) on these platforms only for these calls in daemon.c. It may be obvious to those who develop and review this series, but not for anybody else.

Isn't the situation more like:

We use sigaction(2) everywhere and have been happy with it in our code, but this topic discovered that on some platforms, their sigaction(2) does not do XYZ that everybody else's sigaction(2) does, so on them we need to fall back on the plain old signal(2) on selected code paths that we need XYZ out of the signal handling interface.

What is this XYZ that describes the characteristics of signal/sigaction implementation on these platforms? A name constructed more like SIGACTION_LACKS_XYZ (hence we have to resort to signal), possibly with a more appropriate verb than "lack", would be less confusing.

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

A future change will start using sigaction to setup a SIGCHLD signal handler.

The current code uses signal() which returns SIG_ERR (but doesn't seem to set errno) so instruct sigaction() to do the same.

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
  compat/mingw-posix.h | 1 +
  compat/mingw.c       | 4 +++-
  2 files changed, 4 insertions(+), 1 deletion(-)

compat/mingw-posix.h +1 -0

index a0dca756d104..847d558c9b2d 100644
@@ -95,6 +95,7 @@ struct sigaction {
  	sig_handler_t sa_handler;
  	unsigned sa_flags;
  };
+#define SA_NOCLDSTOP 1
 
  struct itimerval {
  	struct timeval it_value, it_interval;

compat/mingw.c +3 -1

index 8a9972a1ca19..5d69ae32f4b9 100644
@@ -2561,7 +2561,9 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)
 
  int sigaction(int sig, struct sigaction *in, struct sigaction *out)
  {
-	if (sig != SIGALRM)
+	if (sig == SIGCHLD)
+		return -1;
+	else if (sig != SIGALRM)
  		return errno = EINVAL,
  			error("sigaction only implemented for SIGALRM");
  	if (out)

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

In a future change, the flags used for processing SIGCHLD will need to be updated, which is only possible by using sigaction().

Replace signal() with an equivalent invocation of sigaction(), which has the added benefit of using BSD semantics reliably and therefore not needing the rearming call in the signal handler.

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
  daemon.c | 12 +++++++-----
  1 file changed, 7 insertions(+), 5 deletions(-)

daemon.c +7 -5

index d1be61fd5789..155b2e180167 100644
@@ -915,11 +915,9 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
  static void child_handler(int signo UNUSED)
  {
  	/*
-	 * Otherwise empty handler because systemcalls will get interrupted
-	 * upon signal receipt
-	 * SysV needs the handler to be rearmed
+	 * Otherwise empty handler because systemcalls should get interrupted
+	 * upon signal receipt.
  	 */
-	signal(SIGCHLD, child_handler);
  }
 
  static int set_reuse_addr(int sockfd)

@@ -1120,6 +1118,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
 
  static int service_loop(struct socketlist *socklist)
  {
+	struct sigaction sa;
  	struct pollfd *pfd;
 
  	CALLOC_ARRAY(pfd, socklist->nr);

@@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)
  		pfd[i].events = POLLIN;
  	}
 
-	signal(SIGCHLD, child_handler);
+	sigemptyset(&sa.sa_mask);
+	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
+	sa.sa_handler = child_handler;
+	sigaction(SIGCHLD, &sa, NULL);
 
  	for (;;) {
  		check_dead_children();

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

If the setup for the SIGCHLD signal handler sets SA_RESTART, poll() might not return with -1 and set errno to EINTR when a signal is received.

Since the logic to reap zombie childs relies on those interruptions make sure to explicitly disable SA_RESTART around this function.

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
  daemon.c | 21 +++++++++++++++++++++
  1 file changed, 21 insertions(+)

daemon.c +21 -0

index 155b2e180167..7e29c03e313f 100644
@@ -1116,6 +1116,25 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
  	}
  }
 
+#ifndef NO_RESTARTABLE_SIGNALS
+
+static void set_sa_restart(struct sigaction *psa, int enable)
+{
+	if (enable)
+		psa->sa_flags |= SA_RESTART;
+	else
+		psa->sa_flags &= ~SA_RESTART;
+	sigaction(SIGCHLD, psa, NULL);
+}
+
+#else
+
+static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)
+{
+}
+
+#endif
+
  static int service_loop(struct socketlist *socklist)
  {
  	struct sigaction sa;

@@ -1136,6 +1155,7 @@ static int service_loop(struct socketlist *socklist)
  	for (;;) {
  		check_dead_children();
 
+		set_sa_restart(&sa, 0);
  		if (poll(pfd, socklist->nr, -1) < 0) {
  			if (errno != EINTR) {
  				logerror("Poll failed, resuming: %s",

@@ -1144,6 +1164,7 @@ static int service_loop(struct socketlist *socklist)
  			}
  			continue;
  		}
+		set_sa_restart(&sa, 1);
 
  		for (size_t i = 0; i < socklist->nr; i++) {
  			if (pfd[i].revents & POLLIN) {

Quoted 7 lines · Carlo Marcelo Arenas Belón via GitGitGadget

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

A future change will start using sigaction to setup a SIGCHLD signal handler.

The current code uses signal() which returns SIG_ERR (but doesn't seem to set errno) so instruct sigaction() to do the same.

Why are we returning -1 below instead of SIG_ERR if we want the behavior to match?

Quoted 11 lines · Phillip Wood

Quoted 7 lines · Carlo Marcelo Arenas Belón via GitGitGadget

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

A future change will start using sigaction to setup a SIGCHLD signal handler.

The current code uses signal() which returns SIG_ERR (but doesn't seem to set errno) so instruct sigaction() to do the same.

Why are we returning -1 below instead of SIG_ERR if we want the behavior to match?

By "match", I mean that in both cases we will get an error return value and errno won't be set to EINVAL (which is what POSIX requires)

In our codebase since we ignore the return code anyway, it wouldn't make a difference, either way.

signal() returns a pointer, and sigaction() returns and int,

so you can
have the later be literally SIG_ERR, eventhough it will be ironically equivalent it casted into an int.

Csrlo

Quoted 19 lines · Carlo Marcelo Arenas Belón

Quoted 11 lines · Phillip Wood

Quoted 7 lines · Carlo Marcelo Arenas Belón via GitGitGadget

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

A future change will start using sigaction to setup a SIGCHLD signal handler.

The current code uses signal() which returns SIG_ERR (but doesn't seem to set errno) so instruct sigaction() to do the same.

Why are we returning -1 below instead of SIG_ERR if we want the behavior to match?

By "match", I mean that in both cases we will get an error return value and errno won't be set to EINVAL (which is what POSIX requires)

In our codebase since we ignore the return code anyway, it wouldn't make a difference, either way.

Quoted 1 line

signal() returns a pointer, and sigaction() returns and int,

Oh right, I'd forgotten they have different return types. I think we should probably be setting errno = EINVAL before returning -1 to match what this function does with other signals it does not support - just because our current callers ignore the return value doesn't mean that future callers will and they might want check errno if they see the function fail.

Quoted 20 lines · Phillip Wood

Quoted 19 lines · Carlo Marcelo Arenas Belón

Quoted 11 lines · Phillip Wood

Quoted 7 lines · Carlo Marcelo Arenas Belón via GitGitGadget

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

A future change will start using sigaction to setup a SIGCHLD signal handler.

The current code uses signal() which returns SIG_ERR (but doesn't seem to set errno) so instruct sigaction() to do the same.

Why are we returning -1 below instead of SIG_ERR if we want the behavior to match?

By "match", I mean that in both cases we will get an error return value and errno won't be set to EINVAL (which is what POSIX requires)

In our codebase since we ignore the return code anyway, it wouldn't make a difference, either way.

Quoted 7 lines

Quoted 1 line

signal() returns a pointer, and sigaction() returns and int,

Oh right, I'd forgotten they have different return types. I think we should probably be setting errno = EINVAL before returning -1 to match what this function does with other signals it does not support - just because our current callers ignore the return value doesn't mean that future callers will and they might want check errno if they see the function fail.

I agree, and indeed had to triple check and change my implementation after I confirmed that signal(SIGCHLD) does not change errno on Windows (not our version, neither of the windows libc or mingw, even if it is documented[1] to do so.

It might be because the signal number itself is bogus (there is none for SIGCHLD in their headers, and git uses their own numbers in compat), but either way, I would rather be consistent with signal() at least originally.

Carlo

[1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal

Only because you have chosen to use SA_RESTART here. I think it would be better to drop that and instead say something like

POSIX leaves several aspects of the behavior of signal() up to the implementer. It is unspecified whether long running syscalls are restarted after an interrupt is received. The event loop in "git daemon" assumes that poll() will return with EINTR if a child exits while it is polling but if poll() is restarted after an interrupt is received that does not happen which can lead to a build up of uncollected zombie processes.

It is also unspecified whether the handler is reset after an interrupt is received. In order to work correctly on operating systems such as Solaris where the handler for SIGCLD is reset when a signal is received "git daemon" calls signal() from the SIGCLD handler to re-establish a handler for that signal. Unfortunately this causes infinite recursion on other operating systems such as AIX.

Both of these problems can be addressed by using sigaction() instead of signal() which has much stricter semantics and so, by setting the appropriate flags, we can rely on poll() being interrupted and the SIGCLD handler not being reset. This change means that all long running system calls could fail with EINTR not just pall() but rest of the event loop code is designed to gracefully handle that.

After this change there is still a race where a child that exits after it has been checked in check_dead_children() but before we call poll() will not be collected until a new connection is received or a child exits while we're polling. We could fix this by using the "self-pipe trick" but do not because ....

Then we can drop patches 1 and 4.

Quoted 33 lines · Phillip Wood

Only because you have chosen to use SA_RESTART here. I think it would be better to drop that and instead say something like

POSIX leaves several aspects of the behavior of signal() up to the implementer. It is unspecified whether long running syscalls are restarted after an interrupt is received. The event loop in "git daemon" assumes that poll() will return with EINTR if a child exits while it is polling but if poll() is restarted after an interrupt is received that does not happen which can lead to a build up of uncollected zombie processes.

It is also unspecified whether the handler is reset after an interrupt is received. In order to work correctly on operating systems such as Solaris where the handler for SIGCLD is reset when a signal is received "git daemon" calls signal() from the SIGCLD handler to re-establish a handler for that signal. Unfortunately this causes infinite recursion on other operating systems such as AIX.

Both of these problems can be addressed by using sigaction() instead of signal() which has much stricter semantics and so, by setting the appropriate flags, we can rely on poll() being interrupted and the SIGCLD handler not being reset. This change means that all long running system calls could fail with EINTR not just pall() but rest of the event loop code is designed to gracefully handle that.

After this change there is still a race where a child that exits after it has been checked in check_dead_children() but before we call poll() will not be collected until a new connection is received or a child exits while we're polling. We could fix this by using the "self-pipe trick" but do not because ....

Quoted 1 line

Then we can drop patches 1 and 4.

Thanks for a suggestion. I really like the "everybody in these code paths are prepared to receive and handle EINTR just fine, so we do not have to do the SA_RESTART" observation very much.

Quoted 5 lines · Junio C Hamano

Phillip Wood <[email protected]> writes:

Thanks for a suggestion. I really like the "everybody in these code paths are prepared to receive and handle EINTR just fine, so we do not have to do the SA_RESTART" observation very much.

Except that is unlikely to be true, as the code has changed a lot on those 20 years (including that it now uses run_command) and that we had been effectively using SA_RESTART under the covers for most of that time because signal() behaviour changed. An obvious bug:
(ex: [email protected])

I think using SA_RESTART by default might be safer,

Quoted 34 lines · Carlo Marcelo Arenas Belón

Quoted 20 lines · Phillip Wood

Quoted 19 lines · Carlo Marcelo Arenas Belón

Quoted 11 lines · Phillip Wood

Quoted 7 lines · Carlo Marcelo Arenas Belón via GitGitGadget

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

A future change will start using sigaction to setup a SIGCHLD signal handler.

The current code uses signal() which returns SIG_ERR (but doesn't seem to set errno) so instruct sigaction() to do the same.

Why are we returning -1 below instead of SIG_ERR if we want the behavior to match?

By "match", I mean that in both cases we will get an error return value and errno won't be set to EINVAL (which is what POSIX requires)

In our codebase since we ignore the return code anyway, it wouldn't make a difference, either way.

Quoted 7 lines

Quoted 1 line

signal() returns a pointer, and sigaction() returns and int,

Oh right, I'd forgotten they have different return types. I think we should probably be setting errno = EINVAL before returning -1 to match what this function does with other signals it does not support - just because our current callers ignore the return value doesn't mean that future callers will and they might want check errno if they see the function fail.

I agree, and indeed had to triple check and change my implementation after I confirmed that signal(SIGCHLD) does not change errno on Windows (not our version, neither of the windows libc or mingw, even if it is documented[1] to do so.

Quoted 3 lines

It might be because the signal number itself is bogus (there is none for SIGCHLD in their headers, and git uses their own numbers in compat), but either way, I would rather be consistent with signal() at least originally.

I'm not sure I understand - don't we want the sigaction() wrapper to behave like sigaction() would?

Quoted 2 lines

[1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal

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().

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.

Suggested=by: Phillip Wood <[email protected]>

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

---
Implements the "self pipe" trick Hillip suggested.

I tried to make self healing and optional, as I also presume the race condition it is meant to address is very unlikely.

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.

daemon.c +15 -4

+
+			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);
 

daemon.c +10 -0

+				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--;
  			}
  		}
  	}

Quoted 4 lines · Carlo Marcelo Arenas Belón

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 static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen) · line 914

Quoted 9 lines

daemon.c +1 -1

index d1be61fd57..d3b9421575 100644
@@ -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.

daemon.c static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen) · line 922

Quoted 9 lines

daemon.c +2 -2

+
+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.

daemon.c static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen) · line 925

Quoted 3 lines

daemon.c +3 -0

+
+	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.

Quoted 6 lines

daemon.c +0 -0

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

daemon.c static void socksetup(struct string_list *listen_addr, int listen_port, struct s · line 1130

Quoted 4 lines

daemon.c +4 -0

+	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().

daemon.c static void socksetup(struct string_list *listen_addr, int listen_port, struct s · line 1136

Quoted 5 lines

daemon.c +5 -0

+			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.

Quoted 10 lines

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.

Quoted 14 lines · Phillip Wood

daemon.c static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen) · line 914

Quoted 10 lines · Carlo Marcelo Arenas Belón

daemon.c +1 -1

index d1be61fd57..d3b9421575 100644
@@ -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.

Quoted 12 lines

daemon.c static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen) · line 922

Quoted 9 lines

daemon.c +2 -2

+
+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.

It really depends on which flags the signal was expected to use. For maximum portability it would seem better to have it at the beginning to minimize the inherent race condition from when SA_RESETHAND is on effect (which was more of a SysV signal() tradition).

Will move it to the end, regardless, as it won't be needed once the call is setup using sigaction().

Quoted 2 lines

As I said above we could add an additional commit that moves this into the event loop to fix the infinite recursion on AIX.

Not sure I understant what you mean by this.

I think Chris made clear that
the AIX issue comes from the handler being expected to do the wait() calls,

Quoted 8 lines

daemon.c static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen) · line 925

Quoted 3 lines

daemon.c +3 -0

+
+	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.

EINTR shouldn't be possible here from SIGCHLD, because that signal is blocked here,

a fallback to the EINTR being triggered in poll() and so it wouldn't matter if it was lost (for example because of a SIGPIPE).

Once we move to sigaction, SIGPIPE could be added to the mask for this handler, but that code can't be implemented on the current base.

Quoted 6 lines

Quoted 6 lines

daemon.c +0 -0

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

Quoted 8 lines

daemon.c static void socksetup(struct string_list *listen_addr, int listen_port, struct s · line 1130

Quoted 4 lines

daemon.c +4 -0

+	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().

True, but since this is "optional", there is no harm on letting this continue even in failure.

The counter argument to this is that it is masking bugs that happen on platforms without SA_RESTART.

This series addresses and ambiguity that is at least visible in OpenBSD, where zombie proceses would only be cleared after a new connection is received.

The underlying problem is that when this code was originally introduced, SA_RESTART was not widely implemented, and the signal() call usually implemented SysV like semantics, at least until it started being reimplemented by calling sigaction() internally.

Changes since v2

* Add a new patch 2 that modifies windows' sigaction so there is no more need for a fallback
* Hopefully no more silly mistakes and a variable that finally makes sense

Changes since v1

* Almost all references to siginterrupt has been removed and a better named variable used instead
* Changes had been abstracted to minimize ifdefs and their introduction staged more naturally

Carlo Marcelo Arenas Belón (4):
compat/posix.h: track SA_RESTART fallback compat/mingw: allow sigaction(SIGCHLD)
daemon: use sigaction() to install child_handler()
daemon: explicitly allow EINTR during poll()

  Makefile             |  5 +++++
  compat/mingw-posix.h |  2 +-
  compat/mingw.c       |  4 +++-
  compat/posix.h       |  8 ++++++++
  config.mak.uname     |  7 ++++---
  configure.ac         | 16 ++++++++++++++++
  daemon.c             | 33 ++++++++++++++++++++++++++++-----
  meson.build          |  4 ++++
  8 files changed, 69 insertions(+), 10 deletions(-)

base-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v3
Pull-Request: https://github.com/git/git/pull/2002

Range-diff vs v2:

1: e82b7425bbc ! 1: ae1ca6bb2b2 compat/posix.h: track SA_RESTART fallback
@@ Makefile: include shared.mak
# when attempting to read from an fopen'ed directory (or even to fopen # it at all).
#
-+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or -+# prefer to use ANSI C signal() over POSIX sigaction()
++# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART
+#
# Define OPEN_RETURNS_EINTR if your open() system call may return EINTR # when a signal is received (as opposed to restarting).
@@ Makefile: ifdef FREAD_READS_DIRECTORIES
COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
COMPAT_OBJS += compat/fopen.o
endif
-+ifdef USE_NON_POSIX_SIGNAL
-+ COMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL
++ifdef NO_RESTARTABLE_SIGNALS
++ COMPAT_CFLAGS += -DNO_RESTARTABLE_SIGNALS
+endif
ifdef OPEN_RETURNS_EINTR
COMPAT_CFLAGS += -DOPEN_RETURNS_EINTR
@@ compat/posix.h: char *gitdirname(char *);
+ * not on some systems (e.g. NonStop, QNX).
+ */
+#ifndef SA_RESTART
-+# define SA_RESTART 0 /* disabled for sigaction() */
++# define SA_RESTART 0 /* disabled for sigaction() */
+#endif
+
typedef uintmax_t timestamp_t;
@@ config.mak.uname: ifeq ($(uname_S),Windows)
NO_STRTOUMAX = YesPlease
NO_MKDTEMP = YesPlease
NO_INTTYPES_H = YesPlease
-+ USE_NON_POSIX_SIGNAL = YesPlease
++ NO_RESTARTABLE_SIGNALS = YesPlease
CSPRNG_METHOD = rtlgenrandom
# VS2015 with UCRT claims that snprintf and friends are C99 compliant, # so we don't need this:
@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
NO_MMAP = YesPlease
NO_POLL = YesPlease
NO_INTPTR_T = UnfortunatelyYes
-+ USE_NON_POSIX_SIGNAL = UnfortunatelyYes
++ NO_RESTARTABLE_SIGNALS = UnfortunatelyYes
CSPRNG_METHOD = openssl
SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin SHELL_PATH = /usr/coreutils/bin/bash
@@ config.mak.uname: ifeq ($(uname_S),MINGW)
NEEDS_LIBICONV = YesPlease
NO_STRTOUMAX = YesPlease
NO_MKDTEMP = YesPlease
-+ USE_NON_POSIX_SIGNAL = YesPlease
++ NO_RESTARTABLE_SIGNALS = YesPlease
NO_SVN_TESTS = YesPlease

# The builtin FSMonitor requires Named Pipes and Threads on Windows.
@@ config.mak.uname: ifeq ($(uname_S),QNX)
NO_PTHREADS = YesPlease
NO_STRCASESTR = YesPlease
NO_STRLCPY = YesPlease
-+ USE_NON_POSIX_SIGNAL = UnfortunatelyYes
++ NO_RESTARTABLE_SIGNALS = UnfortunatelyYes
endif

## configure.ac ##
@@ configure.ac: fi
GIT_CONF_SUBST([ICONV_OMITS_BOM])
fi

-+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or -+# prefer using ANSI C signal() over POSIX sigaction()
++# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART
+
+AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
+ AC_COMPILE_IFELSE(
+ [AC_LANG_PROGRAM([#include <signal.h>], [[
-+ #ifdef SA_RESTART
-+ #endif
-+ siginterrupt(SIGCHLD, 1)
-+ ]])],[ac_cv_siginterrupt=yes],[
++ #ifdef SA_RESTART
++ restartable signals supported
++ #endif
++ ]])],[
+ ac_cv_siginterrupt=no
-+ USE_NON_POSIX_SIGNAL=UnfortunatelyYes
-+ ]
++ NO_RESTARTABLE_SIGNALS=UnfortunatelyYes
++ ], [ac_cv_siginterrupt=yes]
+ )
+])
-+GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL]) ++GIT_CONF_SUBST([NO_RESTARTABLE_SIGNALS])
+
## Checks for typedefs, structures, and compiler characteristics. AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])
@@ meson.build: else
endif

+if compiler.get_define('SA_RESTART', prefix: '#include <signal.h>') == ''
-+ libgit_c_args += '-DUSE_NON_POSIX_SIGNAL'
++ libgit_c_args += '-DNO_RESTARTABLE_SIGNALS'
+endif
+
if not compiler.has_header('sys/select.h')

  -:  ----------- > 2:  3f63479119f compat/mingw: allow sigaction(SIGCHLD)
  2:  05d945aa1e5 ! 3:  c66bda461f4 daemon: use sigaction() to install child_handler()
      @@ Commit message
           In a future change, the flags used for processing SIGCHLD will need to
           be updated, which is only possible by using sigaction().
       
      -    Factor out the call to set the signal handler and use sigaction instead
      -    of signal for the systems that allow that, which has the added benefit
      -    of using BSD semantics reliably and therefore not needing the rearming
      -    call.
      +    Replace signal() with an equivalent invocation of sigaction(), which
      +    has the added benefit of using BSD semantics reliably and therefore
      +    not needing the rearming call in the signal handler.
       
           Signed-off-by: Carlo Marcelo Arenas Belón <[email protected]>
       
        ## daemon.c ##
       @@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
      - 		add_child(&cld, addr, addrlen);
      - }
      -
      --static void child_handler(int signo UNUSED)
      -+static void child_handler(int signo MAYBE_UNUSED)
      + static void child_handler(int signo UNUSED)
        {
        	/*
       -	 * Otherwise empty handler because systemcalls will get interrupted
      @@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addr
       +	 * upon signal receipt.
        	 */
       -	signal(SIGCHLD, child_handler);
      -+#ifdef USE_NON_POSIX_SIGNAL
      -+	/*
      -+	 * SysV needs the handler to be rearmed, but this is known
      -+	 * to trigger infinite recursion crashes at least in AIX.
      -+	 */
      -+	signal(signo, child_handler);
      -+#endif
        }
        
        static int set_reuse_addr(int sockfd)
       @@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s
      - 	}
      - }
        
      -+#ifndef USE_NON_POSIX_SIGNAL
      -+
      -+static void set_signal_handler(struct sigaction *psa)
      -+{
      -+	sigemptyset(&psa->sa_mask);
      -+	psa->sa_flags = SA_NOCLDSTOP | SA_RESTART;
      -+	psa->sa_handler = child_handler;
      -+	sigaction(SIGCHLD, psa, NULL);
      -+}
      -+
      -+#else
      -+
      -+static void set_signal_handler(struct sigaction *psa UNUSED)
      -+{
      -+	signal(SIGCHLD, child_handler);
      -+}
      -+
        static int service_loop(struct socketlist *socklist)
        {
       +	struct sigaction sa;
      @@ daemon.c: static int service_loop(struct socketlist *socklist)
        	}
        
       -	signal(SIGCHLD, child_handler);
      -+	set_signal_handler(&sa);
      ++	sigemptyset(&sa.sa_mask);
      ++	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
      ++	sa.sa_handler = child_handler;
      ++	sigaction(SIGCHLD, &sa, NULL);
        
        	for (;;) {
        		check_dead_children();

3: b737e0389df ! 4: 851d663be0b daemon: explicitly allow EINTR during poll()
@@ Commit message

Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>

## daemon.c ##
-@@ daemon.c: static void set_signal_handler(struct sigaction *psa)
- sigaction(SIGCHLD, psa, NULL);
+@@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s
+ }
}

++#ifndef NO_RESTARTABLE_SIGNALS
++
+static void set_sa_restart(struct sigaction *psa, int enable) +{
+ if (enable)
@@ daemon.c: static void set_signal_handler(struct sigaction *psa)
+ sigaction(SIGCHLD, psa, NULL);
+}
+
- #else
-
- static void set_signal_handler(struct sigaction *psa UNUSED) -@@ daemon.c: static void set_signal_handler(struct sigaction *psa UNUSED)
- signal(SIGCHLD, child_handler);
- }
-
++#else
++
+static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED) +{
+}

Quoted 39 lines · On Thu, Jun 26, 2025 at 04:19:11PM -0800, [email protected] wrote:

Quoted 37 lines · Carlo Marcelo Arenas Belón

Quoted 20 lines · Phillip Wood

Quoted 19 lines · Carlo Marcelo Arenas Belón

Quoted 11 lines · Phillip Wood

Quoted 7 lines · Carlo Marcelo Arenas Belón via GitGitGadget

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <[email protected]>

A future change will start using sigaction to setup a SIGCHLD signal handler.

The current code uses signal() which returns SIG_ERR (but doesn't seem to set errno) so instruct sigaction() to do the same.

Why are we returning -1 below instead of SIG_ERR if we want the behavior to match?

By "match", I mean that in both cases we will get an error return value and errno won't be set to EINVAL (which is what POSIX requires)

In our codebase since we ignore the return code anyway, it wouldn't make a difference, either way.

Quoted 7 lines

Quoted 1 line

signal() returns a pointer, and sigaction() returns and int,

Oh right, I'd forgotten they have different return types. I think we should probably be setting errno = EINVAL before returning -1 to match what this function does with other signals it does not support - just because our current callers ignore the return value doesn't mean that future callers will and they might want check errno if they see the function fail.

I agree, and indeed had to triple check and change my implementation after I confirmed that signal(SIGCHLD) does not change errno on Windows (not our version, neither of the windows libc or mingw, even if it is documented[1] to do so.

It might be because the signal number itself is bogus (there is none for SIGCHLD in their headers, and git uses their own numbers in compat), but either way, I would rather be consistent with signal() at least originally.