[PATCH 4/5] linux: Optimize posix_spawn spurious sigaction calls

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

The child helper process on Linux posix_spawn child must ensure that no signal
handler are enabled, so the signal disposition must be either SIG_DFL or
SIG_IGN.  However, it requires a sigprocmask to obtain the current signal mask
and at least _NSIG sigaction calls to reset the signal handlers for each
posix_spawn call.

This patch optimizes it by tracking on sigaction implementation when a signal
action is set different than SIG_DFL or SIG_IGN.  It allows remove a
sigprocmask call and isse sigaction to reset the disposition only the signals
that has non-default actions set.

It might incur in false positive, since it not easy to remove bits from the
mask without race conditions, but it does not allow false negative since the
mask is updated atomically prior the syscall.  The false positive incur in
just extra sigactions on posix_spawn.

Checked on x86_64 and i686.

	* include/atomic.h (atomic_fetch_or_seq_cst, atomic_fetch_or_seq_cst):
	New macros.
	* posix/Makefile (tests): Add tst-spawn6.
	* posix/tst-spawn6.c: New file.
	* sysdeps/generic/sigsetops.h (__sigorset_atomic): New macro.
	* sysdeps/unix/sysv/linux/internal-signals.h (__get_sighandler_set):
	New prototype.
	* sysdeps/unix/sysv/linux/sigaction.c (__get_sighandler_set): New
	function.
	(__libc_sigaction): Set the internal handler_set for a new action.
	* sysdeps/unix/sysv/linux/sigsetops.h (__sigorset_atomic,
	__sigaddset_atomic): New macros.
	* sysdeps/unix/sysv/linux/spawni.c (spawni_child): Replace
	__sigprocmask with __get_sighandler_set.
---
 include/atomic.h                           |  10 +
 posix/Makefile                             |   4 +-
 posix/tst-spawn6.c                         | 220 +++++++++++++++++++++
 sysdeps/generic/sigsetops.h                |   7 +
 sysdeps/unix/sysv/linux/internal-signals.h |   3 +
 sysdeps/unix/sysv/linux/sigaction.c        |  17 ++
 sysdeps/unix/sysv/linux/sigsetops.h        |  15 ++
 sysdeps/unix/sysv/linux/spawni.c           |   9 +-
 8 files changed, 278 insertions(+), 7 deletions(-)
 create mode 100644 posix/tst-spawn6.c

diff --git a/include/atomic.h b/include/atomic.h
index ee1978eb3b..72609efde9 100644
--- a/include/atomic.h
+++ b/include/atomic.h
@@ -646,6 +646,9 @@ void __atomic_link_error (void);
 # define atomic_fetch_or_release(mem, operand) \
   ({ __atomic_check_size((mem));					      \
   __atomic_fetch_or ((mem), (operand), __ATOMIC_RELEASE); })
+# define atomic_fetch_or_seq_cst(mem, operand) \
+  ({ __atomic_check_size((mem));					      \
+  __atomic_fetch_or ((mem), (operand), __ATOMIC_SEQ_CST); })
 
 # define atomic_fetch_xor_release(mem, operand) \
   ({ __atomic_check_size((mem));					      \
@@ -791,6 +794,13 @@ void __atomic_link_error (void);
    ({ atomic_thread_fence_release ();					      \
    atomic_fetch_or_acquire ((mem), (operand)); })
 # endif
+# ifndef atomic_fetch_or_seq_cst
+#  define atomic_fetch_or_seq_cst(mem, operand) \
+   ({ atomic_thread_fence_acquire ();					      \
+   atomic_fetch_or_relaxed ((mem), (operand));				      \
+   atomic_thread_fence_release (); })
+# endif
+
 
 # ifndef atomic_fetch_xor_release
 /* Failing the atomic_compare_exchange_weak_release reloads the value in
diff --git a/posix/Makefile b/posix/Makefile
index 1ac41ad85a..131ae052fd 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -102,7 +102,8 @@ tests		:= test-errno tstgetopt testfnm runtests runptests \
 		   tst-sysconf-empty-chroot tst-glob_symlinks tst-fexecve \
 		   tst-glob-tilde test-ssize-max tst-spawn4 bug-regex37 \
 		   bug-regex38 tst-regcomp-truncated tst-spawn-chdir \
-		   tst-spawn5
+		   tst-spawn5 \
+		   tst-spawn6
 tests-internal	:= bug-regex5 bug-regex20 bug-regex33 \
 		   tst-rfc3484 tst-rfc3484-2 tst-rfc3484-3 \
 		   tst-glob_lstat_compat tst-spawn4-compat
@@ -255,6 +256,7 @@ tst-exec-ARGS = -- $(host-test-program-cmd)
 tst-exec-static-ARGS = $(tst-exec-ARGS)
 tst-execvpe5-ARGS = -- $(host-test-program-cmd)
 tst-spawn-ARGS = -- $(host-test-program-cmd)
+tst-spawn6-ARGS = -- $(host-test-program-cmd)
 tst-spawn-static-ARGS = $(tst-spawn-ARGS)
 tst-spawn5-ARGS = -- $(host-test-program-cmd)
 tst-dir-ARGS = `pwd` `cd $(common-objdir)/$(subdir); pwd` `cd $(common-objdir); pwd` $(objpfx)tst-dir
diff --git a/posix/tst-spawn6.c b/posix/tst-spawn6.c
new file mode 100644
index 0000000000..466e66f104
--- /dev/null
+++ b/posix/tst-spawn6.c
@@ -0,0 +1,220 @@
+/* Tests for posix_spawn signal handling.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <getopt.h>
+#include <spawn.h>
+#include <sys/wait.h>
+
+#include <support/check.h>
+#include <support/xunistd.h>
+#include <support/support.h>
+#include <array_length.h>
+
+/* Nonzero if the program gets called via `exec'.  */
+static int restart;
+#define CMDLINE_OPTIONS \
+  { "restart", no_argument, &restart, 1 },
+
+enum spawn_test_t
+{
+  SPAWN_SETSIGMASK,
+  SPAWN_SETSIGDEF
+};
+
+static int signal_to_check[] =
+{
+  SIGHUP, SIGINT, SIGALRM, SIGUSR2
+};
+
+/* Called on process re-execution.  */
+static int
+handle_restart (enum spawn_test_t test)
+{
+  switch (test)
+    {
+    case SPAWN_SETSIGMASK:
+      {
+	sigset_t mask;
+	sigprocmask (SIG_BLOCK, NULL, &mask);
+	for (int i = 0; i < array_length (signal_to_check); i++)
+	  if (sigismember (&mask, signal_to_check[i]) != 1)
+	    exit (EXIT_FAILURE);
+      }
+      break;
+    case SPAWN_SETSIGDEF:
+      {
+	for (int i = 0; i < array_length (signal_to_check); i++)
+	  {
+	    struct sigaction act;
+	    if (sigaction (signal_to_check[i], NULL, &act) != 0)
+	      exit (EXIT_FAILURE);
+	    if (act.sa_handler != SIG_DFL)
+	      exit (EXIT_FAILURE);
+	  }
+      }
+      break;
+    }
+
+  return 0;
+}
+
+/* Common argument used for process re-execution.  */
+static char *initial_spargv[5];
+static size_t initial_spargv_size;
+
+/* Re-execute the test process with both '--direct', '--restart', and the
+   TEST (as integer value) as arguments.  */
+static void
+reexecute (enum spawn_test_t test, const posix_spawnattr_t *attrp)
+{
+  char *spargv[8];
+  int i;
+
+  for (i = 0; i < initial_spargv_size; i++)
+    spargv[i] = initial_spargv[i];
+  /* Three digits per byte plus null terminator.  */
+  char teststr[3 * sizeof (test) + 1];
+  snprintf (teststr, array_length (teststr), "%d", test);
+  spargv[i++] = teststr;
+  spargv[i] = NULL;
+  TEST_VERIFY (i < 8);
+
+  pid_t pid;
+  int status;
+
+  TEST_COMPARE (posix_spawn (&pid, spargv[0], NULL, attrp, spargv, environ),
+		0);
+  TEST_COMPARE (xwaitpid (pid, &status, 0), pid);
+  TEST_VERIFY (WIFEXITED (status));
+  TEST_VERIFY (!WIFSIGNALED (status));
+  TEST_COMPARE (WEXITSTATUS (status), 0);
+}
+
+/* Test if POSIX_SPAWN_SETSIGMASK change the spawn process signal mask to
+   the value blocked signals defined by SIGNAL_TO_CHECK signals.  */
+static void
+do_test_setsigmask (void)
+{
+  posix_spawnattr_t attr;
+  /* posix_spawnattr_init does not fail.  */
+  posix_spawnattr_init (&attr);
+
+  {
+    sigset_t mask;
+    TEST_COMPARE (sigemptyset (&mask), 0);
+    for (int i = 0; i < array_length (signal_to_check); i++)
+      TEST_COMPARE (sigaddset (&mask, signal_to_check[i]), 0);
+    TEST_COMPARE (posix_spawnattr_setsigmask (&attr, &mask), 0);
+    TEST_COMPARE (posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETSIGMASK), 0);
+  }
+
+  /* Change current mask to be different than the one asked for spawned
+     process.  */
+  {
+    sigset_t empty_mask, current_mask;
+    TEST_COMPARE (sigemptyset (&empty_mask), 0);
+    TEST_COMPARE (sigprocmask (SIG_BLOCK, &empty_mask, &current_mask), 0);
+
+    reexecute (SPAWN_SETSIGMASK, &attr);
+
+    TEST_COMPARE (sigprocmask (SIG_SETMASK, &current_mask, NULL), 0);
+  }
+}
+
+/* Test if POSIX_SPAWN_SETSIGDEF change the spawn process signal actions
+   defined by SIGNAL_TO_CHECK signals to default actions.  */
+static void
+do_test_setsigdef (void)
+{
+  posix_spawnattr_t attr;
+  /* posix_spawnattr_init does not fail.  */
+  posix_spawnattr_init (&attr);
+
+  {
+    sigset_t mask;
+    TEST_COMPARE (sigemptyset (&mask), 0);
+    for (int i = 0; i < array_length (signal_to_check); i++)
+      TEST_COMPARE (sigaddset (&mask, signal_to_check[i]), 0);
+    TEST_COMPARE (posix_spawnattr_setsigdefault (&attr, &mask), 0);
+    TEST_COMPARE (posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETSIGDEF), 0);
+  }
+
+  /* Change current signal disposition to be different than the one asked for
+     spawned process.  */
+  struct sigaction default_act[array_length (signal_to_check)];
+  {
+    sigset_t empty_mask;
+    TEST_COMPARE (sigemptyset (&empty_mask), 0);
+    for (int i = 0; i < array_length (signal_to_check); i++)
+      TEST_COMPARE (sigaction (signal_to_check[i],
+			       &((struct sigaction) { .sa_handler = SIG_IGN,
+						      .sa_mask = empty_mask,
+						      .sa_flags = 0 }),
+			       &default_act[i]),
+		    0);
+  }
+
+  reexecute (SPAWN_SETSIGDEF, &attr);
+
+  /* Restore signal dispositions.  */
+  for (int i = 0; i < array_length (signal_to_check); i++)
+    TEST_COMPARE (sigaction (signal_to_check[i], &default_act[i], NULL), 0);
+}
+
+static int
+do_test (int argc, char *argv[])
+{
+  /* We must have one or four parameters left if called initially:
+       + path for ld.so		optional
+       + "--library-path"	optional
+       + the library path	optional
+       + the application name
+
+     Plus one parameter to indicate which test to execute through
+     re-execution.
+
+     So for default usage without --enable-hardcoded-path-in-tests, it
+     will be called initially with 5 arguments and later with 2.  For
+     --enable-hardcoded-path-in-tests it will be called with 2 arguments
+     regardless.  */
+
+  if (argc != (restart ? 2 : 5) && argc != 2)
+    FAIL_EXIT1 ("wrong number of arguments (%d)", argc);
+
+  if (restart)
+    return handle_restart (atoi (argv[1]));
+
+  {
+    int i;
+    for (i = 0; i < (argc == 5 ? 4 : 1); i++)
+      initial_spargv[i] = argv[i + 1];
+    initial_spargv[i++] = (char *) "--direct";
+    initial_spargv[i++] = (char *) "--restart";
+    initial_spargv_size = i;
+  }
+
+  do_test_setsigmask ();
+  do_test_setsigdef ();
+
+  return 0;
+}
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
diff --git a/sysdeps/generic/sigsetops.h b/sysdeps/generic/sigsetops.h
index ddeeb0b0d5..9cae11771b 100644
--- a/sysdeps/generic/sigsetops.h
+++ b/sysdeps/generic/sigsetops.h
@@ -66,6 +66,13 @@
     0;						\
   }))
 
+# define __sigorset_atomic(set)			\
+  (__extension__ ({				\
+    __sigset_t __mask = __sigmask (sig);	\
+    atomic_fetch_or_seq_cst (set, mask);	\
+    0;						\
+  }))
+
 # define __sigdelset(set, sig)			\
   (__extension__ ({				\
     __sigset_t __mask = __sigmask (sig);	\
diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
index 3562011d21..385442f81e 100644
--- a/sysdeps/unix/sysv/linux/internal-signals.h
+++ b/sysdeps/unix/sysv/linux/internal-signals.h
@@ -88,4 +88,7 @@ __libc_signal_restore_set (const sigset_t *set)
 /* Used to communicate with signal handler.  */
 extern struct xid_command *__xidcmd attribute_hidden;
 
+/* Used to obtained the modified signal handlers.  */
+extern void __get_sighandler_set (sigset_t *set) attribute_hidden;
+
 #endif
diff --git a/sysdeps/unix/sysv/linux/sigaction.c b/sysdeps/unix/sysv/linux/sigaction.c
index 52722b08ae..3bcf3946ab 100644
--- a/sysdeps/unix/sysv/linux/sigaction.c
+++ b/sysdeps/unix/sysv/linux/sigaction.c
@@ -20,6 +20,7 @@
 #include <string.h>
 
 #include <sysdep.h>
+#include <sigsetops.h>
 #include <sys/syscall.h>
 
 /* New ports should not define the obsolete SA_RESTORER, however some
@@ -36,6 +37,13 @@
 # define STUB(act, sigsetsize) (sigsetsize)
 #endif
 
+static sigset_t handler_set;
+
+void __get_sighandler_set (sigset_t *set)
+{
+  *set = handler_set;
+}
+
 /* If ACT is not NULL, change the action for SIG to *ACT.
    If OACT is not NULL, put the old action for SIG in *OACT.  */
 int
@@ -47,6 +55,15 @@ __libc_sigaction (int sig, const struct sigaction *act, struct sigaction *oact)
 
   if (act)
     {
+      /* Tracks which signal had a signal handler set different from default
+	 (SIG_DFL/SIG_IGN).  It allows optimize posix_spawn to reset only
+	 those signals.  It might incur in false positive, since it not easy
+	 to remove bits from the mask without race conditions, but it does not
+	 allow false negative since the mask is updated atomically prior the
+	 syscall.  The false positive incur in just extra sigactions on
+	 posix_spawn.  */
+      if (act->sa_handler != SIG_DFL && act->sa_handler != SIG_IGN)
+	__sigaddset_atomic (&handler_set, sig);
       kact.k_sa_handler = act->sa_handler;
       memcpy (&kact.sa_mask, &act->sa_mask, sizeof (sigset_t));
       kact.sa_flags = act->sa_flags;
diff --git a/sysdeps/unix/sysv/linux/sigsetops.h b/sysdeps/unix/sysv/linux/sigsetops.h
index 713d4840d8..6c98c83e42 100644
--- a/sysdeps/unix/sysv/linux/sigsetops.h
+++ b/sysdeps/unix/sysv/linux/sigsetops.h
@@ -20,6 +20,7 @@
 #define _SIGSETOPS_H 1
 
 #include <signal.h>
+#include <atomic.h>
 
 /* Return a mask that includes the bit for SIG only.  */
 # define __sigmask(sig) \
@@ -80,6 +81,12 @@
     (void)0;							\
   }))
 
+# define __sigorset_atomic(dest, left, right)	\
+  (__extension__ ({				\
+     atomic_fetch_or_seq_cst (dest, left, right); \
+    0;						\
+  }))
+
 /* These macros needn't check for a bogus signal number;
    error checking is done in the non-__ versions.  */
 # define __sigismember(set, sig)				\
@@ -97,6 +104,14 @@
     (void)0;							\
   }))
 
+# define __sigaddset_atomic(set, sig)				\
+  (__extension__ ({						\
+    unsigned long int __mask = __sigmask (sig);			\
+    unsigned long int __word = __sigword (sig);			\
+    atomic_fetch_or_seq_cst (&((set)->__val[__word]), __mask);	\
+    (void)0;							\
+  }))
+
 # define __sigdelset(set, sig)					\
   (__extension__ ({						\
     unsigned long int __mask = __sigmask (sig);			\
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index 0f7a8ca5df..264edd09c6 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -132,17 +132,14 @@ spawni_child (void *arguments)
   const posix_spawnattr_t *restrict attr = args->attr;
   const posix_spawn_file_actions_t *file_actions = args->fa;
 
-  /* The child must ensure that no signal handler are enabled because it shared
+  /* The child must ensure that no signal handler are enabled because it share
      memory with parent, so the signal disposition must be either SIG_DFL or
-     SIG_IGN.  It does by iterating over all signals and although it could
-     possibly be more optimized (by tracking which signal potentially have a
-     signal handler), it might requires system specific solutions (since the
-     sigset_t data type can be very different on different architectures).  */
+     SIG_IGN.  */
   struct sigaction sa;
   memset (&sa, '\0', sizeof (sa));
 
   sigset_t hset;
-  __sigprocmask (SIG_BLOCK, 0, &hset);
+  __get_sighandler_set (&hset);
   for (int sig = 1; sig < _NSIG; ++sig)
     {
       if ((attr->__flags & POSIX_SPAWN_SETSIGDEF)
-- 
2.17.1