[PATCH v3 0/4] 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 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 <carenas@gmail.com>
      
       ## 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 <carenas@gmail.com>
      
       ## 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)
      +{
      +}

-- 
gitgitgadget