[PATCH v2 0/3] daemon: explicitly allow EINTR during poll()

["Carlo Marcelo Arenas Belón via GitGitGadget" <gitgitgadget@gmail.com>]

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 <carenas@gmail.com>
      
     + ## 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 <carenas@gmail.com>
      
       ## 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 <carenas@gmail.com>
      
     - ## 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'],
     - }

-- 
gitgitgadget