On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:

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.

I'm all in favor of using sigaction() but I think the SA_RESTART parts 
of this series are an unnecessary complication that has the potential to 
hide bugs as we support platforms without SA_RESTART. Your EINTR patches 
have all been about efficiency rather than correctness so I think my 
initial assessment that the existing code handles EINTR correctly was 
probably true. As I said in my comments on patch 3 I think we can 
happily drop patches 1 and 4.

Thanks

Phillip

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 [off-list ref]
       
        ## 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 [off-list ref]
       
        ## 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)
       +{
       +}