Improve robustness of the crash signal handler

ddprof-lib/src/main/cpp/ctimer_linux.cpp

@@ -20,6 +20,7 @@
 #include "debugSupport.h"
 #include "profiler.h"
 #include "vmStructs.h"
+#include <assert.h>
 #include <stdlib.h>
 #include <sys/syscall.h>
 #include <time.h>
@@ -202,6 +203,7 @@ void CTimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) {
     return;
   int tid = 0;
   ProfiledThread *current = ProfiledThread::current();
+  assert(current == nullptr || !current->isDeepCrashHandler());
   if (current != NULL) {
     current->noteCPUSample(Profiler::instance()->recordingEpoch());
     tid = current->tid();

ddprof-lib/src/main/cpp/os_linux.cpp

@@ -242,6 +242,7 @@ SigAction OS::replaceSigsegvHandler(SigAction action) {
   sigaction(SIGSEGV, NULL, &sa);
   SigAction old_action = sa.sa_sigaction;
   sa.sa_sigaction = action;
+  sa.sa_flags = SA_SIGINFO;
   sigaction(SIGSEGV, &sa, NULL);
   return old_action;
 }

ddprof-lib/src/main/cpp/os_macos.cpp

@@ -216,6 +216,7 @@ SigAction OS::replaceSigsegvHandler(SigAction action) {
   sigaction(SIGSEGV, NULL, &sa);
   SigAction old_action = sa.sa_sigaction;
   sa.sa_sigaction = action;
+  sa.sa_flags = SA_SIGINFO;
   sigaction(SIGSEGV, &sa, NULL);
   return old_action;
 }

ddprof-lib/src/main/cpp/profiler.cpp

@@ -40,6 +40,7 @@
 #include <fstream>
 #include <memory>
 #include <set>
+#include <signal.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -883,16 +884,6 @@ void Profiler::disableEngines() {
   _wall_engine->enableEvents(false);
 }
 
-void Profiler::trapHandlerEntry(int signo, siginfo_t *siginfo, void *ucontext) {
-  Profiler::instance()->trapHandler(signo, siginfo, ucontext);
-}
-
-void Profiler::trapHandler(int signo, siginfo_t *siginfo, void *ucontext) {
-  if (orig_trapHandler != NULL) {
-    orig_trapHandler(signo, siginfo, ucontext);
-  }
-}
-
 void Profiler::segvHandler(int signo, siginfo_t *siginfo, void *ucontext) {
   if (!crashHandler(signo, siginfo, ucontext)) {
     orig_segvHandler(signo, siginfo, ucontext);
@@ -906,14 +897,30 @@ void Profiler::busHandler(int signo, siginfo_t *siginfo, void *ucontext) {
 }
 
 bool Profiler::crashHandler(int signo, siginfo_t *siginfo, void *ucontext) {
+  ProfiledThread* thrd = ProfiledThread::current();
+  if (thrd != nullptr && !thrd->enterCrashHandler()) {
+    // we are already in a crash handler; don't recurse!
+    return false;
+  }
+  uintptr_t fault_address = (uintptr_t)siginfo->si_addr;
   StackFrame frame(ucontext);
   uintptr_t pc = frame.pc();
+  if (pc == fault_address) {
+    // it is 'pc' that is causing the fault; can not access it safely
+    if (thrd != nullptr) {
+      thrd->exitCrashHandler();
+    }
+    return false;
+  }
 
   uintptr_t length = SafeAccess::skipLoad(pc);
   if (length > 0) {
     // Skip the fault instruction, as if it successfully loaded NULL
     frame.pc() += length;
     frame.retval() = 0;
+    if (thrd != nullptr) {
+      thrd->exitCrashHandler();
+    }
     return true;
   }
 
@@ -922,35 +929,44 @@ bool Profiler::crashHandler(int signo, siginfo_t *siginfo, void *ucontext) {
     // Act as if the load returned default_value argument
     frame.pc() += length;
     frame.retval() = frame.arg1();
+    if (thrd != nullptr) {
+      thrd->exitCrashHandler();
+    }
     return true;
   }
 
   if (WX_MEMORY && Trap::isFaultInstruction(pc)) {
+    if (thrd != nullptr) {
+      thrd->exitCrashHandler();
+    }
     return true;
   }
 
   if (VM::isHotspot()) {
     // the following checks require vmstructs and therefore HotSpot
-    StackWalker::checkFault();
+
+    // this check can longjmp to a completely different location - need to call exitCrashHandler() before
+    StackWalker::checkFault(thrd);
 
     // Workaround for JDK-8313796. Setting cstack=dwarf also helps
     if (VMStructs::isInterpretedFrameValidFunc((const void *)pc) &&
         frame.skipFaultInstruction()) {
+      if (thrd != nullptr) {
+        thrd->exitCrashHandler();
+      }
       return true;
     }
   }
 
+  if (thrd != nullptr) {
+    thrd->exitCrashHandler();
+  }
   return false;
 }
 
 void Profiler::setupSignalHandlers() {
-  orig_trapHandler =
-      OS::installSignalHandler(SIGTRAP, Profiler::trapHandlerEntry);
-  if (orig_trapHandler == (void *)SIG_DFL ||
-      orig_trapHandler == (void *)SIG_IGN) {
-    orig_trapHandler = NULL;
-  }
-  if (VM::java_version() > 0) {
+  // do not re-run the signal setup (run only when VM has not been loaded yet)
+  if (VM::java_version() > 0 && !VM::loaded()) {
     // HotSpot and J9 tolerate interposed SIGSEGV/SIGBUS handler; other JVMs
     // probably not
     orig_segvHandler = OS::replaceSigsegvHandler(segvHandler);

ddprof-lib/src/main/cpp/profiler.h

@@ -242,8 +242,6 @@ class Profiler {
   CodeBlob *findRuntimeStub(const void *address);
   bool isAddressInCode(const void *pc);
 
-  static void trapHandlerEntry(int signo, siginfo_t *siginfo, void *ucontext);
-  void trapHandler(int signo, siginfo_t *siginfo, void *ucontext);
   static void segvHandler(int signo, siginfo_t *siginfo, void *ucontext);
   static void busHandler(int signo, siginfo_t *siginfo, void *ucontext);
   static void setupSignalHandlers();

ddprof-lib/src/main/cpp/stackWalker.cpp

@@ -454,9 +454,12 @@ int StackWalker::walkVM(void *ucontext, ASGCT_CallFrame *frames, int max_depth,
   return depth;
 }
 
-void StackWalker::checkFault() {
+void StackWalker::checkFault(ProfiledThread* thrd) {
   VMThread *vm_thread = VMThread::current();
   if (vm_thread != NULL && sameStack(vm_thread->exception(), &vm_thread)) {
+    if (thrd) {
+      thrd->exitCrashHandler();
+    }
     longjmp(*(jmp_buf *)vm_thread->exception(), 1);
   }
 }

ddprof-lib/src/main/cpp/stackWalker.h

@@ -17,7 +17,9 @@
 #ifndef _STACKWALKER_H
 #define _STACKWALKER_H
 
+#include "thread.h"
 #include "vmEntry.h"
+#include <functional>
 #include <stdint.h>
 
 struct StackContext {
@@ -42,7 +44,7 @@ class StackWalker {
                     const void *_termination_frame_begin,
                     const void *_termination_frame_end);
 
-  static void checkFault();
+  static void checkFault(ProfiledThread* thrd);
 };
 
 #endif // _STACKWALKER_H

ddprof-lib/src/main/cpp/thread.h

@@ -4,6 +4,7 @@
 #include "os.h"
 #include "threadLocalData.h"
 #include <atomic>
+#include <cstdint>
 #include <jvmti.h>
 #include <pthread.h>
 #include <stdlib.h>
@@ -12,6 +13,13 @@
 
 class ProfiledThread : public ThreadLocalData {
 private:
+  // We are allowing several levels of nesting because we can be
+  // eg. in a crash handler when wallclock signal kicks in,
+  // catching sigseg while also triggering CPU signal handler
+  // which would also potentially trigger sigseg we need to handle.
+  // This means 3 levels but we allow for some wiggling space, just in case.
+  // Even with 5 levels cap we will need any highly recursing signal handlers
+  static constexpr u32 CRASH_HANDLER_NESTING_LIMIT = 5;
   static pthread_key_t _tls_key;
   static int _buffer_size;
   static std::atomic<int> _running_buffer_pos;
@@ -25,17 +33,18 @@ class ProfiledThread : public ThreadLocalData {
   static void prepareBuffer(int size);
   static void *delayedUninstallUSR1(void *unused);
 
+  u64 _pc;
+  u64 _span_id;
+  volatile u32 _crash_depth;
   int _buffer_pos;
   int _tid;
   u32 _cpu_epoch;
   u32 _wall_epoch;
-  u64 _pc;
   u32 _call_trace_id;
   u32 _recording_epoch;
-  u64 _span_id;
 
   ProfiledThread(int buffer_pos, int tid)
-      : ThreadLocalData(), _buffer_pos(buffer_pos), _tid(tid), _cpu_epoch(0),
+      : ThreadLocalData(), _crash_depth(0), _buffer_pos(buffer_pos), _tid(tid), _cpu_epoch(0),
         _wall_epoch(0), _pc(0), _call_trace_id(0), _recording_epoch(0),
         _span_id(0){};
 
@@ -77,6 +86,26 @@ class ProfiledThread : public ThreadLocalData {
     _call_trace_id = call_trace_id;
   }
 
+  // this is called in the crash handler to avoid recursing
+  bool enterCrashHandler() {
+    u32 prev = _crash_depth;
+    // This is thread local; no need for atomic cmpxchg
+    if (prev < CRASH_HANDLER_NESTING_LIMIT) {
+      _crash_depth++;
+      return true;
+    }
+    return false;
+  }
+
+  // needs to be called when the crash handler exits
+  void exitCrashHandler() {
+    _crash_depth--;
+  }
+
+  bool isDeepCrashHandler() {
+    return _crash_depth > CRASH_HANDLER_NESTING_LIMIT;
+  }
+
   static void signalHandler(int signo, siginfo_t *siginfo, void *ucontext);
 };
 

ddprof-lib/src/main/cpp/vmEntry.h

@@ -122,6 +122,8 @@ class VM {
 
   static jvmtiEnv *jvmti() { return _jvmti; }
 
+  static bool loaded() { return _jvmti != nullptr; }
+
   static JNIEnv *jni() {
     JNIEnv *jni;
     return _vm->GetEnv((void **)&jni, JNI_VERSION_1_6) == 0 ? jni : NULL;