Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
8271490: [ppc] [s390]: Crash in JavaThread::pd_get_top_frame_for_prof… 
…iling

Reviewed-by: mdoerr
Backport-of: 276b07b36af01d339e48baada7a512451fe34afe
  • Loading branch information
@RealLucy @TheRealMDoerr
RealLucy authored and TheRealMDoerr committed Sep 15, 2021
1 parent 9437d45 commit 579fbcb
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 33 deletions.
38 changes: 22 additions & 16 deletions src/hotspot/cpu/ppc/frame_ppc.cpp
View file
Expand Up @@ -48,7 +48,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;
Expand Down Expand Up @@ -76,29 +75,23 @@ bool frame::safe_for_sender(JavaThread *thread) {

// An fp must be within the stack and above (but not equal) sp.
bool fp_safe = (fp <= thread->stack_base()) && (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.
bool fp_interp_safe = (fp <= thread->stack_base()) && (fp > sp) &&
((fp - sp) >= ijava_state_size);
// 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;
Expand All @@ -110,10 +103,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;;
Expand Down
40 changes: 23 additions & 17 deletions src/hotspot/cpu/s390/frame_s390.cpp
View file
Expand Up @@ -52,7 +52,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;
Expand Down Expand Up @@ -80,29 +79,23 @@ bool frame::safe_for_sender(JavaThread *thread) {

// An fp must be within the stack and above (but not equal) sp.
bool fp_safe = (fp <= thread->stack_base()) && (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.
bool fp_interp_safe = (fp <= thread->stack_base()) && (fp > sp) &&
((fp - sp) >= z_ijava_state_size);
// 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;
Expand All @@ -114,13 +107,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);
Expand Down
13 changes: 13 additions & 0 deletions src/hotspot/os_cpu/linux_ppc/thread_linux_ppc.cpp
View file
Expand Up @@ -34,6 +34,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);
}
Expand Down Expand Up @@ -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);
Expand Down
37 changes: 37 additions & 0 deletions src/hotspot/os_cpu/linux_s390/thread_linux_s390.cpp
View file
Expand Up @@ -34,6 +34,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);
}
Expand All @@ -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*/],
Expand All @@ -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 (stack_base() >= (address)istate && (address)istate > stack_end()) {
Expand Down

1 comment on commit 579fbcb

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.