commit 7bfacaabf250d8aed5de82e8091b3251bd8dd53f
author Elliott Hughes <enh@google.com> Tue Nov 28 19:58:00 2017 -0800
committer Elliott Hughes <enh@google.com> Wed Nov 29 18:36:38 2017 -0800
tree d7f131953ebc2112db639e19dad7f152faa7f155
parent 0a25aa60a2a7cc6b681dd2e31321000cedbff77e

Fix posix_spawn signal defaulting.

Add a new stress test, and fix the code to pass it. We need to ensure that
we reset signal handlers for caught signals before unblocking signals in
the child, we need to ensure that this happens even if you haven't passed
a pthread_spawn_attr_t, and we need to ensure that this happens if you
pass in an empty sigdefault set.

Bug: http://b/68707996
Test: ran tests
Change-Id: I348e9b17b1bdf221591da42c0ada133d98471d66

libc/bionic/spawn.cpp

diff --git a/libc/bionic/spawn.cpp b/libc/bionic/spawn.cpp
index 061d68c..e5075f5 100644
--- a/libc/bionic/spawn.cpp
+++ b/libc/bionic/spawn.cpp
@@ -89,39 +89,47 @@
   int schedpolicy;
   sigset_t sigmask;
   sigset_t sigdefault;
-
-  void Do() {
-    if ((flags & POSIX_SPAWN_SETSIGDEF) != 0) {
-      // POSIX: "If POSIX_SPAWN_SETSIGDEF is set ... signals in sigdefault ... shall be set to
-      // their default actions in the child process."
-      struct sigaction sa = {};
-      sa.sa_handler = SIG_DFL;
-      for (int s = 1; s < _NSIG; ++s) {
-        if (sigismember(&sigdefault, s) && sigaction(s, &sa, nullptr) == -1) _exit(127);
-      }
-    }
-
-    if ((flags & POSIX_SPAWN_SETPGROUP) != 0 && setpgid(0, pgroup) == -1) _exit(127);
-    if ((flags & POSIX_SPAWN_SETSID) != 0 && setsid() == -1) _exit(127);
-
-    // POSIX_SPAWN_SETSCHEDULER overrides POSIX_SPAWN_SETSCHEDPARAM, but it is not an error
-    // to set both.
-    if ((flags & POSIX_SPAWN_SETSCHEDULER) != 0) {
-      if (sched_setscheduler(0, schedpolicy, &schedparam) == -1) _exit(127);
-    } else if ((flags & POSIX_SPAWN_SETSCHEDPARAM) != 0) {
-      if (sched_setparam(0, &schedparam) == -1) _exit(127);
-    }
-
-    if ((flags & POSIX_SPAWN_RESETIDS) != 0) {
-      if (seteuid(getuid()) == -1 || setegid(getgid()) == -1) _exit(127);
-    }
-
-    if ((flags & POSIX_SPAWN_SETSIGMASK) != 0) {
-      if (sigprocmask(SIG_SETMASK, &sigmask, nullptr)) _exit(127);
-    }
-  }
 };
 
+static void ApplyAttrs(short flags, const posix_spawnattr_t* attr) {
+  // POSIX: "If POSIX_SPAWN_SETSIGDEF is set ... signals in sigdefault ...
+  // shall be set to their default actions in the child process."
+  // POSIX: "Signals set to be caught by the calling process shall be
+  // set to the default action in the child process."
+  bool use_sigdefault = ((flags & POSIX_SPAWN_SETSIGDEF) != 0);
+  const struct sigaction default_sa = { .sa_handler = SIG_DFL };
+  for (int s = 1; s < _NSIG; ++s) {
+    bool reset = false;
+    if (use_sigdefault && sigismember(&(*attr)->sigdefault, s)) {
+      reset = true;
+    } else {
+      struct sigaction current;
+      if (sigaction(s, nullptr, &current) == -1) _exit(127);
+      reset = (current.sa_handler != SIG_IGN && current.sa_handler != SIG_DFL);
+    }
+    if (reset && sigaction(s, &default_sa, nullptr) == -1) _exit(127);
+  }
+
+  if ((flags & POSIX_SPAWN_SETPGROUP) != 0 && setpgid(0, (*attr)->pgroup) == -1) _exit(127);
+  if ((flags & POSIX_SPAWN_SETSID) != 0 && setsid() == -1) _exit(127);
+
+  // POSIX_SPAWN_SETSCHEDULER overrides POSIX_SPAWN_SETSCHEDPARAM, but it is not an error
+  // to set both.
+  if ((flags & POSIX_SPAWN_SETSCHEDULER) != 0) {
+    if (sched_setscheduler(0, (*attr)->schedpolicy, &(*attr)->schedparam) == -1) _exit(127);
+  } else if ((flags & POSIX_SPAWN_SETSCHEDPARAM) != 0) {
+    if (sched_setparam(0, &(*attr)->schedparam) == -1) _exit(127);
+  }
+
+  if ((flags & POSIX_SPAWN_RESETIDS) != 0) {
+    if (seteuid(getuid()) == -1 || setegid(getgid()) == -1) _exit(127);
+  }
+
+  if ((flags & POSIX_SPAWN_SETSIGMASK) != 0) {
+    if (sigprocmask(SIG_SETMASK, &(*attr)->sigmask, nullptr)) _exit(127);
+  }
+}
+
 static int posix_spawn(pid_t* pid_ptr,
                        const char* path,
                        const posix_spawn_file_actions_t* actions,
@@ -142,7 +150,7 @@
 
   if (pid == 0) {
     // Child.
-    if (attr) (*attr)->Do();
+    ApplyAttrs(flags, attr);
     if (actions) (*actions)->Do();
     if ((flags & POSIX_SPAWN_SETSIGMASK) == 0) ssb.reset();
     exec_fn(path, argv, env ? env : environ);