8271490: [ppc] [s390]: Crash in JavaThread::pd_get_top_frame_for_profiling

RealLucy · RealLucy · commit 276b07b36af0 · 2021-08-30T12:31:08.000Z
Reviewed-by: stuefe, mbaesken
diff --git a/src/hotspot/cpu/ppc/frame_ppc.cpp b/src/hotspot/cpu/ppc/frame_ppc.cpp
@@ -52,7 +52,6 @@ void RegisterMap::check_location_valid() {
 #endif // ASSERT
 
 bool frame::safe_for_sender(JavaThread *thread) {
-  bool safe = false;
   address sp = (address)_sp;
   address fp = (address)_fp;
   address unextended_sp = (address)_unextended_sp;
@@ -70,28 +69,23 @@ bool frame::safe_for_sender(JavaThread *thread) {
 
   // An fp must be within the stack and above (but not equal) sp.
   bool fp_safe = thread->is_in_stack_range_excl(fp, sp);
-  // An interpreter fp must be within the stack and above (but not equal) sp.
-  // Moreover, it must be at least the size of the ijava_state structure.
+  // An interpreter fp must be fp_safe.
+  // Moreover, it must be at a distance at least the size of the ijava_state structure.
   bool fp_interp_safe = fp_safe && ((fp - sp) >= ijava_state_size);
 
   // We know sp/unextended_sp are safe, only fp is questionable here
 
   // If the current frame is known to the code cache then we can attempt to
-  // to construct the sender and do some validation of it. This goes a long way
+  // construct the sender and do some validation of it. This goes a long way
   // toward eliminating issues when we get in frame construction code
 
   if (_cb != NULL ){
-    // Entry frame checks
-    if (is_entry_frame()) {
-      // An entry frame must have a valid fp.
-      return fp_safe && is_entry_frame_valid(thread);
-    }
 
-    // Now check if the frame is complete and the test is
-    // reliable. Unfortunately we can only check frame completeness for
-    // runtime stubs and nmethods. Other generic buffer blobs are more
-    // problematic so we just assume they are OK. Adapter blobs never have a
-    // complete frame and are never OK
+    // First check if the frame is complete and the test is reliable.
+    // Unfortunately we can only check frame completeness for runtime stubs
+    // and nmethods. Other generic buffer blobs are more problematic
+    // so we just assume they are OK.
+    // Adapter blobs never have a complete frame and are never OK
     if (!_cb->is_frame_complete_at(_pc)) {
       if (_cb->is_compiled() || _cb->is_adapter_blob() || _cb->is_runtime_stub()) {
         return false;
@@ -103,10 +97,23 @@ bool frame::safe_for_sender(JavaThread *thread) {
       return false;
     }
 
+    // Entry frame checks
+    if (is_entry_frame()) {
+      // An entry frame must have a valid fp.
+      return fp_safe && is_entry_frame_valid(thread);
+    }
+
     if (is_interpreted_frame() && !fp_interp_safe) {
       return false;
     }
 
+    // At this point, there still is a chance that fp_safe is false.
+    // In particular, (fp == NULL) might be true. So let's check and
+    // bail out before we actually dereference from fp.
+    if (!fp_safe) {
+      return false;
+    }
+
     abi_minframe* sender_abi = (abi_minframe*) fp;
     intptr_t* sender_sp = (intptr_t*) fp;
     address   sender_pc = (address) sender_abi->lr;;
diff --git a/src/hotspot/cpu/s390/frame_s390.cpp b/src/hotspot/cpu/s390/frame_s390.cpp
@@ -55,7 +55,6 @@ void RegisterMap::check_location_valid() {
 // Profiling/safepoint support
 
 bool frame::safe_for_sender(JavaThread *thread) {
-  bool safe = false;
   address sp = (address)_sp;
   address fp = (address)_fp;
   address unextended_sp = (address)_unextended_sp;
@@ -73,28 +72,23 @@ bool frame::safe_for_sender(JavaThread *thread) {
 
   // An fp must be within the stack and above (but not equal) sp.
   bool fp_safe = thread->is_in_stack_range_excl(fp, sp);
-  // An interpreter fp must be within the stack and above (but not equal) sp.
-  // Moreover, it must be at least the size of the z_ijava_state structure.
+  // An interpreter fp must be fp_safe.
+  // Moreover, it must be at a distance at least the size of the z_ijava_state structure.
   bool fp_interp_safe = fp_safe && ((fp - sp) >= z_ijava_state_size);
 
   // We know sp/unextended_sp are safe, only fp is questionable here
 
   // If the current frame is known to the code cache then we can attempt to
-  // to construct the sender and do some validation of it. This goes a long way
+  // construct the sender and do some validation of it. This goes a long way
   // toward eliminating issues when we get in frame construction code
 
   if (_cb != NULL ) {
-    // Entry frame checks
-    if (is_entry_frame()) {
-      // An entry frame must have a valid fp.
-      return fp_safe && is_entry_frame_valid(thread);
-    }
 
-    // Now check if the frame is complete and the test is
-    // reliable. Unfortunately we can only check frame completeness for
-    // runtime stubs. Other generic buffer blobs are more
-    // problematic so we just assume they are OK. Adapter blobs never have a
-    // complete frame and are never OK. nmethods should be OK on s390.
+    // First check if the frame is complete and the test is reliable.
+    // Unfortunately we can only check frame completeness for runtime stubs.
+    // Other generic buffer blobs are more problematic so we just assume they are OK.
+    // Adapter blobs never have a complete frame and are never OK.
+    // nmethods should be OK on s390.
     if (!_cb->is_frame_complete_at(_pc)) {
       if (_cb->is_adapter_blob() || _cb->is_runtime_stub()) {
         return false;
@@ -106,13 +100,26 @@ bool frame::safe_for_sender(JavaThread *thread) {
       return false;
     }
 
+    // Entry frame checks
+    if (is_entry_frame()) {
+      // An entry frame must have a valid fp.
+      return fp_safe && is_entry_frame_valid(thread);
+    }
+
     if (is_interpreted_frame() && !fp_interp_safe) {
       return false;
     }
 
+    // At this point, there still is a chance that fp_safe is false.
+    // In particular, (fp == NULL) might be true. So let's check and
+    // bail out before we actually dereference from fp.
+    if (!fp_safe) {
+      return false;
+    }
+
     z_abi_160* sender_abi = (z_abi_160*) fp;
     intptr_t* sender_sp = (intptr_t*) sender_abi->callers_sp;
-    address   sender_pc = (address) sender_abi->return_pc;
+    address   sender_pc = (address)   sender_abi->return_pc;
 
     // We must always be able to find a recognizable pc.
     CodeBlob* sender_blob = CodeCache::find_blob_unsafe(sender_pc);
diff --git a/src/hotspot/os_cpu/linux_ppc/thread_linux_ppc.cpp b/src/hotspot/os_cpu/linux_ppc/thread_linux_ppc.cpp
@@ -35,6 +35,8 @@ frame JavaThread::pd_last_frame() {
   address pc = _anchor.last_Java_pc();
 
   // Last_Java_pc ist not set, if we come here from compiled code.
+  // Assume spill slot for link register contains a suitable pc.
+  // Should have been filled by method entry code.
   if (pc == NULL) {
     pc = (address) *(sp + 2);
   }
@@ -64,6 +66,17 @@ bool JavaThread::pd_get_top_frame_for_profiling(frame* fr_addr, void* ucontext,
       return false;
     }
 
+    if (ret_frame.fp() == NULL) {
+      // The found frame does not have a valid frame pointer.
+      // Bail out because this will create big trouble later on, either
+      //  - when using istate, calculated as (NULL - ijava_state_size) or
+      //  - when using fp() directly in safe_for_sender()
+      //
+      // There is no conclusive description (yet) how this could happen, but it does.
+      // For more details on what was observed, see thread_linux_s390.cpp
+      return false;
+    }
+
     if (ret_frame.is_interpreted_frame()) {
       frame::ijava_state *istate = ret_frame.get_ijava_state();
       const Method *m = (const Method*)(istate->method);
diff --git a/src/hotspot/os_cpu/linux_s390/thread_linux_s390.cpp b/src/hotspot/os_cpu/linux_s390/thread_linux_s390.cpp
@@ -35,6 +35,8 @@ frame JavaThread::pd_last_frame() {
   address pc = _anchor.last_Java_pc();
 
   // Last_Java_pc ist not set if we come here from compiled code.
+  // Assume spill slot for Z_R14 (return register) contains a suitable pc.
+  // Should have been filled by method entry code.
   if (pc == NULL) {
     pc = (address) *(sp + 14);
   }
@@ -51,6 +53,9 @@ bool JavaThread::pd_get_top_frame_for_profiling(frame* fr_addr, void* ucontext,
     return true;
   }
 
+  // At this point, we don't have a last_Java_frame, so
+  // we try to glean some information out of the ucontext
+  // if we were running Java code when SIGPROF came in.
   if (isInJava) {
     ucontext_t* uc = (ucontext_t*) ucontext;
     frame ret_frame((intptr_t*)uc->uc_mcontext.gregs[15/*Z_SP*/],
@@ -61,6 +66,38 @@ bool JavaThread::pd_get_top_frame_for_profiling(frame* fr_addr, void* ucontext,
       return false;
     }
 
+    if (ret_frame.fp() == NULL) {
+      // The found frame does not have a valid frame pointer.
+      // Bail out because this will create big trouble later on, either
+      //  - when using istate, calculated as (NULL - z_ijava_state_size (= 0x70 (dbg) or 0x68 (rel)) or
+      //  - when using fp() directly in safe_for_sender()
+      //
+      // There is no conclusive description (yet) how this could happen, but it does:
+      //
+      // We observed a SIGSEGV with the following stack trace (openjdk.jdk11u-dev, 2021-07-07, linuxs390x fastdebug)
+      // V  [libjvm.so+0x12c8f12]  JavaThread::pd_get_top_frame_for_profiling(frame*, void*, bool)+0x142
+      // V  [libjvm.so+0xb1020c]  JfrGetCallTrace::get_topframe(void*, frame&)+0x3c
+      // V  [libjvm.so+0xba0b08]  OSThreadSampler::protected_task(os::SuspendedThreadTaskContext const&)+0x98
+      // V  [libjvm.so+0xff33c4]  os::SuspendedThreadTask::internal_do_task()+0x14c
+      // V  [libjvm.so+0xfe3c9c]  os::SuspendedThreadTask::run()+0x24
+      // V  [libjvm.so+0xba0c66]  JfrThreadSampleClosure::sample_thread_in_java(JavaThread*, JfrStackFrame*, unsigned int)+0x66
+      // V  [libjvm.so+0xba1718]  JfrThreadSampleClosure::do_sample_thread(JavaThread*, JfrStackFrame*, unsigned int, JfrSampleType)+0x278
+      // V  [libjvm.so+0xba4f54]  JfrThreadSampler::task_stacktrace(JfrSampleType, JavaThread**) [clone .constprop.62]+0x284
+      // V  [libjvm.so+0xba5e54]  JfrThreadSampler::run()+0x2ec
+      // V  [libjvm.so+0x12adc9c]  Thread::call_run()+0x9c
+      // V  [libjvm.so+0xff5ab0]  thread_native_entry(Thread*)+0x128
+      // siginfo: si_signo: 11 (SIGSEGV), si_code: 1 (SEGV_MAPERR), si_addr: 0xfffffffffffff000
+      // failing instruction: e320 6008 0004   LG   r2,8(r0,r6)
+      // contents of r6:  0xffffffffffffff90
+      //
+      // Here is the sequence of what happens:
+      //  - ret_frame is constructed with _fp == NULL (for whatever reason)
+      //  - ijava_state_unchecked() calculates it's result as
+      //      istate = fp() - z_ijava_state_size() = NULL - 0x68 DEBUG_ONLY(-8)
+      //  - istate->method dereferences memory at offset 8 from istate
+      return false;
+    }
+
     if (ret_frame.is_interpreted_frame()) {
       frame::z_ijava_state* istate = ret_frame.ijava_state_unchecked();
       if (is_in_full_stack((address)istate)) {
