sigchain: limit use of SetHandlingSignal.

The native bridge and user signal handlers are able to not return to
our signal handler, leaving the HandlingSignal flag set to true for the
rest of the lifetime of the thread. Fix this by only using
SetHandlingSignal for handlers that we know will return (i.e. the ART
fault handler).

This effectively reverts commit 90444558, which means sigprocmask's
behavior is back to filtering out claimed signals when inside a user
signal handler.

Include an update to test/115-native-bridge from Zhenhua Wang, to make
sure we keep handling signals when a signal handler longjmps away
instead of returning.

Bug: http://b/37988407
Test: m test-art-host
Test: m test-art-target
Change-Id: Ia7159ddfa38f1f055e5cd6089c849a208d335752

8 files changed, 247 insertions, 63 deletions

diff --git a/runtime/fault_handler.cc b/runtime/fault_handler.cc
index 7f738bf5a9..5594f4dfc7 100644
--- a/runtime/fault_handler.cc
+++ b/runtime/fault_handler.cc
@@ -129,7 +129,21 @@ FaultManager::~FaultManager() {
 
 void FaultManager::Init() {
   CHECK(!initialized_);
-  AddSpecialSignalHandlerFn(SIGSEGV, art_fault_handler);
+  sigset_t mask;
+  sigfillset(&mask);
+  sigdelset(&mask, SIGABRT);
+  sigdelset(&mask, SIGBUS);
+  sigdelset(&mask, SIGFPE);
+  sigdelset(&mask, SIGILL);
+  sigdelset(&mask, SIGSEGV);
+
+  SigchainAction sa = {
+    .sc_sigaction = art_fault_handler,
+    .sc_mask = mask,
+    .sc_flags = 0UL,
+  };
+
+  AddSpecialSignalHandlerFn(SIGSEGV, &sa);
   initialized_ = true;
 }
 
diff --git a/runtime/native_bridge_art_interface.cc b/runtime/native_bridge_art_interface.cc
index d77cfa1d35..cd8315cdf9 100644
--- a/runtime/native_bridge_art_interface.cc
+++ b/runtime/native_bridge_art_interface.cc
@@ -118,7 +118,15 @@ void InitializeNativeBridge(JNIEnv* env, const char* instruction_set) {
       for (int signal = 0; signal < _NSIG; ++signal) {
         android::NativeBridgeSignalHandlerFn fn = android::NativeBridgeGetSignalHandler(signal);
         if (fn != nullptr) {
-          AddSpecialSignalHandlerFn(signal, fn);
+          sigset_t mask;
+          sigfillset(&mask);
+          SigchainAction sa = {
+            .sc_sigaction = fn,
+            .sc_mask = mask,
+            // The native bridge signal might not return back to sigchain's handler.
+            .sc_flags = SIGCHAIN_ALLOW_NORETURN,
+          };
+          AddSpecialSignalHandlerFn(signal, &sa);
         }
       }
 #endif
diff --git a/sigchainlib/sigchain.cc b/sigchainlib/sigchain.cc
index 13c03e8261..df4372f431 100644
--- a/sigchainlib/sigchain.cc
+++ b/sigchainlib/sigchain.cc
@@ -190,10 +190,10 @@ class SignalChain {
     return action_;
   }
 
-  void AddSpecialHandler(SpecialSignalHandlerFn fn) {
-    for (SpecialSignalHandlerFn& slot : special_handlers_) {
-      if (slot == nullptr) {
-        slot = fn;
+  void AddSpecialHandler(SigchainAction* sa) {
+    for (SigchainAction& slot : special_handlers_) {
+      if (slot.sc_sigaction == nullptr) {
+        slot = *sa;
         return;
       }
     }
@@ -201,15 +201,15 @@ class SignalChain {
     fatal("too many special signal handlers");
   }
 
-  void RemoveSpecialHandler(SpecialSignalHandlerFn fn) {
+  void RemoveSpecialHandler(bool (*fn)(int, siginfo_t*, void*)) {
     // This isn't thread safe, but it's unlikely to be a real problem.
     size_t len = sizeof(special_handlers_)/sizeof(*special_handlers_);
     for (size_t i = 0; i < len; ++i) {
-      if (special_handlers_[i] == fn) {
+      if (special_handlers_[i].sc_sigaction == fn) {
         for (size_t j = i; j < len - 1; ++j) {
           special_handlers_[j] = special_handlers_[j + 1];
         }
-        special_handlers_[len - 1] = nullptr;
+        special_handlers_[len - 1].sc_sigaction = nullptr;
         return;
       }
     }
@@ -223,47 +223,37 @@ class SignalChain {
  private:
   bool claimed_;
   struct sigaction action_;
-  SpecialSignalHandlerFn special_handlers_[2];
+  SigchainAction special_handlers_[2];
 };
 
 static SignalChain chains[_NSIG];
 
-class ScopedSignalUnblocker {
- public:
-  explicit ScopedSignalUnblocker(const std::initializer_list<int>& signals) {
-    sigset_t new_mask;
-    sigemptyset(&new_mask);
-    for (int signal : signals) {
-      sigaddset(&new_mask, signal);
-    }
-    if (sigprocmask(SIG_UNBLOCK, &new_mask, &previous_mask_) != 0) {
-      fatal("failed to unblock signals: %s", strerror(errno));
-    }
-  }
-
-  ~ScopedSignalUnblocker() {
-    if (sigprocmask(SIG_SETMASK, &previous_mask_, nullptr) != 0) {
-      fatal("failed to unblock signals: %s", strerror(errno));
-    }
-  }
-
- private:
-  sigset_t previous_mask_;
-};
-
 void SignalChain::Handler(int signo, siginfo_t* siginfo, void* ucontext_raw) {
-  ScopedHandlingSignal handling_signal;
-
   // Try the special handlers first.
   // If one of them crashes, we'll reenter this handler and pass that crash onto the user handler.
   if (!GetHandlingSignal()) {
-    ScopedSignalUnblocker unblocked { SIGABRT, SIGBUS, SIGFPE, SIGILL, SIGSEGV }; // NOLINT
-    SetHandlingSignal(true);
-
     for (const auto& handler : chains[signo].special_handlers_) {
-      if (handler != nullptr && handler(signo, siginfo, ucontext_raw)) {
+      if (handler.sc_sigaction == nullptr) {
+        break;
+      }
+
+      // The native bridge signal handler might not return.
+      // Avoid setting the thread local flag in this case, since we'll never
+      // get a chance to restore it.
+      bool handler_noreturn = (handler.sc_flags & SIGCHAIN_ALLOW_NORETURN);
+      sigset_t previous_mask;
+      linked_sigprocmask(SIG_SETMASK, &handler.sc_mask, &previous_mask);
+
+      ScopedHandlingSignal restorer;
+      if (!handler_noreturn) {
+        SetHandlingSignal(true);
+      }
+
+      if (handler.sc_sigaction(signo, siginfo, ucontext_raw)) {
         return;
       }
+
+      linked_sigprocmask(SIG_SETMASK, &previous_mask, nullptr);
     }
   }
 
@@ -275,7 +265,7 @@ void SignalChain::Handler(int signo, siginfo_t* siginfo, void* ucontext_raw) {
   if ((handler_flags & SA_NODEFER)) {
     sigdelset(&mask, signo);
   }
-  sigprocmask(SIG_SETMASK, &mask, nullptr);
+  linked_sigprocmask(SIG_SETMASK, &mask, nullptr);
 
   if ((handler_flags & SA_SIGINFO)) {
     chains[signo].action_.sa_sigaction(signo, siginfo, ucontext_raw);
@@ -386,7 +376,7 @@ extern "C" int sigprocmask(int how, const sigset_t* bionic_new_set, sigset_t* bi
   return linked_sigprocmask(how, new_set_ptr, bionic_old_set);
 }
 
-extern "C" void AddSpecialSignalHandlerFn(int signal, SpecialSignalHandlerFn fn) {
+extern "C" void AddSpecialSignalHandlerFn(int signal, SigchainAction* sa) {
   InitializeSignalChain();
 
   if (signal <= 0 || signal >= _NSIG) {
@@ -394,11 +384,11 @@ extern "C" void AddSpecialSignalHandlerFn(int signal, SpecialSignalHandlerFn fn)
   }
 
   // Set the managed_handler.
-  chains[signal].AddSpecialHandler(fn);
+  chains[signal].AddSpecialHandler(sa);
   chains[signal].Claim(signal);
 }
 
-extern "C" void RemoveSpecialSignalHandlerFn(int signal, SpecialSignalHandlerFn fn) {
+extern "C" void RemoveSpecialSignalHandlerFn(int signal, bool (*fn)(int, siginfo_t*, void*)) {
   InitializeSignalChain();
 
   if (signal <= 0 || signal >= _NSIG) {
diff --git a/sigchainlib/sigchain.h b/sigchainlib/sigchain.h
index 0bed117963..23fba030d2 100644
--- a/sigchainlib/sigchain.h
+++ b/sigchainlib/sigchain.h
@@ -18,12 +18,21 @@
 #define ART_SIGCHAINLIB_SIGCHAIN_H_
 
 #include <signal.h>
+#include <stdint.h>
 
 namespace art {
 
-typedef bool (*SpecialSignalHandlerFn)(int, siginfo_t*, void*);
-extern "C" void AddSpecialSignalHandlerFn(int signal, SpecialSignalHandlerFn fn);
-extern "C" void RemoveSpecialSignalHandlerFn(int signal, SpecialSignalHandlerFn fn);
+// Handlers that exit without returning to their caller (e.g. via siglongjmp) must pass this flag.
+static constexpr uint64_t SIGCHAIN_ALLOW_NORETURN = 0x1UL;
+
+struct SigchainAction {
+  bool (*sc_sigaction)(int, siginfo_t*, void*);
+  sigset_t sc_mask;
+  uint64_t sc_flags;
+};
+
+extern "C" void AddSpecialSignalHandlerFn(int signal, SigchainAction* sa);
+extern "C" void RemoveSpecialSignalHandlerFn(int signal, bool (*fn)(int, siginfo_t*, void*));
 
 extern "C" void EnsureFrontOfChain(int signal);
 
diff --git a/sigchainlib/sigchain_dummy.cc b/sigchainlib/sigchain_dummy.cc
index 2d7985aad1..edce965e33 100644
--- a/sigchainlib/sigchain_dummy.cc
+++ b/sigchainlib/sigchain_dummy.cc
@@ -54,13 +54,13 @@ extern "C" void EnsureFrontOfChain(int signal ATTRIBUTE_UNUSED) {
 }
 
 extern "C" void AddSpecialSignalHandlerFn(int signal ATTRIBUTE_UNUSED,
-                                          SpecialSignalHandlerFn fn ATTRIBUTE_UNUSED) {
+                                          SigchainAction* sa ATTRIBUTE_UNUSED) {
   log("SetSpecialSignalHandlerFn is not exported by the main executable.");
   abort();
 }
 
 extern "C" void RemoveSpecialSignalHandlerFn(int signal ATTRIBUTE_UNUSED,
-                                          SpecialSignalHandlerFn fn ATTRIBUTE_UNUSED) {
+                                             bool (*fn)(int, siginfo_t*, void*) ATTRIBUTE_UNUSED) {
   log("SetSpecialSignalHandlerFn is not exported by the main executable.");
   abort();
 }
diff --git a/test/115-native-bridge/expected.txt b/test/115-native-bridge/expected.txt
index 9c64111027..343a76247e 100644
--- a/test/115-native-bridge/expected.txt
+++ b/test/115-native-bridge/expected.txt
@@ -3,7 +3,7 @@ Checking for getEnvValues.
 Ready for native bridge tests.
 Checking for support.
 Getting trampoline for JNI_OnLoad with shorty (null).
-Test ART callbacks: all JNI function number is 11.
+Test ART callbacks: all JNI function number is 12.
     name:booleanMethod, signature:(ZZZZZZZZZZ)Z, shorty:ZZZZZZZZZZZ.
     name:byteMethod, signature:(BBBBBBBBBB)B, shorty:BBBBBBBBBBB.
     name:charMethod, signature:(CCCCCCCCCC)C, shorty:CCCCCCCCCCC.
@@ -14,6 +14,7 @@ Test ART callbacks: all JNI function number is 11.
     name:testGetMirandaMethodNative, signature:()Ljava/lang/reflect/Method;, shorty:L.
     name:testNewStringObject, signature:()V, shorty:V.
     name:testSignal, signature:()I, shorty:I.
+    name:testSignalHandlerNotReturn, signature:()V, shorty:V.
     name:testZeroLengthByteBuffers, signature:()V, shorty:V.
 trampoline_JNI_OnLoad called!
 JNI_OnLoad called
@@ -67,3 +68,13 @@ Checking for support.
 Was to load 'libinvalid.so', force fail.
 getError() in native bridge.
 Catch UnsatisfiedLinkError exception as expected.
+Getting trampoline for Java_Main_testSignalHandlerNotReturn with shorty V.
+start testSignalHandlerNotReturn
+raising first SIGSEGV
+NB signal handler with signal 11.
+handling first SIGSEGV, will raise another
+unblock SIGSEGV in handler
+raising second SIGSEGV
+NB signal handler with signal 11.
+handling second SIGSEGV, will jump back to test function
+back to test from signal handler via siglongjmp(), and done!
diff --git a/test/115-native-bridge/nativebridge.cc b/test/115-native-bridge/nativebridge.cc
index b3b89491bf..307fd9b766 100644
--- a/test/115-native-bridge/nativebridge.cc
+++ b/test/115-native-bridge/nativebridge.cc
@@ -26,6 +26,7 @@
 #include "stdio.h"
 #include "unistd.h"
 #include "sys/stat.h"
+#include "setjmp.h"
 
 #include "base/macros.h"
 #include "nativebridge/native_bridge.h"
@@ -191,6 +192,19 @@ char *go_away_compiler = nullptr;
   abort();
 }
 
+static void raise_sigsegv() {
+#if defined(__arm__) || defined(__i386__) || defined(__aarch64__)
+  *go_away_compiler = 'a';
+#elif defined(__x86_64__)
+  // Cause a SEGV using an instruction known to be 2 bytes long to account for hardcoded jump
+  // in the signal handler
+  asm volatile("movl $0, %%eax;" "movb %%ah, (%%rax);" : : : "%eax");
+#else
+  // On other architectures we simulate SEGV.
+  kill(getpid(), SIGSEGV);
+#endif
+}
+
 static jint trampoline_Java_Main_testSignal(JNIEnv*, jclass) {
   // Install the sigaction handler above, which should *not* be reached as the native-bridge
   // handler should be called first. Note: we won't chain at all, if we ever get here, we'll die.
@@ -203,16 +217,7 @@ static jint trampoline_Java_Main_testSignal(JNIEnv*, jclass) {
 
   // Test segv
   sigaction(SIGSEGV, &tmp, nullptr);
-#if defined(__arm__) || defined(__i386__) || defined(__aarch64__)
-  *go_away_compiler = 'a';
-#elif defined(__x86_64__)
-  // Cause a SEGV using an instruction known to be 2 bytes long to account for hardcoded jump
-  // in the signal handler
-  asm volatile("movl $0, %%eax;" "movb %%ah, (%%rax);" : : : "%eax");
-#else
-  // On other architectures we simulate SEGV.
-  kill(getpid(), SIGSEGV);
-#endif
+  raise_sigsegv();
 
   // Test sigill
   sigaction(SIGILL, &tmp, nullptr);
@@ -221,6 +226,135 @@ static jint trampoline_Java_Main_testSignal(JNIEnv*, jclass) {
   return 1234;
 }
 
+// Status of the tricky control path of testSignalHandlerNotReturn.
+//
+// "kNone" is the default status except testSignalHandlerNotReturn,
+// others are used by testSignalHandlerNotReturn.
+enum class TestStatus {
+  kNone,
+  kRaiseFirst,
+  kHandleFirst,
+  kRaiseSecond,
+  kHandleSecond,
+};
+
+// State transition helper for testSignalHandlerNotReturn.
+class SignalHandlerTestStatus {
+ public:
+  SignalHandlerTestStatus() : state_(TestStatus::kNone) {
+  }
+
+  TestStatus Get() {
+    return state_;
+  }
+
+  void Reset() {
+    Set(TestStatus::kNone);
+  }
+
+  void Set(TestStatus state) {
+    switch (state) {
+      case TestStatus::kNone:
+        AssertState(TestStatus::kHandleSecond);
+        break;
+
+      case TestStatus::kRaiseFirst:
+        AssertState(TestStatus::kNone);
+        break;
+
+      case TestStatus::kHandleFirst:
+        AssertState(TestStatus::kRaiseFirst);
+        break;
+
+      case TestStatus::kRaiseSecond:
+        AssertState(TestStatus::kHandleFirst);
+        break;
+
+      case TestStatus::kHandleSecond:
+        AssertState(TestStatus::kRaiseSecond);
+        break;
+
+      default:
+        printf("ERROR: unknown state\n");
+        abort();
+    }
+
+    state_ = state;
+  }
+
+ private:
+  TestStatus state_;
+
+  void AssertState(TestStatus expected) {
+    if (state_ != expected) {
+      printf("ERROR: unexpected state, was %d, expected %d\n", state_, expected);
+    }
+  }
+};
+
+static SignalHandlerTestStatus gSignalTestStatus;
+// The context is used to jump out from signal handler.
+static sigjmp_buf gSignalTestJmpBuf;
+
+// Test whether NativeBridge can receive future signal when its handler doesn't return.
+//
+// Control path:
+//  1. Raise first SIGSEGV in test function.
+//  2. Raise another SIGSEGV in NativeBridge's signal handler which is handling
+//     the first SIGSEGV.
+//  3. Expect that NativeBridge's signal handler invokes again. And jump back
+//     to test function in when handling second SIGSEGV.
+//  4. Exit test.
+//
+// NOTE: sigchain should be aware that "special signal handler" may not return.
+//       Pay attention if this case fails.
+static void trampoline_Java_Main_testSignalHandlerNotReturn(JNIEnv*, jclass) {
+  if (gSignalTestStatus.Get() != TestStatus::kNone) {
+    printf("ERROR: test already started?\n");
+    return;
+  }
+  printf("start testSignalHandlerNotReturn\n");
+
+  if (sigsetjmp(gSignalTestJmpBuf, 1) == 0) {
+    gSignalTestStatus.Set(TestStatus::kRaiseFirst);
+    printf("raising first SIGSEGV\n");
+    raise_sigsegv();
+  } else {
+    // jump to here from signal handler when handling second SIGSEGV.
+    if (gSignalTestStatus.Get() != TestStatus::kHandleSecond) {
+      printf("ERROR: not jump from second SIGSEGV?\n");
+      return;
+    }
+    gSignalTestStatus.Reset();
+    printf("back to test from signal handler via siglongjmp(), and done!\n");
+  }
+}
+
+// Signal handler for testSignalHandlerNotReturn.
+// This handler won't return.
+static bool NotReturnSignalHandler() {
+  if (gSignalTestStatus.Get() == TestStatus::kRaiseFirst) {
+    // handling first SIGSEGV
+    gSignalTestStatus.Set(TestStatus::kHandleFirst);
+    printf("handling first SIGSEGV, will raise another\n");
+    sigset_t set;
+    sigemptyset(&set);
+    sigaddset(&set, SIGSEGV);
+    printf("unblock SIGSEGV in handler\n");
+    sigprocmask(SIG_UNBLOCK, &set, nullptr);
+    gSignalTestStatus.Set(TestStatus::kRaiseSecond);
+    printf("raising second SIGSEGV\n");
+    raise_sigsegv();    // raise second SIGSEGV
+  } else if (gSignalTestStatus.Get() == TestStatus::kRaiseSecond) {
+    // handling second SIGSEGV
+    gSignalTestStatus.Set(TestStatus::kHandleSecond);
+    printf("handling second SIGSEGV, will jump back to test function\n");
+    siglongjmp(gSignalTestJmpBuf, 1);
+  }
+  printf("ERROR: should not reach here!\n");
+  return false;
+}
+
 NativeBridgeMethod gNativeBridgeMethods[] = {
   { "JNI_OnLoad", "", true, nullptr,
     reinterpret_cast<void*>(trampoline_JNI_OnLoad) },
@@ -246,6 +380,8 @@ NativeBridgeMethod gNativeBridgeMethods[] = {
     reinterpret_cast<void*>(trampoline_Java_Main_testZeroLengthByteBuffers) },
   { "testSignal", "()I", true, nullptr,
     reinterpret_cast<void*>(trampoline_Java_Main_testSignal) },
+  { "testSignalHandlerNotReturn", "()V", true, nullptr,
+    reinterpret_cast<void*>(trampoline_Java_Main_testSignalHandlerNotReturn) },
 };
 
 static NativeBridgeMethod* find_native_bridge_method(const char *name) {
@@ -399,10 +535,8 @@ extern "C" bool native_bridge_isCompatibleWith(uint32_t bridge_version ATTRIBUTE
 #endif
 #endif
 
-// A dummy special handler, continueing after the faulting location. This code comes from
-// 004-SignalTest.
-static bool nb_signalhandler(int sig, siginfo_t* info ATTRIBUTE_UNUSED, void* context) {
-  printf("NB signal handler with signal %d.\n", sig);
+static bool StandardSignalHandler(int sig, siginfo_t* info ATTRIBUTE_UNUSED,
+                                     void* context) {
   if (sig == SIGSEGV) {
 #if defined(__arm__)
     struct ucontext *uc = reinterpret_cast<struct ucontext*>(context);
@@ -427,6 +561,21 @@ static bool nb_signalhandler(int sig, siginfo_t* info ATTRIBUTE_UNUSED, void* co
   return true;
 }
 
+// A dummy special handler, continueing after the faulting location. This code comes from
+// 004-SignalTest.
+static bool nb_signalhandler(int sig, siginfo_t* info, void* context) {
+  printf("NB signal handler with signal %d.\n", sig);
+
+  if (gSignalTestStatus.Get() == TestStatus::kNone) {
+    return StandardSignalHandler(sig, info, context);
+  } else if (sig == SIGSEGV) {
+    return NotReturnSignalHandler();
+  } else {
+    printf("ERROR: should not reach here!\n");
+    return false;
+  }
+}
+
 static ::android::NativeBridgeSignalHandlerFn native_bridge_getSignalHandler(int signal) {
   // Test segv for already claimed signal, and sigill for not claimed signal
   if ((signal == SIGSEGV) || (signal == SIGILL)) {
diff --git a/test/115-native-bridge/src/NativeBridgeMain.java b/test/115-native-bridge/src/NativeBridgeMain.java
index e8d1e4e326..11eaa84e80 100644
--- a/test/115-native-bridge/src/NativeBridgeMain.java
+++ b/test/115-native-bridge/src/NativeBridgeMain.java
@@ -35,6 +35,7 @@ class Main {
         testNewStringObject();
         testSignalHandler();
         testGetErrorByLoadInvalidLibrary();
+        testSignalHandlerNotReturn();
     }
 
     public static native void testFindClassOnAttachedNativeThread();
@@ -199,6 +200,8 @@ class Main {
             System.out.println("Catch UnsatisfiedLinkError exception as expected.");
         }
     }
+
+    private static native void testSignalHandlerNotReturn();
 }
 
 public class NativeBridgeMain {