From 8665ddb6d486dc1618cf268c15ceaef0fdd6383f Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Thu, 3 Sep 2026 14:49:47 +0000 Subject: [PATCH 1/6] Restore ucontext on faulted paths --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 910dc7006..5324c5b61 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -1262,6 +1262,24 @@ int HotspotSupport::walkJavaStack(StackWalkRequest& request) { return 0; } const bool prev_unwinding_java = prof_thread->is_unwinding_Java(); + + // getJavaTraceAsync() mutates the real ucontext's pc/sp/fp in place (its + // pc()/sp()/fp() are references into uc_mcontext) and restores them itself + // on every normal exit path. But a SIGSEGV that strikes mid-mutation (e.g. + // the PROBE_SP retry loop, or inside unwindStub/unwindCompiled) is caught + // by checkFault() and siglongjmp's straight here, skipping those restores. + // Since this ucontext is the same one the kernel will use to resume the + // sampled thread when this signal handler returns, snapshot it before the + // risky work and restore it here too, or the thread resumes with a + // corrupted PC/SP/FP. + HotspotStackFrame ctx_frame(ucontext); + uintptr_t saved_ctx_pc = 0, saved_ctx_sp = 0, saved_ctx_fp = 0; + if (ucontext != NULL) { + saved_ctx_pc = ctx_frame.pc(); + saved_ctx_sp = ctx_frame.sp(); + saved_ctx_fp = ctx_frame.fp(); + } + sigjmp_buf crash_protection_ctx; JmpCtxScope jmp_scope(prof_thread); @@ -1273,6 +1291,9 @@ int HotspotSupport::walkJavaStack(StackWalkRequest& request) { // A recovered siglongjmp bypasses AsyncSampleMutex destructors, so restore // the per-thread guard to its pre-walk value. prof_thread->set_unwinding_Java(prev_unwinding_java); + if (ucontext != NULL) { + ctx_frame.restore(saved_ctx_pc, saved_ctx_sp, saved_ctx_fp); + } if (truncated) { *truncated = true; } From be48e948bec893697380bac5572a5bf5c4bfadde Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Thu, 3 Sep 2026 17:34:46 +0000 Subject: [PATCH 2/6] Tests and fault injections --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 24 +++- .../test/cpp/hotspot_crash_protection_ut.cpp | 117 ++++++++++++++++++ 2 files changed, 136 insertions(+), 5 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 5324c5b61..e007968a4 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -1084,8 +1084,15 @@ int HotspotSupport::getJavaTraceAsync(void *ucontext, ASGCT_CallFrame *frames, JVMJavaThreadState state = vm_thread->state(); bool in_java = (state == _thread_in_Java || state == _thread_in_Java_trans); if (in_java && java_ctx->sp != 0) { - // skip ahead to the Java frames before calling AGCT - frame.restore((uintptr_t)java_ctx->pc, java_ctx->sp, java_ctx->fp); + // skip ahead to the Java frames before calling AGCT. + // java_ctx was populated by an earlier, separately-protected walk; fault- + // inject its values here to exercise walkJavaStack's ucontext-restore-on- + // recovered-fault path -- frame.restore() writes straight into the real + // ucontext, and this is one of the few sites that can hand it an + // outright invalid pc/sp/fp. + frame.restore((uintptr_t)INJECT_FAULT_ADDRESS_UNLIKELY(java_ctx->pc), + INJECT_FAULT_ADDRESS_UNLIKELY(java_ctx->sp), + INJECT_FAULT_ADDRESS_UNLIKELY(java_ctx->fp)); } else if (state != _thread_uninitialized) { VMJavaFrameAnchor* a = vm_thread->anchor(); if (a == nullptr || a->lastJavaSP() == 0) { @@ -1147,7 +1154,10 @@ int HotspotSupport::getJavaTraceAsync(void *ucontext, ASGCT_CallFrame *frames, trace.frames--; } for (int i = 0; trace.num_frames < 0 && i < PROBE_SP_LIMIT; i++) { - frame.sp() += sizeof(void*); + // PROBE_SP walks past the real frame boundary by design; + // fault-inject the probed sp so a poisoned value exercises the + // same recovered-fault path a genuinely bad guess would hit. + frame.sp() = INJECT_FAULT_ADDRESS_UNLIKELY(frame.sp() + sizeof(void*)); JVMSupport::jvmAsyncGetCallTrace(&trace, max_depth, ucontext); } } @@ -1172,8 +1182,12 @@ int HotspotSupport::getJavaTraceAsync(void *ucontext, ASGCT_CallFrame *frames, const void* pc = anchor->lastJavaPC(); if (sp != 0 && pc == NULL) { // We have the last Java frame anchor, but it is not marked as walkable. - // Make it walkable here - pc = ((const void**)sp)[-1]; + // Make it walkable here. + // sp comes straight from the anchor with no validation; fault-inject it + // so the unguarded dereference below exercises the sigsetjmp/siglongjmp + // recovery path installed by the caller (walkJavaStack) instead of only + // ever running against a known-good sp. + pc = ((const void**)INJECT_FAULT_ADDRESS_UNLIKELY(sp))[-1]; anchor->setLastJavaPC(pc); VMNMethod *m = CodeHeap::findNMethod(pc); diff --git a/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp b/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp index cba11fad8..fbdc3b7e6 100644 --- a/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp +++ b/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp @@ -29,6 +29,8 @@ * HotspotSupport::resolve()) * F. HotspotSupport::walkJavaStack()'s AsyncSampleMutex release on a * recovered fault + * G. HotspotSupport::walkJavaStack()'s ucontext restore on a recovered + * fault */ #include @@ -40,6 +42,7 @@ #include "jvmThread.h" #include "safeAccess.h" #include "os.h" +#include "stackFrame.h" #ifdef __linux__ @@ -576,4 +579,118 @@ TEST_F(SafeFetch64TocTouGuardTest, ZeroReturnMeansGiveUp) { munmap(page, 4096); } +// --------------------------------------------------------------------------- +// G. HotspotSupport::walkJavaStack()'s ucontext restore on a recovered fault +// +// getJavaTraceAsync() mutates the real signal ucontext's pc/sp/fp in place -- +// StackFrame::pc()/sp()/fp() are references straight into uc_mcontext -- while +// probing AsyncGetCallTrace (e.g. the PROBE_SP retry loop's `frame.sp() += +// sizeof(void*)`, or unwindStub()/unwindCompiled() writing pc()/sp()/fp() by +// reference), and restores them itself on every normal-exit path. But a +// SIGSEGV that strikes mid-mutation is caught by checkFault(), which +// siglongjmp's straight past those restores to walkJavaStack's own +// sigsetjmp. Since this ucontext is the exact one the kernel uses to resume +// the sampled thread when the signal handler returns, walkJavaStack snapshots +// it before installing its jmp ctx and restores it again in the recovery +// branch -- otherwise a fault mid-walk would leave the sampled thread's real +// register state corrupted for sigreturn. +// +// This gtest binary has no live JVM attached, so walkJavaStack() and +// getJavaTraceAsync() can't be invoked directly (they assert VM::isHotspot() +// and dereference VMThread state). This test replicates walkJavaStack's exact +// save/install/mutate/recover protocol against a real ucontext_t obtained via +// getcontext() -- no JVM needed -- to lock down the fix's contract: whatever +// is left in the ucontext at the moment of a recovered fault must be exactly +// what was there before the protected region began. +// --------------------------------------------------------------------------- + +class WalkJavaStackUcontextRestoreTest : public ::testing::Test { +protected: + void SetUp() override { + ProfiledThread::initCurrentThread(); + _pt = ProfiledThread::current(); + ASSERT_NE(nullptr, _pt); + ASSERT_EQ(0, getcontext(&_ctx)); + } + + void TearDown() override { + ProfiledThread::release(); + } + + ProfiledThread* _pt = nullptr; + ucontext_t _ctx; +}; + +// Mirrors walkJavaStack(): snapshot pc/sp/fp, install the jmp ctx, mutate the +// ucontext mid-"walk" the way getJavaTraceAsync does, then take a fault before +// it gets a chance to restore. The recovery branch's frame.restore() must undo +// the mutation -- if that call were ever dropped (the bug this fixes), the +// EXPECT_EQ calls below would see the corrupted values instead. +TEST_F(WalkJavaStackUcontextRestoreTest, FaultMidMutationRestoresOriginalRegisters) { + StackFrame frame(&_ctx); + uintptr_t saved_pc = frame.pc(); + uintptr_t saved_sp = frame.sp(); + uintptr_t saved_fp = frame.fp(); + + sigjmp_buf crash_protection_ctx; + JmpCtxScope jmp_scope(_pt); + int recovered = 0; + + if (sigsetjmp(crash_protection_ctx, 1) != 0) { + recovered++; + jmp_scope.restore(); + // The fix under test. + frame.restore(saved_pc, saved_sp, saved_fp); + } else { + jmp_scope.install(&crash_protection_ctx); + + // Simulate getJavaTraceAsync() mutating the real ucontext mid-walk + // (PROBE_SP loop / unwindStub / unwindCompiled all write pc()/sp()/ + // fp() directly). + frame.sp() += sizeof(void*); + frame.fp() = saved_sp; + frame.pc() = saved_pc + 0x1234; + + // A fault strikes before getJavaTraceAsync reaches its own restore. + // Simulate checkFault(): siglongjmp through whatever is installed. + siglongjmp(*_pt->getJmpCtx(), 1); + FAIL() << "unreachable: siglongjmp does not return"; + } + + EXPECT_EQ(1, recovered); + EXPECT_EQ(saved_pc, frame.pc()); + EXPECT_EQ(saved_sp, frame.sp()); + EXPECT_EQ(saved_fp, frame.fp()); + EXPECT_FALSE(_pt->isProtected()); +} + +// walkJavaStack guards the restore with `if (ucontext != NULL)`, since +// ucontext can legitimately be null (e.g. malloc/socket hooks sampled outside +// any signal context). The recovery branch must not dereference a null +// StackFrame in that case. +TEST_F(WalkJavaStackUcontextRestoreTest, NullUcontextSkipsRestoreWithoutCrashing) { + void* ucontext = nullptr; + StackFrame frame(ucontext); + uintptr_t saved_pc = 0, saved_sp = 0, saved_fp = 0; + + sigjmp_buf crash_protection_ctx; + JmpCtxScope jmp_scope(_pt); + int recovered = 0; + + if (sigsetjmp(crash_protection_ctx, 1) != 0) { + recovered++; + jmp_scope.restore(); + if (ucontext != nullptr) { + frame.restore(saved_pc, saved_sp, saved_fp); + } + } else { + jmp_scope.install(&crash_protection_ctx); + siglongjmp(*_pt->getJmpCtx(), 1); + FAIL() << "unreachable: siglongjmp does not return"; + } + + EXPECT_EQ(1, recovered); + EXPECT_FALSE(_pt->isProtected()); +} + #endif // __linux__ From 77195ed6e81c31a446e5ddda421dc8d7b6b18b81 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Thu, 3 Sep 2026 18:15:42 +0000 Subject: [PATCH 3/6] Fix test --- .../src/test/cpp/hotspot_crash_protection_ut.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp b/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp index fbdc3b7e6..3e3842477 100644 --- a/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp +++ b/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp @@ -598,10 +598,10 @@ TEST_F(SafeFetch64TocTouGuardTest, ZeroReturnMeansGiveUp) { // This gtest binary has no live JVM attached, so walkJavaStack() and // getJavaTraceAsync() can't be invoked directly (they assert VM::isHotspot() // and dereference VMThread state). This test replicates walkJavaStack's exact -// save/install/mutate/recover protocol against a real ucontext_t obtained via -// getcontext() -- no JVM needed -- to lock down the fix's contract: whatever -// is left in the ucontext at the moment of a recovered fault must be exactly -// what was there before the protected region began. +// save/install/mutate/recover protocol against a ucontext_t to lock down the +// fix's contract: whatever is left in the ucontext at the moment of a +// recovered fault must be exactly what was there before the protected region +// began. // --------------------------------------------------------------------------- class WalkJavaStackUcontextRestoreTest : public ::testing::Test { @@ -610,7 +610,11 @@ class WalkJavaStackUcontextRestoreTest : public ::testing::Test { ProfiledThread::initCurrentThread(); _pt = ProfiledThread::current(); ASSERT_NE(nullptr, _pt); - ASSERT_EQ(0, getcontext(&_ctx)); + // Zero-initialized rather than populated via getcontext() -- musl + // doesn't provide getcontext(), and this test only round-trips + // arbitrary values through StackFrame::pc()/sp()/fp() (references + // into uc_mcontext), so a real, live context is unnecessary here. + _ctx = ucontext_t{}; } void TearDown() override { From 8a1214038c48596c7b1e8102587831699acad9d6 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Thu, 3 Sep 2026 18:45:18 +0000 Subject: [PATCH 4/6] Fix --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 13 +- .../test/cpp/hotspot_crash_protection_ut.cpp | 111 +++++++++++++++--- 2 files changed, 99 insertions(+), 25 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index e007968a4..59e1f493b 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -1085,14 +1085,7 @@ int HotspotSupport::getJavaTraceAsync(void *ucontext, ASGCT_CallFrame *frames, bool in_java = (state == _thread_in_Java || state == _thread_in_Java_trans); if (in_java && java_ctx->sp != 0) { // skip ahead to the Java frames before calling AGCT. - // java_ctx was populated by an earlier, separately-protected walk; fault- - // inject its values here to exercise walkJavaStack's ucontext-restore-on- - // recovered-fault path -- frame.restore() writes straight into the real - // ucontext, and this is one of the few sites that can hand it an - // outright invalid pc/sp/fp. - frame.restore((uintptr_t)INJECT_FAULT_ADDRESS_UNLIKELY(java_ctx->pc), - INJECT_FAULT_ADDRESS_UNLIKELY(java_ctx->sp), - INJECT_FAULT_ADDRESS_UNLIKELY(java_ctx->fp)); + frame.restore((uintptr_t)java_ctx->pc, java_ctx->sp, java_ctx->fp); } else if (state != _thread_uninitialized) { VMJavaFrameAnchor* a = vm_thread->anchor(); if (a == nullptr || a->lastJavaSP() == 0) { @@ -1155,9 +1148,7 @@ int HotspotSupport::getJavaTraceAsync(void *ucontext, ASGCT_CallFrame *frames, } for (int i = 0; trace.num_frames < 0 && i < PROBE_SP_LIMIT; i++) { // PROBE_SP walks past the real frame boundary by design; - // fault-inject the probed sp so a poisoned value exercises the - // same recovered-fault path a genuinely bad guess would hit. - frame.sp() = INJECT_FAULT_ADDRESS_UNLIKELY(frame.sp() + sizeof(void*)); + frame.sp() = frame.sp() + sizeof(void*); JVMSupport::jvmAsyncGetCallTrace(&trace, max_depth, ucontext); } } diff --git a/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp b/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp index 3e3842477..740663e4f 100644 --- a/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp +++ b/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp @@ -43,6 +43,7 @@ #include "safeAccess.h" #include "os.h" #include "stackFrame.h" +#include "hotspot/hotspotSupport.h" #ifdef __linux__ @@ -597,19 +598,40 @@ TEST_F(SafeFetch64TocTouGuardTest, ZeroReturnMeansGiveUp) { // // This gtest binary has no live JVM attached, so walkJavaStack() and // getJavaTraceAsync() can't be invoked directly (they assert VM::isHotspot() -// and dereference VMThread state). This test replicates walkJavaStack's exact -// save/install/mutate/recover protocol against a ucontext_t to lock down the -// fix's contract: whatever is left in the ucontext at the moment of a -// recovered fault must be exactly what was there before the protected region -// began. +// and dereference VMThread state). These tests replicate walkJavaStack's +// exact save/install/mutate/recover protocol against a ucontext_t, but drive +// recovery through the REAL Profiler::checkFault() -- not a hand-rolled +// siglongjmp -- so its `pc < min || pc >= max` address-range gate +// (profiler_min_address/_max_address, see stackWalker_ut.cpp's +// StackWalkerCrashRecoveryTest for the same pattern) is exercised for real. +// That gate matters here specifically: a fault raised while HotSpot's own +// AsyncGetCallTrace (libjvm.so) dereferences a poisoned sp/pc/fp has its +// faulting instruction *inside libjvm.so*, not inside this library, so +// checkFault() correctly refuses to recover it -- which means +// walkJavaStack's frame.restore() fix is never reached, and the mutated +// ucontext stays corrupted. SetUp() installs a real range via the +// UNIT_TEST-only Profiler::setAddressRangeForTest() so both sides of that +// gate -- recovered (pc inside range) and rejected (pc outside range) -- are +// exercised, rather than only the hand-simulated "always recovers" path. // --------------------------------------------------------------------------- class WalkJavaStackUcontextRestoreTest : public ::testing::Test { protected: + // Comfortably covers walkJavaStack's own compiled body in any build + // config, while remaining far smaller than the 256MB offset used below + // to land clearly outside the range. + static constexpr uintptr_t kRangeMargin = 256 * 1024; + void SetUp() override { ProfiledThread::initCurrentThread(); _pt = ProfiledThread::current(); ASSERT_NE(nullptr, _pt); + + uintptr_t self_pc = reinterpret_cast(&HotspotSupport::walkJavaStack); + _range_lo = self_pc - kRangeMargin; + _range_hi = self_pc + kRangeMargin; + Profiler::setAddressRangeForTest(_range_lo, _range_hi); + // Zero-initialized rather than populated via getcontext() -- musl // doesn't provide getcontext(), and this test only round-trips // arbitrary values through StackFrame::pc()/sp()/fp() (references @@ -618,19 +640,25 @@ class WalkJavaStackUcontextRestoreTest : public ::testing::Test { } void TearDown() override { + Profiler::resetAddressRangeForTest(); ProfiledThread::release(); } ProfiledThread* _pt = nullptr; ucontext_t _ctx; + uintptr_t _range_lo = 0; + uintptr_t _range_hi = 0; }; // Mirrors walkJavaStack(): snapshot pc/sp/fp, install the jmp ctx, mutate the -// ucontext mid-"walk" the way getJavaTraceAsync does, then take a fault before -// it gets a chance to restore. The recovery branch's frame.restore() must undo -// the mutation -- if that call were ever dropped (the bug this fixes), the -// EXPECT_EQ calls below would see the corrupted values instead. -TEST_F(WalkJavaStackUcontextRestoreTest, FaultMidMutationRestoresOriginalRegisters) { +// ucontext mid-"walk" the way getJavaTraceAsync does, then take a fault whose +// own faulting instruction lands inside this library (e.g. a direct +// dereference of the poisoned sp, as walkVM's existing +// INJECT_FAULT_ADDRESS_UNLIKELY sites do) -- so checkFault() must recover. +// The recovery branch's frame.restore() must undo the mutation -- if that +// call were ever dropped (the bug this fixes), the EXPECT_EQ calls below +// would see the corrupted values instead. +TEST_F(WalkJavaStackUcontextRestoreTest, FaultInsideProfilerRangeRecoversAndRestoresUcontext) { StackFrame frame(&_ctx); uintptr_t saved_pc = frame.pc(); uintptr_t saved_sp = frame.sp(); @@ -655,10 +683,15 @@ TEST_F(WalkJavaStackUcontextRestoreTest, FaultMidMutationRestoresOriginalRegiste frame.fp() = saved_sp; frame.pc() = saved_pc + 0x1234; - // A fault strikes before getJavaTraceAsync reaches its own restore. - // Simulate checkFault(): siglongjmp through whatever is installed. - siglongjmp(*_pt->getJmpCtx(), 1); - FAIL() << "unreachable: siglongjmp does not return"; + // The SIGSEGV's own delivery ucontext -- a distinct object from + // _ctx above -- whose faulting pc sits inside the installed range. + ucontext_t fault_uc{}; + StackFrame(&fault_uc).pc() = _range_lo + kRangeMargin; + + siginfo_t si{}; + si.si_addr = reinterpret_cast(1); + Profiler::checkFault(_pt, &si, &fault_uc); + FAIL() << "unreachable: checkFault() must siglongjmp for an in-range pc"; } EXPECT_EQ(1, recovered); @@ -668,6 +701,56 @@ TEST_F(WalkJavaStackUcontextRestoreTest, FaultMidMutationRestoresOriginalRegiste EXPECT_FALSE(_pt->isProtected()); } +// The counterpart that FaultInsideProfilerRangeRecoversAndRestoresUcontext +// alone can't catch: a fault whose instruction pointer falls OUTSIDE the +// profiler's own range -- standing in for a fault raised deep inside +// libjvm.so while AsyncGetCallTrace dereferences a poisoned sp/pc/fp (see +// getJavaTraceAsync's java_ctx-restore and PROBE_SP-loop fault-injection +// sites). checkFault() must not recover such a fault, which means +// walkJavaStack's frame.restore() fix never runs and the mutated ucontext is +// left exactly as corrupted as the injection left it. +TEST_F(WalkJavaStackUcontextRestoreTest, FaultOutsideProfilerRangeIsNotRecoveredAndLeavesUcontextCorrupted) { + StackFrame frame(&_ctx); + uintptr_t saved_pc = frame.pc(); + uintptr_t saved_sp = frame.sp(); + + sigjmp_buf crash_protection_ctx; + JmpCtxScope jmp_scope(_pt); + ASSERT_EQ(0, sigsetjmp(crash_protection_ctx, 1)) + << "must not have recovered -- this branch only runs once, forward"; + jmp_scope.install(&crash_protection_ctx); + + // Same mutation getJavaTraceAsync() performs right before handing + // sp/pc/fp to jvmAsyncGetCallTrace(). + frame.sp() += sizeof(void*); + frame.fp() = saved_sp; + frame.pc() = saved_pc + 0x1234; + + uintptr_t mutated_pc = frame.pc(); + uintptr_t mutated_sp = frame.sp(); + uintptr_t mutated_fp = frame.fp(); + + // The SIGSEGV's own delivery ucontext, standing in for a fault inside + // libjvm.so -- its pc sits 256MB past the installed range, far beyond + // kRangeMargin regardless of build config. + ucontext_t fault_uc{}; + StackFrame(&fault_uc).pc() = _range_hi + (256u * 1024 * 1024); + + siginfo_t si{}; + si.si_addr = reinterpret_cast(1); + Profiler::checkFault(_pt, &si, &fault_uc); + // Must fall through to here -- checkFault must not siglongjmp for a pc + // outside the installed range. + + jmp_scope.restore(); + + EXPECT_EQ(mutated_pc, frame.pc()) + << "an unrecovered fault must leave the mutated ucontext untouched -- " + "walkJavaStack's frame.restore() fix is never reached in this case"; + EXPECT_EQ(mutated_sp, frame.sp()); + EXPECT_EQ(mutated_fp, frame.fp()); +} + // walkJavaStack guards the restore with `if (ucontext != NULL)`, since // ucontext can legitimately be null (e.g. malloc/socket hooks sampled outside // any signal context). The recovery branch must not dereference a null From a6bc56809a5ca0b70dd92df1dc66832ba00ba08d Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Fri, 4 Sep 2026 13:21:26 +0000 Subject: [PATCH 5/6] Dedup code --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 182 +++++++--------- .../src/main/cpp/hotspot/hotspotSupport.h | 49 ++++- ddprof-lib/src/main/cpp/stackFrame.h | 36 ++++ .../test/cpp/hotspot_crash_protection_ut.cpp | 197 +++++++++--------- 4 files changed, 261 insertions(+), 203 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 59e1f493b..1f7d9ea68 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -1049,23 +1049,24 @@ int HotspotSupport::getJavaTraceAsync(void *ucontext, ASGCT_CallFrame *frames, } HotspotStackFrame frame(ucontext); - uintptr_t saved_pc = 0, saved_sp = 0, saved_fp = 0; + // Snapshot pc/sp/fp before this function starts feeding them to HotSpot's + // own AsyncGetCallTrace below (it mutates them in place via frame.restore() + // / frame.unwindStub() / frame.unwindCompiled() to try alternate frames), + // so they can be put back once AGCT is done. Shared with walkJavaStack()'s + // fault-recovery restore -- see StackFrame::RegisterSnapshot. + StackFrame::RegisterSnapshot ctx_snapshot(ucontext); if (ucontext != NULL) { - saved_pc = frame.pc(); - saved_sp = frame.sp(); - saved_fp = frame.fp(); - - if (JitCodeCache::isCallStub((const void *)saved_pc)) { + if (JitCodeCache::isCallStub((const void *)ctx_snapshot.pc())) { // call_stub is unsafe to walk frames->bci = BCI_ERROR; frames->method_id = (jmethodID) "call_stub"; return 1; } - if (!VMStructs::isSafeToWalk(saved_pc)) { + if (!VMStructs::isSafeToWalk(ctx_snapshot.pc())) { frames->bci = BCI_NATIVE_FRAME; CodeBlob *codeBlob = - VMStructs::libjvm()->findBlobByAddress((const void *)saved_pc); + VMStructs::libjvm()->findBlobByAddress((const void *)ctx_snapshot.pc()); if (codeBlob) { frames->method_id = (jmethodID)codeBlob->_name; } else { @@ -1109,7 +1110,7 @@ int HotspotSupport::getJavaTraceAsync(void *ucontext, ASGCT_CallFrame *frames, JVMSupport::jvmAsyncGetCallTrace(&trace, max_depth, ucontext); if (trace.num_frames > 0) { - frame.restore(saved_pc, saved_sp, saved_fp); + ctx_snapshot.restore(); return trace.num_frames; } @@ -1220,12 +1221,12 @@ int HotspotSupport::getJavaTraceAsync(void *ucontext, ASGCT_CallFrame *frames, if (anchor == NULL || anchor->lastJavaSP() == 0) { // Do not add 'GC_active' for threads with no Java frames, e.g. Compiler // threads - frame.restore(saved_pc, saved_sp, saved_fp); + ctx_snapshot.restore(); return 0; } } - frame.restore(saved_pc, saved_sp, saved_fp); + ctx_snapshot.restore(); if (trace.num_frames > 0) { return trace.num_frames + (trace.frames - frames); @@ -1253,112 +1254,77 @@ int HotspotSupport::walkJavaStack(StackWalkRequest& request) { bool* truncated = request.truncated; u32 lock_index = request.lock_index; - volatile int java_frames = 0; - // walkVM() installs its own sigsetjmp/siglongjmp crash protection (chained - // with any pre-existing jmp ctx, see the comment in walkVM), but the - // getJavaTraceAsync() path below runs without one: it dereferences - // VMThread/anchor state directly and calls into HotSpot's own - // AsyncGetCallTrace. Install a jmp ctx here too, so a SIGSEGV anywhere in - // walkJavaStack, except HotSpot's AsyncGetCallTrace call, is caught by - // Profiler::checkFault() and siglongjmp'd back here instead of crashing the process. ProfiledThread* prof_thread = ProfiledThread::acquireCurrent(); if (prof_thread == nullptr) { Counters::increment(SAMPLES_DROPPED_THREAD_LOCAL); return 0; } - const bool prev_unwinding_java = prof_thread->is_unwinding_Java(); - - // getJavaTraceAsync() mutates the real ucontext's pc/sp/fp in place (its - // pc()/sp()/fp() are references into uc_mcontext) and restores them itself - // on every normal exit path. But a SIGSEGV that strikes mid-mutation (e.g. - // the PROBE_SP retry loop, or inside unwindStub/unwindCompiled) is caught - // by checkFault() and siglongjmp's straight here, skipping those restores. - // Since this ucontext is the same one the kernel will use to resume the - // sampled thread when this signal handler returns, snapshot it before the - // risky work and restore it here too, or the thread resumes with a - // corrupted PC/SP/FP. - HotspotStackFrame ctx_frame(ucontext); - uintptr_t saved_ctx_pc = 0, saved_ctx_sp = 0, saved_ctx_fp = 0; - if (ucontext != NULL) { - saved_ctx_pc = ctx_frame.pc(); - saved_ctx_sp = ctx_frame.sp(); - saved_ctx_fp = ctx_frame.fp(); - } - sigjmp_buf crash_protection_ctx; - JmpCtxScope jmp_scope(prof_thread); - - if (sigsetjmp(crash_protection_ctx, 1) != 0) { - // checkFault() does a siglongjmp from inside segvHandler, bypassing - // segvHandler's SignalHandlerScope destructor. Compensate. - SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); - jmp_scope.restore(); - // A recovered siglongjmp bypasses AsyncSampleMutex destructors, so restore - // the per-thread guard to its pre-walk value. - prof_thread->set_unwinding_Java(prev_unwinding_java); - if (ucontext != NULL) { - ctx_frame.restore(saved_ctx_pc, saved_ctx_sp, saved_ctx_fp); - } - if (truncated) { - *truncated = true; - } - return java_frames; - } - jmp_scope.install(&crash_protection_ctx); - - if (features.mixed) { - java_frames = walkVM(ucontext, frames, max_depth, features, eventTypeFromBCI(request.event_type), lock_index, truncated); - } else if (isHookPrefixedSample(request.event_type)) { - if (cstack >= CSTACK_VM) { + // walkVM() installs its own sigsetjmp/siglongjmp crash protection (chained + // with any pre-existing jmp ctx, see the comment in walkVM), but the + // getJavaTraceAsync() path below runs without one: it dereferences + // VMThread/anchor state directly, calls into HotSpot's own + // AsyncGetCallTrace, and mutates the real ucontext's pc/sp/fp in place + // while doing so. withUcontextFaultRecovery() installs a jmp ctx around + // both paths, so a SIGSEGV anywhere in this dispatch (except HotSpot's own + // AsyncGetCallTrace call) is caught by Profiler::checkFault() and + // recovered instead of crashing the process -- see its own comment for why + // it also restores the ucontext. + return withUcontextFaultRecovery(ucontext, prof_thread, truncated, [&]() -> int { + int java_frames = 0; + if (features.mixed) { java_frames = walkVM(ucontext, frames, max_depth, features, eventTypeFromBCI(request.event_type), lock_index, truncated); - } else { - AsyncSampleMutex mutex(ProfiledThread::current()); - if (mutex.acquired()) { - java_frames = getJavaTraceAsync(ucontext, frames, max_depth, java_ctx, truncated); - if (java_frames > 0 && java_ctx->pc != NULL && VMStructs::hasMethodStructs()) { - VMNMethod* nmethod = CodeHeap::findNMethod(java_ctx->pc); - if (nmethod != NULL) { - fillFrameTypes(frames, java_frames, nmethod); - } - } - } - if (java_frames > 0 && VM::hotspot_version() >= 21 && java_frames < max_depth) { - VMThread* carrier = VMThread::current(); - if (carrier != nullptr && carrier->isCarryingVirtualThread()) { - frames[java_frames].bci = BCI_NATIVE_FRAME; - frames[java_frames].method_id = (jmethodID) "JVM Continuation"; - LP64_ONLY(frames[java_frames].padding = 0;) - java_frames++; - } - } - } - } else if (request.event_type == BCI_CPU || request.event_type == BCI_WALL) { - if (cstack >= CSTACK_VM) { + } else if (isHookPrefixedSample(request.event_type)) { + if (cstack >= CSTACK_VM) { java_frames = walkVM(ucontext, frames, max_depth, features, eventTypeFromBCI(request.event_type), lock_index, truncated); - } else { - AsyncSampleMutex mutex(ProfiledThread::current()); - if (mutex.acquired()) { - java_frames = getJavaTraceAsync(ucontext, frames, max_depth, java_ctx, truncated); - if (java_frames > 0 && java_ctx->pc != NULL && VMStructs::hasMethodStructs()) { - VMNMethod* nmethod = CodeHeap::findNMethod(java_ctx->pc); - if (nmethod != NULL) { - fillFrameTypes(frames, java_frames, nmethod); - } - } - } - if (java_frames > 0 && VM::hotspot_version() >= 21 && java_frames < max_depth) { - VMThread* carrier = VMThread::current(); - if (carrier != nullptr && carrier->isCarryingVirtualThread()) { - frames[java_frames].bci = BCI_NATIVE_FRAME; - frames[java_frames].method_id = (jmethodID) "JVM Continuation"; - LP64_ONLY(frames[java_frames].padding = 0;) - java_frames++; - } - } + } else { + AsyncSampleMutex mutex(ProfiledThread::current()); + if (mutex.acquired()) { + java_frames = getJavaTraceAsync(ucontext, frames, max_depth, java_ctx, truncated); + if (java_frames > 0 && java_ctx->pc != NULL && VMStructs::hasMethodStructs()) { + VMNMethod* nmethod = CodeHeap::findNMethod(java_ctx->pc); + if (nmethod != NULL) { + fillFrameTypes(frames, java_frames, nmethod); + } + } + } + if (java_frames > 0 && VM::hotspot_version() >= 21 && java_frames < max_depth) { + VMThread* carrier = VMThread::current(); + if (carrier != nullptr && carrier->isCarryingVirtualThread()) { + frames[java_frames].bci = BCI_NATIVE_FRAME; + frames[java_frames].method_id = (jmethodID) "JVM Continuation"; + LP64_ONLY(frames[java_frames].padding = 0;) + java_frames++; + } + } + } + } else if (request.event_type == BCI_CPU || request.event_type == BCI_WALL) { + if (cstack >= CSTACK_VM) { + java_frames = walkVM(ucontext, frames, max_depth, features, eventTypeFromBCI(request.event_type), lock_index, truncated); + } else { + AsyncSampleMutex mutex(ProfiledThread::current()); + if (mutex.acquired()) { + java_frames = getJavaTraceAsync(ucontext, frames, max_depth, java_ctx, truncated); + if (java_frames > 0 && java_ctx->pc != NULL && VMStructs::hasMethodStructs()) { + VMNMethod* nmethod = CodeHeap::findNMethod(java_ctx->pc); + if (nmethod != NULL) { + fillFrameTypes(frames, java_frames, nmethod); + } + } + } + if (java_frames > 0 && VM::hotspot_version() >= 21 && java_frames < max_depth) { + VMThread* carrier = VMThread::current(); + if (carrier != nullptr && carrier->isCarryingVirtualThread()) { + frames[java_frames].bci = BCI_NATIVE_FRAME; + frames[java_frames].method_id = (jmethodID) "JVM Continuation"; + LP64_ONLY(frames[java_frames].padding = 0;) + java_frames++; + } + } + } } - } - - return java_frames; + return java_frames; + }); } static void patchClassLoaderData(JNIEnv* jni, jclass klass) { diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h index 7617403fa..8ac5067c7 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h @@ -7,16 +7,17 @@ #ifndef _HOTSPOT_HOTSPOTSUPPORT_H #define _HOTSPOT_HOTSPOTSUPPORT_H +#include "guards.h" #include "hotspot/hotspotStackFrame.h" #include "hotspot/jitCodeCache.h" #include "frame.h" #include "stackFrame.h" #include "stackWalker.h" +#include "threadLocalData.inline.h" #include #include -class ProfiledThread; class VMMethod; class HotspotSupport { @@ -38,7 +39,51 @@ class HotspotSupport { static bool loadMethodIDsIfNeededImpl(jvmtiEnv *jvmti, JNIEnv *jni, jclass klass, bool load_all); public: static void initClassloaderInfo(JNIEnv* jni); - + + // Runs `work` under the ucontext-fault-recovery protocol walkJavaStack() + // needs around getJavaTraceAsync(): installs a sigsetjmp/siglongjmp + // crash-protection scope chained on prof_thread (JmpCtxScope, guards.h), + // and if a SIGSEGV strikes and is recovered by Profiler::checkFault(), + // restores ucontext's pc/sp/fp to what they were before `work` ran. + // getJavaTraceAsync() mutates those registers in place (its pc()/sp()/ + // fp() are references into uc_mcontext) and normally restores them + // itself, but a fault mid-mutation (e.g. the PROBE_SP retry loop, or + // inside unwindStub()/unwindCompiled()) skips that restore -- and this + // ucontext is the exact one the kernel uses to resume the sampled + // thread when the signal handler returns. + // + // Extracted into one place, rather than hand-rolled separately in + // walkJavaStack(), so production and its regression test invoke the + // identical recovery branch -- see hotspot_crash_protection_ut.cpp's + // WalkJavaStackUcontextRestoreTest. A template rather than + // std::function so the hot sample path pays no allocation for + // captures. + template + static int withUcontextFaultRecovery(void* ucontext, ProfiledThread* prof_thread, bool* truncated, Fn&& work) { + const bool prev_unwinding_java = prof_thread->is_unwinding_Java(); + StackFrame::RegisterSnapshot ctx_snapshot(ucontext); + + sigjmp_buf crash_protection_ctx; + JmpCtxScope jmp_scope(prof_thread); + + if (sigsetjmp(crash_protection_ctx, 1) != 0) { + // checkFault() does a siglongjmp from inside segvHandler, bypassing + // segvHandler's SignalHandlerScope destructor. Compensate. + SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); + jmp_scope.restore(); + // A recovered siglongjmp bypasses AsyncSampleMutex destructors, so + // restore the per-thread guard to its pre-walk value. + prof_thread->set_unwinding_Java(prev_unwinding_java); + ctx_snapshot.restore(); + if (truncated) { + *truncated = true; + } + return 0; + } + jmp_scope.install(&crash_protection_ctx); + return work(); + } + static int walkJavaStack(StackWalkRequest& request); static inline bool canUnwind(const StackFrame& frame, const void*& pc) { return HotspotStackFrame::unwindAtomicStub(frame, pc); diff --git a/ddprof-lib/src/main/cpp/stackFrame.h b/ddprof-lib/src/main/cpp/stackFrame.h index 83bd24d66..16f7ca84b 100644 --- a/ddprof-lib/src/main/cpp/stackFrame.h +++ b/ddprof-lib/src/main/cpp/stackFrame.h @@ -33,6 +33,42 @@ class StackFrame { } } + // Captures pc/sp/fp for a later restore() -- the null-safe save/restore + // boilerplate needed around code that mutates the real ucontext in place + // (e.g. HotspotSupport::getJavaTraceAsync()'s PROBE_SP loop, or + // unwindStub()/unwindCompiled() writing pc()/sp()/fp() by reference). + // Capturing is a no-op when ucontext is null (pc()/sp()/fp() themselves + // are not null-safe); restore() delegates to StackFrame::restore() above, + // which already is. + // + // Must be restored via an explicit restore() call, never a destructor: + // Profiler::checkFault()'s siglongjmp bypasses destructors, so RAII alone + // can't reach code here -- the same reason JmpCtxScope::restore() must be + // called explicitly (see guards.h). + class RegisterSnapshot { + public: + explicit RegisterSnapshot(void* ucontext) : _ucontext(ucontext) { + if (_ucontext != nullptr) { + StackFrame frame(_ucontext); + _pc = frame.pc(); + _sp = frame.sp(); + _fp = frame.fp(); + } + } + + void restore() const { + StackFrame(_ucontext).restore(_pc, _sp, _fp); + } + + uintptr_t pc() const { return _pc; } + uintptr_t sp() const { return _sp; } + uintptr_t fp() const { return _fp; } + + private: + void* _ucontext; + uintptr_t _pc = 0, _sp = 0, _fp = 0; + }; + uintptr_t stackAt(int slot) { return ((uintptr_t*)sp())[slot]; } diff --git a/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp b/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp index 740663e4f..43c9f7231 100644 --- a/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp +++ b/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp @@ -581,7 +581,9 @@ TEST_F(SafeFetch64TocTouGuardTest, ZeroReturnMeansGiveUp) { } // --------------------------------------------------------------------------- -// G. HotspotSupport::walkJavaStack()'s ucontext restore on a recovered fault +// G. HotspotSupport::withUcontextFaultRecovery()'s ucontext restore on a +// recovered fault -- the crash-protection wrapper walkJavaStack() uses +// around getJavaTraceAsync() // // getJavaTraceAsync() mutates the real signal ucontext's pc/sp/fp in place -- // StackFrame::pc()/sp()/fp() are references straight into uc_mcontext -- while @@ -589,30 +591,36 @@ TEST_F(SafeFetch64TocTouGuardTest, ZeroReturnMeansGiveUp) { // sizeof(void*)`, or unwindStub()/unwindCompiled() writing pc()/sp()/fp() by // reference), and restores them itself on every normal-exit path. But a // SIGSEGV that strikes mid-mutation is caught by checkFault(), which -// siglongjmp's straight past those restores to walkJavaStack's own +// siglongjmp's straight past those restores to withUcontextFaultRecovery()'s // sigsetjmp. Since this ucontext is the exact one the kernel uses to resume -// the sampled thread when the signal handler returns, walkJavaStack snapshots -// it before installing its jmp ctx and restores it again in the recovery -// branch -- otherwise a fault mid-walk would leave the sampled thread's real -// register state corrupted for sigreturn. +// the sampled thread when the signal handler returns, withUcontextFaultRecovery() +// snapshots it before running the protected work and restores it again in the +// recovery branch -- otherwise a fault mid-walk would leave the sampled +// thread's real register state corrupted for sigreturn. // -// This gtest binary has no live JVM attached, so walkJavaStack() and -// getJavaTraceAsync() can't be invoked directly (they assert VM::isHotspot() -// and dereference VMThread state). These tests replicate walkJavaStack's -// exact save/install/mutate/recover protocol against a ucontext_t, but drive -// recovery through the REAL Profiler::checkFault() -- not a hand-rolled -// siglongjmp -- so its `pc < min || pc >= max` address-range gate +// These tests call HotspotSupport::withUcontextFaultRecovery() directly -- +// the exact function walkJavaStack() delegates to -- rather than replicating +// its sigsetjmp/restore protocol by hand: walkJavaStack() itself can't be +// invoked here (this gtest binary has no live JVM, and its dispatch asserts +// VM::isHotspot() / dereferences VMThread state), but the fault-recovery +// wrapper it calls has no such dependency, so calling it directly means a +// regression to the real recovery branch (e.g. dropping its ctx_snapshot. +// restore() call) fails these tests too, instead of only a hand-rolled copy +// of the same logic. +// +// Recovery is driven through the REAL Profiler::checkFault() -- not a +// simulated siglongjmp -- so its `pc < min || pc >= max` address-range gate // (profiler_min_address/_max_address, see stackWalker_ut.cpp's // StackWalkerCrashRecoveryTest for the same pattern) is exercised for real. // That gate matters here specifically: a fault raised while HotSpot's own // AsyncGetCallTrace (libjvm.so) dereferences a poisoned sp/pc/fp has its // faulting instruction *inside libjvm.so*, not inside this library, so // checkFault() correctly refuses to recover it -- which means -// walkJavaStack's frame.restore() fix is never reached, and the mutated +// withUcontextFaultRecovery()'s restore is never reached, and the mutated // ucontext stays corrupted. SetUp() installs a real range via the // UNIT_TEST-only Profiler::setAddressRangeForTest() so both sides of that // gate -- recovered (pc inside range) and rejected (pc outside range) -- are -// exercised, rather than only the hand-simulated "always recovers" path. +// exercised, rather than only the "always recovers" path. // --------------------------------------------------------------------------- class WalkJavaStackUcontextRestoreTest : public ::testing::Test { @@ -650,32 +658,24 @@ class WalkJavaStackUcontextRestoreTest : public ::testing::Test { uintptr_t _range_hi = 0; }; -// Mirrors walkJavaStack(): snapshot pc/sp/fp, install the jmp ctx, mutate the -// ucontext mid-"walk" the way getJavaTraceAsync does, then take a fault whose -// own faulting instruction lands inside this library (e.g. a direct -// dereference of the poisoned sp, as walkVM's existing -// INJECT_FAULT_ADDRESS_UNLIKELY sites do) -- so checkFault() must recover. -// The recovery branch's frame.restore() must undo the mutation -- if that -// call were ever dropped (the bug this fixes), the EXPECT_EQ calls below -// would see the corrupted values instead. +// Drives HotspotSupport::withUcontextFaultRecovery() -- the real production +// function, not a replica -- with `work` mutating the ucontext mid-"walk" the +// way getJavaTraceAsync does, then taking a fault whose own faulting +// instruction lands inside this library (e.g. a direct dereference of the +// poisoned sp, as walkVM's existing INJECT_FAULT_ADDRESS_UNLIKELY sites do) +// -- so checkFault() must recover. The recovery branch's ctx_snapshot. +// restore() must undo the mutation -- if that call were ever dropped (the bug +// this fixes), the EXPECT_EQ calls below would see the corrupted values +// instead, because they're reading back through the very ucontext +// withUcontextFaultRecovery() owns. TEST_F(WalkJavaStackUcontextRestoreTest, FaultInsideProfilerRangeRecoversAndRestoresUcontext) { StackFrame frame(&_ctx); uintptr_t saved_pc = frame.pc(); uintptr_t saved_sp = frame.sp(); uintptr_t saved_fp = frame.fp(); + bool truncated = false; - sigjmp_buf crash_protection_ctx; - JmpCtxScope jmp_scope(_pt); - int recovered = 0; - - if (sigsetjmp(crash_protection_ctx, 1) != 0) { - recovered++; - jmp_scope.restore(); - // The fix under test. - frame.restore(saved_pc, saved_sp, saved_fp); - } else { - jmp_scope.install(&crash_protection_ctx); - + int result = HotspotSupport::withUcontextFaultRecovery(&_ctx, _pt, &truncated, [&]() -> int { // Simulate getJavaTraceAsync() mutating the real ucontext mid-walk // (PROBE_SP loop / unwindStub / unwindCompiled all write pc()/sp()/ // fp() directly). @@ -690,11 +690,22 @@ TEST_F(WalkJavaStackUcontextRestoreTest, FaultInsideProfilerRangeRecoversAndRest siginfo_t si{}; si.si_addr = reinterpret_cast(1); + // A real SIGSEGV delivery enters this via segvHandler's + // SignalHandlerScope before reaching checkFault(); calling checkFault() + // directly (no real fault, deterministic pc) means mimicking that + // entry ourselves, so SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP()'s + // compensating exitSignalScope() -- run inside the recovery branch + // below -- has a matching enter to unwind instead of underflowing. + _pt->enterSignalScope(); Profiler::checkFault(_pt, &si, &fault_uc); - FAIL() << "unreachable: checkFault() must siglongjmp for an in-range pc"; - } - - EXPECT_EQ(1, recovered); + // Not FAIL(): that macro does a bare `return;`, which doesn't + // compile in a lambda declared to return int. + ADD_FAILURE() << "unreachable: checkFault() must siglongjmp for an in-range pc"; + return -1; + }); + + EXPECT_EQ(0, result); + EXPECT_TRUE(truncated); EXPECT_EQ(saved_pc, frame.pc()); EXPECT_EQ(saved_sp, frame.sp()); EXPECT_EQ(saved_fp, frame.fp()); @@ -705,78 +716,78 @@ TEST_F(WalkJavaStackUcontextRestoreTest, FaultInsideProfilerRangeRecoversAndRest // alone can't catch: a fault whose instruction pointer falls OUTSIDE the // profiler's own range -- standing in for a fault raised deep inside // libjvm.so while AsyncGetCallTrace dereferences a poisoned sp/pc/fp (see -// getJavaTraceAsync's java_ctx-restore and PROBE_SP-loop fault-injection -// sites). checkFault() must not recover such a fault, which means -// walkJavaStack's frame.restore() fix never runs and the mutated ucontext is -// left exactly as corrupted as the injection left it. +// getJavaTraceAsync's anchor-derived fault-injection site). checkFault() must +// not recover such a fault, which means withUcontextFaultRecovery()'s restore +// never runs and the mutated ucontext is left exactly as corrupted as the +// injection left it -- `work()` runs to completion and its return value comes +// straight back out, proving checkFault() truly fell through rather than +// recovering. TEST_F(WalkJavaStackUcontextRestoreTest, FaultOutsideProfilerRangeIsNotRecoveredAndLeavesUcontextCorrupted) { StackFrame frame(&_ctx); uintptr_t saved_pc = frame.pc(); uintptr_t saved_sp = frame.sp(); + bool truncated = false; + uintptr_t mutated_pc = 0, mutated_sp = 0, mutated_fp = 0; - sigjmp_buf crash_protection_ctx; - JmpCtxScope jmp_scope(_pt); - ASSERT_EQ(0, sigsetjmp(crash_protection_ctx, 1)) - << "must not have recovered -- this branch only runs once, forward"; - jmp_scope.install(&crash_protection_ctx); - - // Same mutation getJavaTraceAsync() performs right before handing - // sp/pc/fp to jvmAsyncGetCallTrace(). - frame.sp() += sizeof(void*); - frame.fp() = saved_sp; - frame.pc() = saved_pc + 0x1234; - - uintptr_t mutated_pc = frame.pc(); - uintptr_t mutated_sp = frame.sp(); - uintptr_t mutated_fp = frame.fp(); + int result = HotspotSupport::withUcontextFaultRecovery(&_ctx, _pt, &truncated, [&]() -> int { + // Same mutation getJavaTraceAsync() performs right before handing + // sp/pc/fp to jvmAsyncGetCallTrace(). + frame.sp() += sizeof(void*); + frame.fp() = saved_sp; + frame.pc() = saved_pc + 0x1234; - // The SIGSEGV's own delivery ucontext, standing in for a fault inside - // libjvm.so -- its pc sits 256MB past the installed range, far beyond - // kRangeMargin regardless of build config. - ucontext_t fault_uc{}; - StackFrame(&fault_uc).pc() = _range_hi + (256u * 1024 * 1024); + mutated_pc = frame.pc(); + mutated_sp = frame.sp(); + mutated_fp = frame.fp(); - siginfo_t si{}; - si.si_addr = reinterpret_cast(1); - Profiler::checkFault(_pt, &si, &fault_uc); - // Must fall through to here -- checkFault must not siglongjmp for a pc - // outside the installed range. + // The SIGSEGV's own delivery ucontext, standing in for a fault + // inside libjvm.so -- its pc sits 256MB past the installed range, + // far beyond kRangeMargin regardless of build config. + ucontext_t fault_uc{}; + StackFrame(&fault_uc).pc() = _range_hi + (256u * 1024 * 1024); - jmp_scope.restore(); + siginfo_t si{}; + si.si_addr = reinterpret_cast(1); + Profiler::checkFault(_pt, &si, &fault_uc); + // Falls through: checkFault must not recover a pc outside the range. + return 42; // sentinel proving work() ran to completion, unrecovered + }); + EXPECT_EQ(42, result) << "checkFault must not have recovered an out-of-range fault"; + EXPECT_FALSE(truncated); EXPECT_EQ(mutated_pc, frame.pc()) << "an unrecovered fault must leave the mutated ucontext untouched -- " - "walkJavaStack's frame.restore() fix is never reached in this case"; + "withUcontextFaultRecovery's restore is never reached in this case"; EXPECT_EQ(mutated_sp, frame.sp()); EXPECT_EQ(mutated_fp, frame.fp()); + EXPECT_FALSE(_pt->isProtected()); } -// walkJavaStack guards the restore with `if (ucontext != NULL)`, since -// ucontext can legitimately be null (e.g. malloc/socket hooks sampled outside -// any signal context). The recovery branch must not dereference a null -// StackFrame in that case. +// withUcontextFaultRecovery() must not dereference a null ucontext in its +// recovery branch, since ucontext can legitimately be null (e.g. malloc/ +// socket hooks sampled outside any signal context). TEST_F(WalkJavaStackUcontextRestoreTest, NullUcontextSkipsRestoreWithoutCrashing) { - void* ucontext = nullptr; - StackFrame frame(ucontext); - uintptr_t saved_pc = 0, saved_sp = 0, saved_fp = 0; - - sigjmp_buf crash_protection_ctx; - JmpCtxScope jmp_scope(_pt); - int recovered = 0; - - if (sigsetjmp(crash_protection_ctx, 1) != 0) { - recovered++; - jmp_scope.restore(); - if (ucontext != nullptr) { - frame.restore(saved_pc, saved_sp, saved_fp); - } - } else { - jmp_scope.install(&crash_protection_ctx); - siglongjmp(*_pt->getJmpCtx(), 1); - FAIL() << "unreachable: siglongjmp does not return"; - } + bool truncated = false; + + int result = HotspotSupport::withUcontextFaultRecovery(nullptr, _pt, &truncated, [&]() -> int { + // A fault whose pc is inside the installed range, same as the + // "recovers" test above, but with a null ucontext -- the recovery + // branch's ctx_snapshot.restore() must be a safe no-op here rather + // than dereferencing a null StackFrame. + ucontext_t fault_uc{}; + StackFrame(&fault_uc).pc() = _range_lo + kRangeMargin; + + siginfo_t si{}; + si.si_addr = reinterpret_cast(1); + // See the matching comment in FaultInsideProfilerRangeRecoversAndRestoresUcontext. + _pt->enterSignalScope(); + Profiler::checkFault(_pt, &si, &fault_uc); + ADD_FAILURE() << "unreachable: checkFault() must siglongjmp for an in-range pc"; + return -1; + }); - EXPECT_EQ(1, recovered); + EXPECT_EQ(0, result); + EXPECT_TRUE(truncated); EXPECT_FALSE(_pt->isProtected()); } From 46443bfc0e063ed2783bf4e5f143a079d056bc74 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Fri, 4 Sep 2026 18:28:57 +0000 Subject: [PATCH 6/6] Fix --- ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp | 5 ++--- ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h | 12 ++++++++++-- .../src/test/cpp/hotspot_crash_protection_ut.cpp | 11 +++++++++++ 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 1f7d9ea68..08850308f 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -1148,7 +1148,6 @@ int HotspotSupport::getJavaTraceAsync(void *ucontext, ASGCT_CallFrame *frames, trace.frames--; } for (int i = 0; trace.num_frames < 0 && i < PROBE_SP_LIMIT; i++) { - // PROBE_SP walks past the real frame boundary by design; frame.sp() = frame.sp() + sizeof(void*); JVMSupport::jvmAsyncGetCallTrace(&trace, max_depth, ucontext); } @@ -1270,8 +1269,8 @@ int HotspotSupport::walkJavaStack(StackWalkRequest& request) { // AsyncGetCallTrace call) is caught by Profiler::checkFault() and // recovered instead of crashing the process -- see its own comment for why // it also restores the ucontext. + volatile int java_frames = 0; return withUcontextFaultRecovery(ucontext, prof_thread, truncated, [&]() -> int { - int java_frames = 0; if (features.mixed) { java_frames = walkVM(ucontext, frames, max_depth, features, eventTypeFromBCI(request.event_type), lock_index, truncated); } else if (isHookPrefixedSample(request.event_type)) { @@ -1324,7 +1323,7 @@ int HotspotSupport::walkJavaStack(StackWalkRequest& request) { } } return java_frames; - }); + }, &java_frames); } static void patchClassLoaderData(JNIEnv* jni, jclass klass) { diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h index 8ac5067c7..e7f66c3f0 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h @@ -52,6 +52,14 @@ class HotspotSupport { // ucontext is the exact one the kernel uses to resume the sampled // thread when the signal handler returns. // + // `partial_result`, if non-null, is the caller's own accumulator for + // whatever `work` has already committed to the output buffer (e.g. + // walkJavaStack's java_frames): getJavaTraceAsync() can fault *after* + // already returning a valid frame count and filling `frames` (e.g. inside + // fillFrameTypes()/isCarryingVirtualThread()'s follow-up work), and that + // partial progress must come back as a truncated-but-valid count rather + // than being discarded as zero frames. + // // Extracted into one place, rather than hand-rolled separately in // walkJavaStack(), so production and its regression test invoke the // identical recovery branch -- see hotspot_crash_protection_ut.cpp's @@ -59,7 +67,7 @@ class HotspotSupport { // std::function so the hot sample path pays no allocation for // captures. template - static int withUcontextFaultRecovery(void* ucontext, ProfiledThread* prof_thread, bool* truncated, Fn&& work) { + static int withUcontextFaultRecovery(void* ucontext, ProfiledThread* prof_thread, bool* truncated, Fn&& work, volatile int* partial_result = nullptr) { const bool prev_unwinding_java = prof_thread->is_unwinding_Java(); StackFrame::RegisterSnapshot ctx_snapshot(ucontext); @@ -78,7 +86,7 @@ class HotspotSupport { if (truncated) { *truncated = true; } - return 0; + return partial_result ? *partial_result : 0; } jmp_scope.install(&crash_protection_ctx); return work(); diff --git a/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp b/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp index 43c9f7231..765bc33c7 100644 --- a/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp +++ b/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp @@ -645,6 +645,17 @@ class WalkJavaStackUcontextRestoreTest : public ::testing::Test { // arbitrary values through StackFrame::pc()/sp()/fp() (references // into uc_mcontext), so a real, live context is unnecessary here. _ctx = ucontext_t{}; + + // Seed with distinct non-zero values so the restore assertions below + // are actually exercised. A zeroed ucontext makes fp's mutation + // (`frame.fp() = saved_sp`, mirroring getJavaTraceAsync) a no-op -- + // saved_sp is 0, same as fp's own untouched value -- so a broken + // restore()/no-restore-at-all would pass EXPECT_EQ(saved_fp, + // frame.fp()) by coincidence rather than by actually restoring. + StackFrame seed(&_ctx); + seed.pc() = 0xAAAA1000; + seed.sp() = 0xBBBB2000; + seed.fp() = 0xCCCC3000; } void TearDown() override {