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
(cherry picked from commit 6b2018f4b847a60f39c86d67e1cae8a00ce977bc)
diff --git a/runtime/fault_handler.cc b/runtime/fault_handler.cc
index 7f738bf..5594f4d 100644
--- a/runtime/fault_handler.cc
+++ b/runtime/fault_handler.cc
@@ -129,7 +129,21 @@
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 d77cfa1..cd8315c 100644
--- a/runtime/native_bridge_art_interface.cc
+++ b/runtime/native_bridge_art_interface.cc
@@ -118,7 +118,15 @@
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 13c03e8..df4372f 100644
--- a/sigchainlib/sigchain.cc
+++ b/sigchainlib/sigchain.cc
@@ -190,10 +190,10 @@
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 @@
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 @@
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 @@
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 @@
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 @@
}
// 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 0bed117..23fba03 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 2d7985a..edce965 100644
--- a/sigchainlib/sigchain_dummy.cc
+++ b/sigchainlib/sigchain_dummy.cc
@@ -54,13 +54,13 @@
}
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 9c64111..343a762 100644
--- a/test/115-native-bridge/expected.txt
+++ b/test/115-native-bridge/expected.txt
@@ -3,7 +3,7 @@
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 @@
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 @@
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 b3b8949..307fd9b 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 @@
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 @@
// 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 @@
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 @@
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 @@
#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 @@
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 e8d1e4e..11eaa84 100644
--- a/test/115-native-bridge/src/NativeBridgeMain.java
+++ b/test/115-native-bridge/src/NativeBridgeMain.java
@@ -35,6 +35,7 @@
testNewStringObject();
testSignalHandler();
testGetErrorByLoadInvalidLibrary();
+ testSignalHandlerNotReturn();
}
public static native void testFindClassOnAttachedNativeThread();
@@ -199,6 +200,8 @@
System.out.println("Catch UnsatisfiedLinkError exception as expected.");
}
}
+
+ private static native void testSignalHandlerNotReturn();
}
public class NativeBridgeMain {
