[fault-injection] Profiler::recordSample() stack walk is unprotected

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

@@ -128,7 +128,7 @@
   X(SAMPLES_DROPPED_THREAD_LOCAL, "samples_dropped_thread_local")             \
   X(SAFECOPY_FAILED, "safecopy_failed")                                       \
   X(SAFEFETCH_FAILED, "safefetch_failed")                                     \
-  X(WALKVM_LONGJMP_RECOVERED, "walkvm_longjmp_recovered")                     \
+  X(STACKWALK_LONGJMP_RECOVERED, "stackwalk_longjmp_recovered")               \
   DD_COUNTER_TABLE_FAULT_INJECTION(X)                                          \
   DD_COUNTER_TABLE_FI_DEBUG(X)                                                 \
   DD_COUNTER_TABLE_DEBUG(X)

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

@@ -1204,7 +1204,7 @@ void Recording::writeSettings(Buffer *buf, Arguments &args) {
   }
 
   writeBoolSetting(buf, T_ACTIVE_RECORDING, "debugSymbols",
-                   VMStructs::libjvm()->hasDebugSymbols());
+                   VM::libjvm()->hasDebugSymbols());
   writeBoolSetting(buf, T_ACTIVE_RECORDING, "kernelSymbols",
                    Symbols::haveKernelSymbols());
   writeStringSetting(buf, T_ACTIVE_RECORDING, "cpuEngine",

ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp

@@ -575,6 +575,9 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex
                                        level >= 1 && level <= 3 ? FRAME_C1_COMPILED : FRAME_JIT_COMPILED;
                             }
                             VMMethod* method = scope.method();
+                            if (method == nullptr) {
+                                break;
+                            }
                             jmethodID method_id = method->id();
                             fillFrame(frames[depth++], type, scope.bci(), method_id, method);
                         } while (scope_offset > 0 && depth < max_depth);
@@ -960,22 +963,6 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex
     return depth;
 }
 
-void HotspotSupport::checkFault(ProfiledThread* thrd) {
-    // Should not get to here (?)
-    if (thrd == nullptr) {
-        return;
-    }
-
-    // Check if longjmp is setup for this thread
-    if (!thrd->isProtected()) {
-        return;
-    }
-
-    thrd->resetCrashHandler();
-    Counters::increment(WALKVM_LONGJMP_RECOVERED);
-    longjmp(*thrd->getJmpCtx(), 1);
-}
-
 int HotspotSupport::getJavaTraceAsync(void *ucontext, ASGCT_CallFrame *frames,
                                 int max_depth, StackContext *java_ctx,
                                 bool *truncated) {

ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h

@@ -35,8 +35,6 @@ class HotspotSupport {
     static bool loadMethodIDsIfNeededImpl(jvmtiEnv *jvmti, JNIEnv *jni, jclass klass, bool load_all);
 public:
     static void initClassloaderInfo(JNIEnv* jni);
-
-    static void checkFault(ProfiledThread* thrd = nullptr);
     static int walkJavaStack(StackWalkRequest& request);
     static inline bool canUnwind(const StackFrame& frame, const void*& pc) {
         return HotspotStackFrame::unwindAtomicStub(frame, pc);

ddprof-lib/src/main/cpp/hotspot/vmStructs.cpp

@@ -842,7 +842,8 @@ VMFlag* VMFlag::find(const char* name) {
 
         for (size_t i = 0; i < count; i++) {
             VMFlag* f = VMFlag::cast(*(const char**)_flags_addr + i * VMFlag::type_size());
-            if (f->name() != NULL && strcmp(f->name(), name) == 0 && f->addr() != NULL) {
+            const char* fname = f->name();
+            if (fname != nullptr && strcmp(fname, name) == 0 && f->addr() != nullptr) {
                 return f;
             }
         }
@@ -863,7 +864,8 @@ VMFlag *VMFlag::find(const char *name, int type_mask) {
         size_t count = *(size_t*)_flag_count;
         for (size_t i = 0; i < count; i++) {
             VMFlag* f = VMFlag::cast(*(const char**)_flags_addr + i * VMFlag::type_size());
-            if (f->name() != NULL && strcmp(f->name(), name) == 0) {
+            const char* fname = f->name();
+            if (fname != nullptr && strcmp(fname, name) == 0) {
                 int masked = 0x1 << f->type();
                 if (masked & type_mask) {
                     return (VMFlag*)f;

ddprof-lib/src/main/cpp/hotspot/vmStructs.h

@@ -449,18 +449,11 @@ class VMStructs {
     static void checkNativeBinding(jvmtiEnv *jvmti, JNIEnv *jni, jmethodID method, void *address);
     static const void *findHeapUsageFunc();
 
-    const char* at(int offset) {
-        const char* ptr = (const char*)this + offset;
-        assert(crashProtectionActive() || SafeAccess::isReadable(ptr));
-        // Poison only the returned pointer; the assert above sees the real ptr.
-        return INJECT_FAULT_ADDRESS_RARE(ptr);
-    }
+    inline const char* at(int offset);
+    inline const char* at(int offset) const;
 
-    const char* at(int offset) const {
-        const char* ptr = (const char*)this + offset;
-        assert(crashProtectionActive() || SafeAccess::isReadable(ptr));
-        return INJECT_FAULT_ADDRESS_RARE(ptr);
-    }
+    template <typename T, bool safe = false>
+    T load_at_offset(int offset) const;
 
     static bool goodPtr(const void* ptr) {
         return (uintptr_t)ptr >= 0x1000 && ((uintptr_t)ptr & (sizeof(uintptr_t) - 1)) == 0;
@@ -1136,10 +1129,7 @@ DECLARE(VMFlag)
     static VMFlag* find(const char* name);
     static VMFlag *find(const char* name, std::initializer_list<Type> types);
 
-    const char* name() {
-        assert(_flag_name_offset >= 0);
-        return *(const char**) at(_flag_name_offset);
-    }
+    inline const char* name() const;
 
     int type();
 

ddprof-lib/src/main/cpp/hotspot/vmStructs.inline.h

@@ -10,6 +10,7 @@
 #include "hotspot/hotspotSupport.h"
 #include "hotspot/vmStructs.h"
 #include "jvmThread.h"
+#include "safeAccess.h"
 #include "threadLocalData.h"
 
 inline bool crashProtectionActive() {
@@ -28,6 +29,30 @@ inline T* cast_to(const void* ptr) {
     return reinterpret_cast<T*>(const_cast<void*>(ptr));
 }
 
+inline const char* VMStructs::at(int offset) {
+    const char* ptr = (const char*)this + offset;
+    assert(crashProtectionActive() || SafeAccess::isReadable(ptr));
+    // Poison only the returned pointer; the assert above sees the real ptr.
+    return INJECT_FAULT_ADDRESS_RARE(ptr);
+}
+
+inline const char* VMStructs::at(int offset) const {
+    const char* ptr = (const char*)this + offset;
+    assert(crashProtectionActive() || SafeAccess::isReadable(ptr));
+    return INJECT_FAULT_ADDRESS_RARE(ptr);
+}
+
+template <typename T, bool safe>
+T VMStructs::load_at_offset(int offset) const {
+    const char* ptr = at(offset);
+    if (safe) {
+        return (T)SafeAccess::loadPtr((void**)ptr, nullptr);
+    } else {
+        return  *((T*)ptr);
+    }
+}
+
+
 VMThread* VMThread::current() {
     assert(VM::isHotspot());
     return VMThread::cast(JVMThread::current());
@@ -188,5 +213,10 @@ u16 VMConstMethod::signatureIndex() const {
     return *(u16*)at(_constmethod_sig_index_offset);
 }
 
+// This method may be called without crash protection, so read the name via SafeAccess.
+inline const char* VMFlag::name() const {
+    assert(_flag_name_offset >= 0);
+    return load_at_offset<const char*, true /* safe load */>(_flag_name_offset);
+}
 
 #endif // _HOTSPOT_VMSTRUCTS_INLINE_H

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

@@ -614,30 +614,78 @@ bool Profiler::recordSample(void *ucontext, u64 counter, int tid,
 #endif // COUNTERS
     ASGCT_CallFrame *frames = _calltrace_buffer[lock_index]->_asgct_frames;
 
-    int num_frames = 0;
+    // Read again after the setjmp landing below, so it must be volatile: a
+    // longjmp out of the unwind leaves non-volatile locals indeterminate.
+    volatile int num_frames = 0;
 
     StackContext java_ctx = {0};
-    ASGCT_CallFrame *native_stop = frames + num_frames;
-    num_frames += getNativeTrace(ucontext, native_stop, event_type, tid,
-                                 &java_ctx, &truncated, lock_index);
-    assert(num_frames >= 0);
-
-    int max_remaining = _max_stack_depth - num_frames;
-    if (max_remaining > 0) {
-      StackWalkRequest request = {event_type, lock_index, ucontext, frames + num_frames, max_remaining, &java_ctx, &truncated};
-      num_frames += JVMSupport::walkJavaStack(request);
+
+    // Establish setjmp/longjmp crash protection around the unwind. The native
+    // walkers (walkFP/walkDwarf) protect their pointer loads with SafeAccess
+    // safefetch, but the surrounding metadata reads (isJitCode, findFrameDesc,
+    // findLibraryByAddress, AGCT in getJavaTraceAsync, frame.link, ...) are raw
+    // dereferences of signal-supplied pc/sp/fp. Without an active jmp_buf a
+    // fault there is unrecoverable: crashHandlerInternal -> checkFault() finds
+    // isProtected()==false and chains to the JVM handler, crashing the process.
+    // walkVM installs its own inner jmp_buf and chains back to whatever we set
+    // here, so nesting is safe. When there is no ProfiledThread we have nowhere
+    // to publish a jmp_buf, so we skip the unwind entirely rather than risk an
+    // unrecoverable fault on a raw dereference.
+    ProfiledThread *walk_thread = ProfiledThread::current();
+
+    if (walk_thread == nullptr) {
+      num_frames += makeFrame(frames + num_frames, BCI_ERROR, "no_ProfiledThread");
+      truncated = true;
+    } else {
+      jmp_buf unwind_ctx;
+      jmp_buf *prev_jmp_buf = walk_thread->getJmpCtx();
+
+      // setjmp() must be evaluated as a standalone expression/controlling
+      // expression (C11 7.13.1.1), hence the explicit jmp_rc.
+      int jmp_rc = setjmp(unwind_ctx);
+      if (jmp_rc != 0) {
+        // A fault during unwinding longjmp'd back here (via checkFault). The
+        // longjmp bypassed segvHandler's SignalHandlerScope destructor, so
+        // compensate, restore the previous jmp_buf chain, and record the
+        // partial trace with an error marker instead of crashing.
+        SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP();
+        walk_thread->setJmpCtx(prev_jmp_buf);
+        truncated = true;
+        if (num_frames < _max_stack_depth) {
+          num_frames += makeFrame(frames + num_frames, BCI_ERROR, "break_unwind_fault");
+        }
+      } else {
+        walk_thread->setJmpCtx(&unwind_ctx);
+
+        // truncated_local is never read after a longjmp landing (only on the
+        // clean path below), so it need not be volatile; the outer `truncated`
+        // stays false on the recovery path.
+        bool truncated_local = false;
+        ASGCT_CallFrame *native_stop = frames + num_frames;
+        num_frames += getNativeTrace(ucontext, native_stop, event_type, tid,
+                                     &java_ctx, &truncated_local, lock_index);
+        assert(num_frames >= 0);
+
+        int max_remaining = _max_stack_depth - num_frames;
+        if (max_remaining > 0) {
+          StackWalkRequest request = {event_type, lock_index, ucontext, frames + num_frames, max_remaining, &java_ctx, &truncated_local};
+          num_frames += JVMSupport::walkJavaStack(request);
+        }
+        assert(num_frames >= 0);
+
+        walk_thread->setJmpCtx(prev_jmp_buf);
+        truncated = truncated_local;
+      }
     }
-
-    assert(num_frames >= 0);
+
     if (num_frames == 0) {
       num_frames += makeFrame(frames + num_frames, BCI_ERROR, "no_Java_frame");
     }
 
     call_trace_id =
         _call_trace_storage.put(num_frames, frames, truncated, counter);
-    ProfiledThread *thread = ProfiledThread::current();
-    if (thread != nullptr) {
-      thread->recordCallTraceId(call_trace_id);
+    if (walk_thread != nullptr) {
+      walk_thread->recordCallTraceId(call_trace_id);
     }
 #ifdef COUNTERS
     u64 duration = TSC::ticks() - startTime;
@@ -924,6 +972,18 @@ void Profiler::busHandler(int signo, siginfo_t *siginfo, void *ucontext) {
   }
 }
 
+void Profiler::checkFault(ProfiledThread* thrd) {
+  if (thrd == nullptr || !thrd->isProtected()) {
+    return;
+  }
+
+  thrd->resetCrashHandler();
+  // Shared recovery point for every setjmp/longjmp-protected stack walk
+  // (walkVM's inner region and recordSample's native/AGCT unwind), so the
+  // counter is deliberately not walkVM-specific.
+  Counters::increment(STACKWALK_LONGJMP_RECOVERED);
+  longjmp(*thrd->getJmpCtx(), 1);
+}
 // Returns: 0 = not handled (chain to next handler), non-zero = handled
 int Profiler::crashHandlerInternal(int signo, siginfo_t *siginfo, void *ucontext) {
   ProfiledThread* thrd = ProfiledThread::current();
@@ -966,14 +1026,15 @@ int Profiler::crashHandlerInternal(int signo, siginfo_t *siginfo, void *ucontext
     return 1;  // handled
   }
 
+
+  // checkFault has its own check if we're in a protected stack walk.
+  // If the fault is from our protected walk, it will longjmp and never return.
+  // If it returns, the fault wasn't from our code.
+  checkFault(thrd);
+
   if (VM::isHotspot()) {
     // the following checks require vmstructs and therefore HotSpot
 
-    // HotspotSupport::checkFault has its own check if we're in a protected stack walk.
-    // If the fault is from our protected walk, it will longjmp and never return.
-    // If it returns, the fault wasn't from our code.
-    HotspotSupport::checkFault(thrd);
-
     // Workaround for JDK-8313796 if needed. Setting cstack=dwarf also helps
     if (_need_JDK_8313796_workaround &&
         VMStructs::isInterpretedFrameValidFunc((const void *)pc) &&
@@ -1221,7 +1282,7 @@ Error Profiler::checkJvmCapabilities() {
     }
   }
 
-  if (!VMStructs::libjvm()->hasDebugSymbols() && !VM::isOpenJ9()) {
+  if (!VM::libjvm()->hasDebugSymbols()) {
     Log::warn("Install JVM debug symbols to improve profile accuracy");
   }
 

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

@@ -215,6 +215,8 @@ class alignas(alignof(SpinLock)) Profiler {
     }
   }
 
+  static void checkFault(ProfiledThread* thrd = nullptr);
+
   static inline Profiler *instance() {
     return _instance;
   }