From ca9643fbd4b216b8ac465b816271dc0b7106531e Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 12 Aug 2026 12:50:42 +0000 Subject: [PATCH 01/19] v0 --- ddprof-lib/src/main/cpp/common.h | 3 ++ ddprof-lib/src/main/cpp/counters.h | 1 + ddprof-lib/src/main/cpp/faultInjection.cpp | 7 +++ ddprof-lib/src/main/cpp/faultInjection.h | 56 +++++++++++++++++++++- ddprof-lib/src/main/cpp/flightRecorder.cpp | 32 ++++++++++++- ddprof-lib/src/main/cpp/guards.cpp | 5 ++ ddprof-lib/src/main/cpp/guards.h | 3 ++ ddprof-lib/src/main/cpp/threadLocalData.h | 11 +++-- 8 files changed, 111 insertions(+), 7 deletions(-) diff --git a/ddprof-lib/src/main/cpp/common.h b/ddprof-lib/src/main/cpp/common.h index 13b8ae9cae..6998da0ad6 100644 --- a/ddprof-lib/src/main/cpp/common.h +++ b/ddprof-lib/src/main/cpp/common.h @@ -37,11 +37,14 @@ constexpr size_t KNUTH_MULTIPLICATIVE_CONSTANT = 0x9e3779b97f4a7c15ULL; #ifdef DEBUG +#define debug_only(s) s + #define TEST_LOG(fmt, ...) do { \ fprintf(stdout, "[TEST::INFO] " fmt "\n", ##__VA_ARGS__); \ fflush(stdout); \ } while (0) #else +#define debug_only(s) #define TEST_LOG(fmt, ...) // No-op in non-debug mode #endif diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index a3b3ea34f7..82ee7ef732 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/ddprof-lib/src/main/cpp/counters.h @@ -134,6 +134,7 @@ X(SAFECOPY_FAILED, "safecopy_failed") \ X(SAFEFETCH_FAILED, "safefetch_failed") \ X(STACKWALK_LONGJMP_RECOVERED, "stackwalk_longjmp_recovered") \ + X(METHOD_RESOLUTION_FAILED, "method_resolution_failed") \ /* writeElement() guards against a corrupted/dangling JfrMetadata tree. \ * Root cause is still unconfirmed, so these counters are the durable \ * signal for spotting a recurrence. */ \ diff --git a/ddprof-lib/src/main/cpp/faultInjection.cpp b/ddprof-lib/src/main/cpp/faultInjection.cpp index d61be77b68..41613a3b64 100644 --- a/ddprof-lib/src/main/cpp/faultInjection.cpp +++ b/ddprof-lib/src/main/cpp/faultInjection.cpp @@ -101,6 +101,13 @@ bool shouldFire(u64 threshold, const char* fn) { return false; } +void crashNow() { + volatile uintptr_t* p = (volatile uintptr_t*)poisonAddress(); + *p = 0xBAD; + __builtin_unreachable(); // PROT_NONE guard page: the store above never returns. +} + + uintptr_t poisonAddress() { u64 r = nextRandom(); if (g_guard_ok.load(std::memory_order_acquire)) { diff --git a/ddprof-lib/src/main/cpp/faultInjection.h b/ddprof-lib/src/main/cpp/faultInjection.h index 5ac8ead3ba..f543b0c7ba 100644 --- a/ddprof-lib/src/main/cpp/faultInjection.h +++ b/ddprof-lib/src/main/cpp/faultInjection.h @@ -34,8 +34,18 @@ // // return INJECT_FAULT_BOOL_LIKELY(dlopen(name, flags) != nullptr); // -// The three tiers name their firing frequency: RARE 0.01%, UNLIKELY 0.1%, -// LIKELY 1%. See faultInjection.cpp for the poison-address and PRNG details. +// INJECT_CRASH_* has the same shape and call sites as INJECT_FAULT_ADDRESS_* +// but instead of substituting a poison address for the caller to dereference +// -- which some downstream recovery path (SafeAccess safefetch, walkVM's +// sigsetjmp/siglongjmp) may absorb -- it raises the SIGSEGV itself, right at +// the call site, so it always reaches the top-level crash handler: +// +// INJECT_CRASH_LIKELY(); +// + +// The four tiers name their firing frequency: RARE 0.01%, UNLIKELY 0.1%, +// LIKELY 1%, HIGH 10%. See faultInjection.cpp for the poison-address and PRNG +// details. #ifndef _FAULT_INJECTION_H #define _FAULT_INJECTION_H @@ -56,6 +66,7 @@ namespace faultinj { constexpr u64 PROB_RARE = 1844674407370955ULL; // 1e-4 (0.01%) constexpr u64 PROB_UNLIKELY = 18446744073709552ULL; // 1e-3 (0.1%) constexpr u64 PROB_LIKELY = 184467440737095520ULL; // 1e-2 (1%) +constexpr u64 PROB_HIGH = 1844674407370955162ULL; // 1e-1 (10%) // Called once at profiler startup (off the signal path) to mmap the PROT_NONE // guard region used by poisonAddress(). Safe to call before any injection. @@ -73,6 +84,12 @@ bool shouldFire(u64 threshold, const char* fn); // SIGSEGV). If init() failed, it falls back to a best-effort garbage address. uintptr_t poisonAddress(); +// Deliberately dereferences poisonAddress() to raise a real SIGSEGV right now, +// unconditionally (no probability gate, no shouldFire() draw). For exercising +// crash-handler / recovery paths on demand (e.g. from a test), never from a +// production code path. +[[noreturn]] void crashNow(); + // Returns ptr unchanged, or a poison address (cast to T) when the tier fires. // Templated so the wrapped expression's static type (void**, const char*, // uintptr_t, ...) is preserved exactly. @@ -86,6 +103,18 @@ inline T injectAddress(T ptr, u64 threshold, const char* fn) { return ptr; } +// Like injectAddress(), but instead of substituting a poison pointer into the +// expression (leaving recovery to whatever the caller does with it downstream +// -- SafeAccess safefetch, walkVM's sigsetjmp/siglongjmp), this crashes right +// here, right now, when the tier fires. For exercising the top-level crash +// handler itself rather than a specific recovery path. Returns ptr unchanged +// otherwise, so it's a drop-in replacement at any INJECT_FAULT_ADDRESS_* site. +inline void injectCrash(u64 threshold, const char* fn) { + if (__builtin_expect(shouldFire(threshold, fn), 0)) { + crashNow(); + } +} + // Returns orig unchanged, or `faulty` when the tier fires. Unlike // injectAddress() (which fakes an input about to be dereferenced), this fakes // the *outcome* of a call that already ran for real — e.g. making a @@ -106,6 +135,8 @@ inline T injectValue(T orig, T faulty, u64 threshold, const char* fn) { ::faultinj::injectAddress((ptr), ::faultinj::PROB_UNLIKELY, __func__) #define INJECT_FAULT_ADDRESS_LIKELY(ptr) \ ::faultinj::injectAddress((ptr), ::faultinj::PROB_LIKELY, __func__) +#define INJECT_FAULT_ADDRESS_HIGH(ptr) \ + ::faultinj::injectAddress((ptr), ::faultinj::PROB_HIGH, __func__) #define INJECT_FAULT_BOOL_RARE(v) \ ::faultinj::injectValue((v), false, ::faultinj::PROB_RARE, __func__) @@ -113,12 +144,26 @@ inline T injectValue(T orig, T faulty, u64 threshold, const char* fn) { ::faultinj::injectValue((v), false, ::faultinj::PROB_UNLIKELY, __func__) #define INJECT_FAULT_BOOL_LIKELY(v) \ ::faultinj::injectValue((v), false, ::faultinj::PROB_LIKELY, __func__) +#define INJECT_FAULT_BOOL_HIGH(v) \ + ::faultinj::injectValue((v), false, ::faultinj::PROB_HIGH, __func__) + + #define INJECT_CRASH_RARE() \ + ::faultinj::injectCrash(::faultinj::PROB_RARE, __func__) +#define INJECT_CRASH_UNLIKELY() \ + ::faultinj::injectCrash(::faultinj::PROB_UNLIKELY, __func__) +#define INJECT_CRASH_LIKELY() \ + ::faultinj::injectCrash(::faultinj::PROB_LIKELY, __func__) +#define INJECT_CRASH_HIGH() \ + ::faultinj::injectCrash(::faultinj::PROB_HIGH, __func__) +#define INJECT_CRASH_ALWAYS() crashNow() + #else // __FAULT_INJECTION__ not defined — strict identity, zero cost. #define INJECT_FAULT_ADDRESS_RARE(ptr) (ptr) #define INJECT_FAULT_ADDRESS_UNLIKELY(ptr) (ptr) #define INJECT_FAULT_ADDRESS_LIKELY(ptr) (ptr) +#define INJECT_FAULT_ADDRESS_HIGH(ptr) (ptr) #define INJECT_FAULT_INT_RARE(v) (v) #define INJECT_FAULT_INT_UNLIKELY(v) (v) @@ -131,6 +176,13 @@ inline T injectValue(T orig, T faulty, u64 threshold, const char* fn) { #define INJECT_FAULT_BOOL_RARE(v) (v) #define INJECT_FAULT_BOOL_UNLIKELY(v) (v) #define INJECT_FAULT_BOOL_LIKELY(v) (v) +#define INJECT_FAULT_BOOL_HIGH(v) (v) + +#define INJECT_CRASH_RARE() +#define INJECT_CRASH_UNLIKELY() +#define INJECT_CRASH_LIKELY() +#define INJECT_CRASH_HIGH() +#define INJECT_CRASH_ALWAYS() #define NO_INJECTION_ASSERT(a) (assert(a)) diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp index 4432fdc692..35d1c9a85c 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.cpp +++ b/ddprof-lib/src/main/cpp/flightRecorder.cpp @@ -14,6 +14,7 @@ #include "counters.h" #include "nativeMem.h" #include "dictionary.h" +#include "faultInjection.h" #include "flightRecorder.inline.h" #include "incbin.h" #include "jfrMetadata.h" @@ -568,11 +569,39 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { method_id = nullptr; } + // Setup siglongjmp protection + // This is outside of a signal handler, there is no reason for allocation to fail, + // other than OOM + ProfiledThread* prof_thread = ProfiledThread::initCurrentThreadSignalSafe(); + MethodInfo* mi = nullptr; + assert(prof_thread != nullptr); + sigjmp_buf crash_protection_ctx; + sigjmp_buf* prev_buf = prof_thread->getJmpCtx(); + if (sigsetjmp(crash_protection_ctx, 1) != 0) { + SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); + prof_thread->setJmpCtx(prev_buf); + key = MethodMap::makeKey(UNKNOWN); + Counters::increment(METHOD_RESOLUTION_FAILED); + mi = &(*_method_map)[key]; + if (!mi->_mark) { + mi->_mark = true; + if (mi->_key == 0) { + mi->_key = _method_map->allocId(); + } + fillNativeMethodInfo(mi, UNKNOWN, nullptr); + } + return mi; + } + prof_thread->setJmpCtx(&crash_protection_ctx); + // Resolve native method if (FrameType::isRawPointer(bci)) { method_id = JVMSupport::resolve(frame.method); } + // Inject fault to test siglongjmp protection + INJECT_CRASH_LIKELY(); + // BCI_VTABLE_RECEIVER: method holds a VMSymbol* (see vmEntry.h). Resolve // to a class_id via the per-dump cache once, then key MethodMap by the // resolved class_id so two distinct Symbol addresses for the same class @@ -599,7 +628,7 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { key = MethodMap::makeKey(method_id); } - MethodInfo *mi = &(*_method_map)[key]; + mi = &(*_method_map)[key]; if (!mi->_mark) { mi->_mark = true; @@ -2191,6 +2220,7 @@ void FlightRecorder::stop() { } Error FlightRecorder::dump(const char *filename, const int length) { + TEST_LOG("Dump jfr to file: %s", filename); DEBUG_ASSERT_NOT_IN_SIGNAL(); assert(length >= 0); ExclusiveLockGuard locker(&_rec_lock); diff --git a/ddprof-lib/src/main/cpp/guards.cpp b/ddprof-lib/src/main/cpp/guards.cpp index 9905182e9a..23195c369e 100644 --- a/ddprof-lib/src/main/cpp/guards.cpp +++ b/ddprof-lib/src/main/cpp/guards.cpp @@ -14,6 +14,8 @@ * limitations under the License. */ +#include + #include "guards.h" #include "common.h" #include "os.h" @@ -39,6 +41,7 @@ bool isInTrackedSignalContext() { SignalHandlerScope::SignalHandlerScope() : _active(true) { ProfiledThread *pt = ProfiledThread::current(); if (pt != nullptr) { + debug_only(_signal_depth = pt->signalDepth();) pt->enterSignalScope(); } else { // No thread context: nothing to update; mark inactive so destructor @@ -52,6 +55,7 @@ SignalHandlerScope::~SignalHandlerScope() { ProfiledThread *pt = ProfiledThread::current(); if (pt != nullptr) { pt->exitSignalScope(); + assert(_signal_depth == pt->signalDepth()); } } @@ -60,6 +64,7 @@ void SignalHandlerScope::release() { ProfiledThread *pt = ProfiledThread::current(); if (pt != nullptr) { pt->exitSignalScope(); + assert(_signal_depth == pt->signalDepth()); } _active = false; } diff --git a/ddprof-lib/src/main/cpp/guards.h b/ddprof-lib/src/main/cpp/guards.h index 18bc4fbeda..4a8d0faf6f 100644 --- a/ddprof-lib/src/main/cpp/guards.h +++ b/ddprof-lib/src/main/cpp/guards.h @@ -22,6 +22,8 @@ #include #include +#include "common.h" + class ProfiledThread; // --------------------------------------------------------------------------- @@ -82,6 +84,7 @@ class SignalHandlerScope { SignalHandlerScope& operator=(const SignalHandlerScope&) = delete; private: bool _active; + debug_only(int _signal_depth;) }; // Declare a scope guard local that increments the depth on entry and diff --git a/ddprof-lib/src/main/cpp/threadLocalData.h b/ddprof-lib/src/main/cpp/threadLocalData.h index 30f750cb15..8f77344352 100644 --- a/ddprof-lib/src/main/cpp/threadLocalData.h +++ b/ddprof-lib/src/main/cpp/threadLocalData.h @@ -78,7 +78,7 @@ class ProfiledThread : public ThreadLocalData { u64 _park_block_token; int _filter_slot_id; // Slot ID for thread filtering uint8_t _init_window; // Countdown for JVM thread init race window (PROF-13072) - uint8_t _signal_depth; // Nested signal-handler depth (see SignalHandlerScope) + volatile int _signal_depth; // Nested signal-handler depth (see SignalHandlerScope) UnwindFailures _unwind_failures; bool _otel_ctx_initialized; #ifdef __FAULT_INJECTION__ @@ -249,9 +249,12 @@ class ProfiledThread : public ThreadLocalData { // access happens on the owning thread (signal handlers are delivered to the // thread that's interrupted), so plain reads/writes are AS-safe — no locks, // no malloc, no syscalls. See guards.h for the public API. - inline uint8_t signalDepth() const { return _signal_depth; } - inline void enterSignalScope() { ++_signal_depth; } - inline void exitSignalScope() { if (_signal_depth > 0) --_signal_depth; } + inline uint8_t signalDepth() const { return __atomic_load_n(&_signal_depth, __ATOMIC_RELAXED); } + inline void enterSignalScope() { __atomic_fetch_add(&_signal_depth, 1, __ATOMIC_RELAXED); } + inline void exitSignalScope() { + int depth = __atomic_fetch_sub(&_signal_depth, 1, __ATOMIC_RELAXED); + assert(depth > 0); + } #ifdef __FAULT_INJECTION__ // One xorshift64 step (Marsaglia 2003), matching PoissonSampler::nextExp. From c841fab3750442a219860ec9c7f9282da43fdea6 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 12 Aug 2026 13:37:00 +0000 Subject: [PATCH 02/19] ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp --- ddprof-lib/src/main/cpp/counters.h | 1 - ddprof-lib/src/main/cpp/flightRecorder.cpp | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index 82ee7ef732..a3b3ea34f7 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/ddprof-lib/src/main/cpp/counters.h @@ -134,7 +134,6 @@ X(SAFECOPY_FAILED, "safecopy_failed") \ X(SAFEFETCH_FAILED, "safefetch_failed") \ X(STACKWALK_LONGJMP_RECOVERED, "stackwalk_longjmp_recovered") \ - X(METHOD_RESOLUTION_FAILED, "method_resolution_failed") \ /* writeElement() guards against a corrupted/dangling JfrMetadata tree. \ * Root cause is still unconfirmed, so these counters are the durable \ * signal for spotting a recurrence. */ \ diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp index 35d1c9a85c..fa423e1583 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.cpp +++ b/ddprof-lib/src/main/cpp/flightRecorder.cpp @@ -581,7 +581,9 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); prof_thread->setJmpCtx(prev_buf); key = MethodMap::makeKey(UNKNOWN); - Counters::increment(METHOD_RESOLUTION_FAILED); + // We want to have counter to record method resoluation failures. + // Unfortunately, the counter cannot be reported accurately, + // see comments in finishChunk(), just above writeCounters() call. mi = &(*_method_map)[key]; if (!mi->_mark) { mi->_mark = true; From 91878285750b6aaebfffc4c1cd0f4939f4652df4 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 12 Aug 2026 13:37:45 +0000 Subject: [PATCH 03/19] Add missing test --- .../cpp/resolveMethodFaultInjection_ut.cpp | 137 ++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp diff --git a/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp b/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp new file mode 100644 index 0000000000..b4b92ba36e --- /dev/null +++ b/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp @@ -0,0 +1,137 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include + +#include "../../main/cpp/flightRecorder.h" +#include "../../main/cpp/counters.h" +#include "../../main/cpp/faultInjection.h" +#include "../../main/cpp/guards.h" +#include "../../main/cpp/os.h" +#include "../../main/cpp/profiler.h" +#include "../../main/cpp/safeAccess.h" +#include "../../main/cpp/threadLocalData.h" +#include "../../main/cpp/gtest_crash_handler.h" + +// Only meaningful in a fault-injection build (-PenableFaultInjection): that is +// the only configuration where INJECT_CRASH_LIKELY() in +// Lookup::resolveMethod() (see flightRecorder.cpp) expands to anything other +// than a no-op. +#ifdef __FAULT_INJECTION__ + +static constexpr char RESOLVE_METHOD_FI_TEST_NAME[] = "ResolveMethodFaultInjectionTest"; + +// Mirrors the real production signal chain (see Profiler::segvHandler): +// safefetch recovery first, then Profiler::checkFault() -- which, since +// resolveMethod() installs its own sigsetjmp jmp ctx on the current +// ProfiledThread, siglongjmp's straight back into resolveMethod()'s recovery +// branch -- falling back to the previous handler / gtest's crash handler for +// a fault this test did not expect. +static void (*orig_segv)(int, siginfo_t*, void*); +static void (*orig_bus)(int, siginfo_t*, void*); + +static void resolveMethodFiHandler(int signo, siginfo_t* siginfo, void* context) { + // Every installed signal handler in production opens a SIGNAL_HANDLER_GUARD() + // scope (see Profiler::segvHandler/busHandler). resolveMethod()'s recovery + // branch calls SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP() to compensate for that + // scope's destructor being skipped by the siglongjmp out of this handler -- + // without opening the scope here first, that compensation underflows + // ProfiledThread::_signal_depth and trips its debug assert. + SIGNAL_HANDLER_GUARD(); + if (SafeAccess::handle_safefetch(signo, context)) { + return; + } + Profiler::checkFault(ProfiledThread::current(), siginfo, context); // siglongjmp if protected + if (signo == SIGBUS && orig_bus != nullptr) { + orig_bus(signo, siginfo, context); + } else if (signo == SIGSEGV && orig_segv != nullptr) { + orig_segv(signo, siginfo, context); + } else { + gtestCrashHandler(signo, siginfo, context, RESOLVE_METHOD_FI_TEST_NAME); + } +} + +class ResolveMethodFaultInjectionTest : public ::testing::Test { +protected: + void SetUp() override { + ProfiledThread::initCurrentThread(); + faultinj::init(); + orig_segv = OS::replaceSigsegvHandler(resolveMethodFiHandler); + orig_bus = OS::replaceSigbusHandler(resolveMethodFiHandler); + } + + void TearDown() override { + OS::replaceSigsegvHandler(orig_segv); + OS::replaceSigbusHandler(orig_bus); + ProfiledThread::release(); + } +}; + +// resolveMethod() wraps its body in a sigsetjmp/siglongjmp jmp ctx specifically +// so that INJECT_CRASH_LIKELY() (a real SIGSEGV, not a poisoned pointer left +// for the caller to dereference) is recoverable: when it fires, control must +// land back at the sigsetjmp, producing the same "unknown" MethodInfo that a +// genuine resolution failure would, and the process must not crash. +// +// The frame here has a NULL method_id and bci == 0 (not a raw-pointer bci), +// so both the non-injected (~99%) and injected-and-recovered (~1%) paths +// converge on the exact same MethodMap key/fill (see flightRecorder.cpp: +// `if (method_id == nullptr) fillNativeMethodInfo(mi, UNKNOWN, nullptr);`), +// which keeps the assertions below valid regardless of which path any given +// call took. +// +// Note: on the non-recovering (success) path resolveMethod() deliberately +// leaves the ProfiledThread's jmp ctx pointing at its own (now-popped) stack +// frame rather than restoring the caller's prior context -- each call +// re-installs a fresh one via sigsetjmp before doing anything risky, so this +// is safe, but it does mean ProfiledThread::isProtected() cannot be used +// here to distinguish a recovered call from a normal one. +TEST_F(ResolveMethodFaultInjectionTest, RecoversFromInjectedCrashInsteadOfCrashing) { + ProfiledThread* t = ProfiledThread::current(); + ASSERT_NE(t, nullptr); + t->setFiRng(0xD15EA5EDD15EA5EDULL); + + StringDictionary classes; + MethodMap methods; + Lookup lookup(nullptr, &methods, &classes); + + ASGCT_CallFrame frame{}; + frame.bci = 0; + frame.method_id = nullptr; + + long long faultsBefore = Counters::getCounter(FAULTS_INJECTED); + bool sawRecoveredInjection = false; + + // LIKELY tier fires ~1% of the time; 5000 tries makes seeing at least one + // recovery astronomically likely without hardcoding an exact iteration. + for (int i = 0; i < 5000; i++) { + long long recoveredBefore = Counters::getCounter(STACKWALK_LONGJMP_RECOVERED); + + MethodInfo* info = lookup.resolveMethod(frame); + + ASSERT_NE(info, nullptr); + EXPECT_EQ(info->_type, FRAME_NATIVE); + + if (Counters::getCounter(STACKWALK_LONGJMP_RECOVERED) > recoveredBefore) { + sawRecoveredInjection = true; + break; + } + } + + EXPECT_TRUE(sawRecoveredInjection) + << "expected INJECT_CRASH_LIKELY() in Lookup::resolveMethod() to fire " + "and be recovered via siglongjmp within 5000 calls"; + EXPECT_GT(Counters::getCounter(FAULTS_INJECTED), faultsBefore); + + // Every call -- injected-and-recovered or not -- resolves the same NULL + // method_id frame to the single shared "unknown" MethodInfo row. + EXPECT_EQ(methods.size(), 1U); + + // Defensive cleanup: leaving the stale post-call jmp ctx (see note above) + // live into TearDown()/ProfiledThread::release() serves no purpose here. + t->setJmpCtx(nullptr); +} + +#endif // __FAULT_INJECTION__ From e8e1ae2006a105db3651ad6a3febcbcd6d2e0362 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 12 Aug 2026 16:41:03 +0000 Subject: [PATCH 04/19] Cleanup and fix tests --- ddprof-lib/src/main/cpp/common.h | 4 +-- ddprof-lib/src/main/cpp/faultInjection.cpp | 16 ++++++------ ddprof-lib/src/main/cpp/faultInjection.h | 12 ++++----- ddprof-lib/src/main/cpp/flightRecorder.cpp | 6 +++-- ddprof-lib/src/main/cpp/guards.cpp | 6 ++--- ddprof-lib/src/main/cpp/guards.h | 2 +- ddprof-lib/src/main/cpp/profiler.h | 4 +-- ddprof-lib/src/main/cpp/threadLocalData.h | 13 +++++++--- .../cpp/resolveMethodFaultInjection_ut.cpp | 25 +++++++++++++++++++ ddprof-lib/src/test/cpp/signalSafety_ut.cpp | 20 ++++++++++----- 10 files changed, 75 insertions(+), 33 deletions(-) diff --git a/ddprof-lib/src/main/cpp/common.h b/ddprof-lib/src/main/cpp/common.h index 6998da0ad6..1aac256d46 100644 --- a/ddprof-lib/src/main/cpp/common.h +++ b/ddprof-lib/src/main/cpp/common.h @@ -37,14 +37,14 @@ constexpr size_t KNUTH_MULTIPLICATIVE_CONSTANT = 0x9e3779b97f4a7c15ULL; #ifdef DEBUG -#define debug_only(s) s +#define DEBUG_ONLY(s) s #define TEST_LOG(fmt, ...) do { \ fprintf(stdout, "[TEST::INFO] " fmt "\n", ##__VA_ARGS__); \ fflush(stdout); \ } while (0) #else -#define debug_only(s) +#define DEBUG_ONLY(s) #define TEST_LOG(fmt, ...) // No-op in non-debug mode #endif diff --git a/ddprof-lib/src/main/cpp/faultInjection.cpp b/ddprof-lib/src/main/cpp/faultInjection.cpp index 41613a3b64..27c6e2a4ea 100644 --- a/ddprof-lib/src/main/cpp/faultInjection.cpp +++ b/ddprof-lib/src/main/cpp/faultInjection.cpp @@ -16,6 +16,15 @@ #include "faultInjection.h" +#include + +void crashNow() { + volatile uintptr_t* p = (volatile uintptr_t*)nullptr; + *p = 0xBAD; + __builtin_unreachable(); // the store above never returns. +} + + // The whole translation unit is empty unless fault injection is enabled, so a // normal build links a no-op object file. #ifdef __FAULT_INJECTION__ @@ -101,13 +110,6 @@ bool shouldFire(u64 threshold, const char* fn) { return false; } -void crashNow() { - volatile uintptr_t* p = (volatile uintptr_t*)poisonAddress(); - *p = 0xBAD; - __builtin_unreachable(); // PROT_NONE guard page: the store above never returns. -} - - uintptr_t poisonAddress() { u64 r = nextRandom(); if (g_guard_ok.load(std::memory_order_acquire)) { diff --git a/ddprof-lib/src/main/cpp/faultInjection.h b/ddprof-lib/src/main/cpp/faultInjection.h index f543b0c7ba..fdd9817a02 100644 --- a/ddprof-lib/src/main/cpp/faultInjection.h +++ b/ddprof-lib/src/main/cpp/faultInjection.h @@ -52,6 +52,12 @@ #include +// Deliberately dereferences nullptr to raise a real SIGSEGV right now, +// unconditionally (no probability gate, no shouldFire() draw). For exercising +// crash-handler / recovery paths on demand (e.g. from a test), never from a +// production code path. +[[noreturn]] void crashNow(); + #ifdef __FAULT_INJECTION__ #include "arch.h" // u64 @@ -84,12 +90,6 @@ bool shouldFire(u64 threshold, const char* fn); // SIGSEGV). If init() failed, it falls back to a best-effort garbage address. uintptr_t poisonAddress(); -// Deliberately dereferences poisonAddress() to raise a real SIGSEGV right now, -// unconditionally (no probability gate, no shouldFire() draw). For exercising -// crash-handler / recovery paths on demand (e.g. from a test), never from a -// production code path. -[[noreturn]] void crashNow(); - // Returns ptr unchanged, or a poison address (cast to T) when the tier fires. // Templated so the wrapped expression's static type (void**, const char*, // uintptr_t, ...) is preserved exactly. diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp index fa423e1583..2b986a32a5 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.cpp +++ b/ddprof-lib/src/main/cpp/flightRecorder.cpp @@ -573,8 +573,10 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { // This is outside of a signal handler, there is no reason for allocation to fail, // other than OOM ProfiledThread* prof_thread = ProfiledThread::initCurrentThreadSignalSafe(); + if (prof_thread == nullptr) { + return nullptr; + } MethodInfo* mi = nullptr; - assert(prof_thread != nullptr); sigjmp_buf crash_protection_ctx; sigjmp_buf* prev_buf = prof_thread->getJmpCtx(); if (sigsetjmp(crash_protection_ctx, 1) != 0) { @@ -699,7 +701,7 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { fillJavaMethodInfo(mi, method_id, first_time); } } - + prof_thread->setJmpCtx(prev_buf); return mi; } diff --git a/ddprof-lib/src/main/cpp/guards.cpp b/ddprof-lib/src/main/cpp/guards.cpp index 23195c369e..56e46a8cba 100644 --- a/ddprof-lib/src/main/cpp/guards.cpp +++ b/ddprof-lib/src/main/cpp/guards.cpp @@ -41,7 +41,7 @@ bool isInTrackedSignalContext() { SignalHandlerScope::SignalHandlerScope() : _active(true) { ProfiledThread *pt = ProfiledThread::current(); if (pt != nullptr) { - debug_only(_signal_depth = pt->signalDepth();) + DEBUG_ONLY(_signal_depth = pt->signalDepth();) pt->enterSignalScope(); } else { // No thread context: nothing to update; mark inactive so destructor @@ -55,7 +55,7 @@ SignalHandlerScope::~SignalHandlerScope() { ProfiledThread *pt = ProfiledThread::current(); if (pt != nullptr) { pt->exitSignalScope(); - assert(_signal_depth == pt->signalDepth()); + DEBUG_ONLY(assert(_signal_depth == pt->signalDepth());) } } @@ -64,7 +64,7 @@ void SignalHandlerScope::release() { ProfiledThread *pt = ProfiledThread::current(); if (pt != nullptr) { pt->exitSignalScope(); - assert(_signal_depth == pt->signalDepth()); + DEBUG_ONLY(assert(_signal_depth == pt->signalDepth());) } _active = false; } diff --git a/ddprof-lib/src/main/cpp/guards.h b/ddprof-lib/src/main/cpp/guards.h index 4a8d0faf6f..f3ead4e259 100644 --- a/ddprof-lib/src/main/cpp/guards.h +++ b/ddprof-lib/src/main/cpp/guards.h @@ -84,7 +84,7 @@ class SignalHandlerScope { SignalHandlerScope& operator=(const SignalHandlerScope&) = delete; private: bool _active; - debug_only(int _signal_depth;) + DEBUG_ONLY(int _signal_depth;) }; // Declare a scope guard local that increments the depth on entry and diff --git a/ddprof-lib/src/main/cpp/profiler.h b/ddprof-lib/src/main/cpp/profiler.h index a9563bd0a9..84c1c4d398 100644 --- a/ddprof-lib/src/main/cpp/profiler.h +++ b/ddprof-lib/src/main/cpp/profiler.h @@ -16,6 +16,7 @@ #include "stringDictionary.h" #include "engine.h" #include "event.h" +#include "faultInjection.h" #include "flightRecorder.h" #include "guards.h" #include "libraries.h" @@ -519,8 +520,7 @@ class alignas(alignof(SpinLock)) Profiler { // this is a safe place to do it since this wrapper is used solely from the 'vm' stackwalker implementation if (force_stackwalk_crash_env) { TEST_LOG("FORCE_SIGSEGV"); - int* p = nullptr; - *p = 1; + crashNow(); } #endif return Libraries::instance()->findLibraryByAddress(address); diff --git a/ddprof-lib/src/main/cpp/threadLocalData.h b/ddprof-lib/src/main/cpp/threadLocalData.h index 8f77344352..645436f482 100644 --- a/ddprof-lib/src/main/cpp/threadLocalData.h +++ b/ddprof-lib/src/main/cpp/threadLocalData.h @@ -245,12 +245,17 @@ class ProfiledThread : public ThreadLocalData { return _jmp_buf != nullptr; } - // Signal-handler depth counter used by SignalHandlerScope (guards.h). All - // access happens on the owning thread (signal handlers are delivered to the - // thread that's interrupted), so plain reads/writes are AS-safe — no locks, - // no malloc, no syscalls. See guards.h for the public API. + // Signal-handler depth counter used by SignalHandlerScope (guards.h). + // But read-modify-store can be interrupted by other signals, so it has to be an atomic counter. inline uint8_t signalDepth() const { return __atomic_load_n(&_signal_depth, __ATOMIC_RELAXED); } inline void enterSignalScope() { __atomic_fetch_add(&_signal_depth, 1, __ATOMIC_RELAXED); } + // Every real exitSignalScope() call is paired with a prior enterSignalScope(): + // either the normal SignalHandlerScope destructor, or exactly one compensating + // SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP() per skipped destructor (Profiler::checkFault() + // -- the only thing that ever siglongjmp's past a SignalHandlerScope -- is only + // reachable from segvHandler()/busHandler(), both of which open SIGNAL_HANDLER_GUARD() + // first). A call here with depth already 0 means that pairing was broken elsewhere; + // that is a real bug and must fail loudly, not be silently tolerated. inline void exitSignalScope() { int depth = __atomic_fetch_sub(&_signal_depth, 1, __ATOMIC_RELAXED); assert(depth > 0); diff --git a/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp b/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp index b4b92ba36e..5820f50be5 100644 --- a/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp +++ b/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp @@ -134,4 +134,29 @@ TEST_F(ResolveMethodFaultInjectionTest, RecoversFromInjectedCrashInsteadOfCrashi t->setJmpCtx(nullptr); } +#else // __FAULT_INJECTION__ not defined (the default release/debug build). + +// INJECT_CRASH_LIKELY() in resolveMethod() compiles to nothing here (see +// faultInjection.h), so there is nothing to inject -- this is a plain smoke +// test of the same call, kept for two reasons: (1) it documents that the +// call site is inert in this configuration, and (2) a translation unit that +// registers zero gtest tests fails to *link* as its own binary: with no +// TEST/TEST_F in this object file, nothing here pulls a member out of +// -lgtest before -lgtest_main's gtest_main.cc.o (which needs +// testing::InitGoogleTest() etc. from that same archive) is processed, and +// -lgtest is never revisited afterwards. +TEST(ResolveMethodFaultInjectionTest, DisabledBuildResolvesNormally) { + StringDictionary classes; + MethodMap methods; + Lookup lookup(nullptr, &methods, &classes); + + ASGCT_CallFrame frame{}; + frame.bci = 0; + frame.method_id = nullptr; + + MethodInfo* info = lookup.resolveMethod(frame); + ASSERT_NE(info, nullptr); + EXPECT_EQ(info->_type, FRAME_NATIVE); +} + #endif // __FAULT_INJECTION__ diff --git a/ddprof-lib/src/test/cpp/signalSafety_ut.cpp b/ddprof-lib/src/test/cpp/signalSafety_ut.cpp index 4c55a22f35..8b3330650b 100644 --- a/ddprof-lib/src/test/cpp/signalSafety_ut.cpp +++ b/ddprof-lib/src/test/cpp/signalSafety_ut.cpp @@ -120,13 +120,21 @@ TEST_F(SignalSafetyTest, SignalHandlerUnwindAfterLongjmpDecrementsOnce) { EXPECT_EQ(0, getInSignalDepth()); } -// Safety property: signalHandlerUnwindAfterLongjmp() saturates at zero; -// double calls do not underflow. -TEST_F(SignalSafetyTest, SignalHandlerUnwindAfterLongjmpSaturatesAtZero) { - EXPECT_EQ(0, getInSignalDepth()); - signalHandlerUnwindAfterLongjmp(); - signalHandlerUnwindAfterLongjmp(); +// Safety property: signalHandlerUnwindAfterLongjmp() must only ever compensate +// for a SignalHandlerScope whose constructor genuinely ran. In production, +// Profiler::checkFault() is the only thing that ever siglongjmp's past a +// SignalHandlerScope's destructor, and it's only reachable from +// segvHandler()/busHandler(), both of which open a SIGNAL_HANDLER_GUARD() +// first -- so every real compensating call is paired with a prior +// enterSignalScope(). Calling it here with no matching scope at all (depth +// already 0) is exactly the kind of pairing bug that must never be +// tolerated silently: it has to abort loudly instead of underflowing the +// counter (which signalDepth() truncates to uint8_t, so an underflow would +// silently wrap to a large positive value and corrupt +// isInTrackedSignalContext() for the rest of the thread's life). +TEST_F(SignalSafetyTest, SignalHandlerUnwindAfterLongjmpAbortsOnUnmatchedCall) { EXPECT_EQ(0, getInSignalDepth()); + EXPECT_DEATH({ signalHandlerUnwindAfterLongjmp(); }, "Assertion .*"); } TEST(SignalSafetyTestNoContext, NullProfiledThreadIsNotTrackedSignal) { From 9858ee2d11b1da97da6d99fa8b612efbec5c0728 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 12 Aug 2026 23:52:07 +0000 Subject: [PATCH 05/19] v2 --- ddprof-lib/src/main/cpp/counters.h | 12 + ddprof-lib/src/main/cpp/faultInjection.h | 38 ++-- ddprof-lib/src/main/cpp/flightRecorder.cpp | 205 +++++++++++++----- ddprof-lib/src/main/cpp/flightRecorder.h | 51 +++++ ddprof-lib/src/main/cpp/guards.cpp | 23 +- ddprof-lib/src/main/cpp/guards.h | 51 +++++ ddprof-lib/src/main/cpp/threadLocalData.h | 16 +- .../src/test/cpp/hotspotMethodId_ut.cpp | 7 +- .../cpp/resolveMethodFaultInjection_ut.cpp | 114 ++++++---- ddprof-lib/src/test/cpp/signalSafety_ut.cpp | 25 ++- .../profiler/metadata/MethodIdReuseTest.java | 100 ++++++++- 11 files changed, 515 insertions(+), 127 deletions(-) diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index a3b3ea34f7..8ef0040482 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/ddprof-lib/src/main/cpp/counters.h @@ -133,7 +133,19 @@ X(SAMPLES_DROPPED_THREAD_LOCAL, "samples_dropped_thread_local") \ X(SAFECOPY_FAILED, "safecopy_failed") \ X(SAFEFETCH_FAILED, "safefetch_failed") \ + /* Every siglongjmp recovery, from any protected window, counted centrally \ + * in Profiler::checkFault(). */ \ X(STACKWALK_LONGJMP_RECOVERED, "stackwalk_longjmp_recovered") \ + /* Subset of the above: recoveries that landed in Lookup::resolveMethod(), \ + * i.e. faults while symbolicating at dump time rather than while walking a \ + * stack in a signal handler. Counted separately because the two have \ + * different root causes (stale jmethodID / class unload vs. a bad frame \ + * pointer) and would otherwise be indistinguishable. */ \ + X(METHOD_RESOLVE_LONGJMP_RECOVERED, "method_resolve_longjmp_recovered") \ + /* Lookup::resolveMethod() calls that ran without siglongjmp protection \ + * because no ProfiledThread could be allocated for the dump thread (OOM): \ + * there is nowhere to publish a landing pad. Expected to stay at 0. */ \ + X(METHOD_RESOLVE_UNPROTECTED, "method_resolve_unprotected") \ /* writeElement() guards against a corrupted/dangling JfrMetadata tree. \ * Root cause is still unconfirmed, so these counters are the durable \ * signal for spotting a recurrence. */ \ diff --git a/ddprof-lib/src/main/cpp/faultInjection.h b/ddprof-lib/src/main/cpp/faultInjection.h index fdd9817a02..e218b89622 100644 --- a/ddprof-lib/src/main/cpp/faultInjection.h +++ b/ddprof-lib/src/main/cpp/faultInjection.h @@ -34,11 +34,14 @@ // // return INJECT_FAULT_BOOL_LIKELY(dlopen(name, flags) != nullptr); // -// INJECT_CRASH_* has the same shape and call sites as INJECT_FAULT_ADDRESS_* -// but instead of substituting a poison address for the caller to dereference -// -- which some downstream recovery path (SafeAccess safefetch, walkVM's -// sigsetjmp/siglongjmp) may absorb -- it raises the SIGSEGV itself, right at -// the call site, so it always reaches the top-level crash handler: +// INJECT_CRASH_* goes at the same kind of site as INJECT_FAULT_ADDRESS_*, but it +// is a statement rather than an expression wrapper: it takes no argument and +// yields no value. Instead of substituting a poison address for the caller to +// dereference -- which a downstream recovery path (SafeAccess safefetch, a +// sigsetjmp/siglongjmp window) may absorb without a signal ever being raised -- +// it raises the SIGSEGV itself, right at the call site. Use it to exercise the +// sigsetjmp/siglongjmp window enclosing the call site, or the top-level crash +// handler where there is no such window: // // INJECT_CRASH_LIKELY(); // @@ -106,9 +109,13 @@ inline T injectAddress(T ptr, u64 threshold, const char* fn) { // Like injectAddress(), but instead of substituting a poison pointer into the // expression (leaving recovery to whatever the caller does with it downstream // -- SafeAccess safefetch, walkVM's sigsetjmp/siglongjmp), this crashes right -// here, right now, when the tier fires. For exercising the top-level crash -// handler itself rather than a specific recovery path. Returns ptr unchanged -// otherwise, so it's a drop-in replacement at any INJECT_FAULT_ADDRESS_* site. +// here, right now, when the tier fires. Whatever encloses the call site is what +// gets exercised: the nearest sigsetjmp/siglongjmp window if there is one, the +// top-level crash handler otherwise. +// +// Unlike injectAddress() this wraps no expression -- it takes no pointer and +// returns nothing, so it is a statement, not a drop-in for an +// INJECT_FAULT_ADDRESS_* site. It does nothing when the tier does not fire. inline void injectCrash(u64 threshold, const char* fn) { if (__builtin_expect(shouldFire(threshold, fn), 0)) { crashNow(); @@ -147,7 +154,7 @@ inline T injectValue(T orig, T faulty, u64 threshold, const char* fn) { #define INJECT_FAULT_BOOL_HIGH(v) \ ::faultinj::injectValue((v), false, ::faultinj::PROB_HIGH, __func__) - #define INJECT_CRASH_RARE() \ +#define INJECT_CRASH_RARE() \ ::faultinj::injectCrash(::faultinj::PROB_RARE, __func__) #define INJECT_CRASH_UNLIKELY() \ ::faultinj::injectCrash(::faultinj::PROB_UNLIKELY, __func__) @@ -178,11 +185,14 @@ inline T injectValue(T orig, T faulty, u64 threshold, const char* fn) { #define INJECT_FAULT_BOOL_LIKELY(v) (v) #define INJECT_FAULT_BOOL_HIGH(v) (v) -#define INJECT_CRASH_RARE() -#define INJECT_CRASH_UNLIKELY() -#define INJECT_CRASH_LIKELY() -#define INJECT_CRASH_HIGH() -#define INJECT_CRASH_ALWAYS() +// ((void)0) rather than nothing, so `INJECT_CRASH_LIKELY();` stays a +// well-formed expression statement in every context (e.g. as the sole body of +// an unbraced if/else) instead of collapsing to a stray semicolon. +#define INJECT_CRASH_RARE() ((void)0) +#define INJECT_CRASH_UNLIKELY() ((void)0) +#define INJECT_CRASH_LIKELY() ((void)0) +#define INJECT_CRASH_HIGH() ((void)0) +#define INJECT_CRASH_ALWAYS() ((void)0) #define NO_INJECTION_ASSERT(a) (assert(a)) diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp index 2b986a32a5..dc8ebd3836 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.cpp +++ b/ddprof-lib/src/main/cpp/flightRecorder.cpp @@ -555,9 +555,42 @@ u32 Lookup::resolveVTableReceiverCached(void *sym) { return class_id; } +static const char *const UNKNOWN_METHOD_NAME = "unknown"; + +// _mark doubles as "already filled in for this chunk" (writeMethods() clears it +// after serializing), exactly as it does for a MethodMap row. +MethodInfo *Lookup::unknownMethod() { + if (!_unknown_method._mark) { + _unknown_method._key = _method_map->unknownMethodId(); + fillNativeMethodInfo(&_unknown_method, UNKNOWN_METHOD_NAME, nullptr); + _unknown_method._mark = true; // last; see the note in fillMethod() + } + return &_unknown_method; +} + +unsigned long Lookup::methodKey(const ASGCT_CallFrame &frame, + jmethodID method_id, jint bci, + u32 vtable_class_id) { + // A null method_id never reaches here -- both callers divert it to + // unknownMethod(), which is not a map entry and so has no key. + assert(method_id != nullptr); + if (bci == BCI_ERROR || bci == BCI_NATIVE_FRAME) { + return MethodMap::makeKey(frame.native_function_name); + } + if (bci == BCI_NATIVE_FRAME_REMOTE) { + return MethodMap::makeKey(frame.packed_remote_frame); + } + if (bci == BCI_VTABLE_RECEIVER) { + return MethodMap::makeVTableReceiverKey(vtable_class_id); + } + [[maybe_unused]] FrameTypeId frame_type = FrameType::decode(bci); + assert(frame_type == FRAME_INTERPRETED || frame_type == FRAME_JIT_COMPILED || + frame_type == FRAME_INLINED || frame_type == FRAME_C1_COMPILED || + VM::isOpenJ9()); // OpenJ9 may have bugs that produce invalid frame types + return MethodMap::makeKey(method_id); +} + MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { - static const char* UNKNOWN = "unknown"; - unsigned long key; jint bci = frame.bci; jmethodID method_id = frame.method_id; @@ -569,43 +602,97 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { method_id = nullptr; } - // Setup siglongjmp protection - // This is outside of a signal handler, there is no reason for allocation to fail, - // other than OOM - ProfiledThread* prof_thread = ProfiledThread::initCurrentThreadSignalSafe(); - if (prof_thread == nullptr) { - return nullptr; + // Nothing to symbolicate and nothing that can fault, so no protection is + // armed for this case at all. + if (method_id == nullptr) { + return unknownMethod(); + } + + // Fast path, deliberately unprotected. For anything but a raw-pointer or + // BCI_VTABLE_RECEIVER frame, methodKey() reads no VM metadata (see its + // comment) and an already-marked row needs no symbolication -- there is + // nothing here that can fault, hence nothing to recover from. Worth + // special-casing because it is the common case once a chunk is warm, and + // arming the protection below is not free: initCurrentThreadSignalSafe() + // blocks and unblocks signals and sigsetjmp(..., 1) reads the signal mask, + // three syscalls on a loop that runs once per frame per trace. + if (!FrameType::isRawPointer(bci) && bci != BCI_VTABLE_RECEIVER) { + MethodMap::iterator it = _method_map->find(methodKey(frame, method_id, bci, 0)); + if (it != _method_map->end() && it->second._mark) { + return &it->second; + } } - MethodInfo* mi = nullptr; + + // Slow path: symbolication reads VM metadata that a concurrent class unload + // may already have freed, so wrap it in a siglongjmp window that + // Profiler::checkFault() jumps back through on SIGSEGV/SIGBUS. + // + // Runs on the dump thread (finishChunk), never in a signal handler, so + // initCurrentThreadSignalSafe() can only fail on OOM. + ProfiledThread *prof_thread = ProfiledThread::initCurrentThreadSignalSafe(); + if (prof_thread == nullptr) { + // No thread context means nowhere to publish a landing pad. Resolve + // unprotected rather than returning nullptr: both call sites in + // writeStackTraces() dereference the result unconditionally, so a nullptr + // return would convert a transient allocation failure into a SIGSEGV on the + // dump thread. Unprotected is also exactly what this code did before the + // protection was added. + Counters::increment(METHOD_RESOLVE_UNPROTECTED); + return fillMethod(frame, method_id, bci); + } + + // Fill the shared "unknown" row *before* arming. The recovery branch below + // runs with protection already disarmed, so it must not allocate -- a second + // fault there would be unrecoverable -- and filling the row does allocate + // (symbol/class dictionary inserts). Once filled it stays filled for the rest + // of the chunk, so this costs one flag test per call after the first. + unknownMethod(); + + // Reinstates the thread's previous landing pad on every exit from this frame, + // including a std::bad_alloc thrown by one of the map or dictionary inserts + // underneath. Leaving ours installed past the end of this frame would leave + // checkFault() jumping into a dead stack frame. + JmpCtxScope jmp_scope(prof_thread); + sigjmp_buf crash_protection_ctx; - sigjmp_buf* prev_buf = prof_thread->getJmpCtx(); if (sigsetjmp(crash_protection_ctx, 1) != 0) { + // checkFault() absorbed a fault raised somewhere in fillMethod() and jumped + // back here, bypassing the SIGNAL_HANDLER_GUARD() destructor in + // segvHandler()/busHandler(); compensate for it, then disarm before + // touching anything else. SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); - prof_thread->setJmpCtx(prev_buf); - key = MethodMap::makeKey(UNKNOWN); - // We want to have counter to record method resoluation failures. - // Unfortunately, the counter cannot be reported accurately, - // see comments in finishChunk(), just above writeCounters() call. - mi = &(*_method_map)[key]; - if (!mi->_mark) { - mi->_mark = true; - if (mi->_key == 0) { - mi->_key = _method_map->allocId(); - } - fillNativeMethodInfo(mi, UNKNOWN, nullptr); - } - return mi; + jmp_scope.restore(); + // Note: the counter cannot be reported accurately, see the comments in + // finishChunk() just above the writeCounters() call. + Counters::increment(METHOD_RESOLVE_LONGJMP_RECOVERED); + // A member, already filled above -- no map lookup, no allocation, and no + // reliance on a local surviving siglongjmp (the value of a non-volatile + // local assigned after sigsetjmp() is indeterminate here). + return &_unknown_method; } - prof_thread->setJmpCtx(&crash_protection_ctx); + jmp_scope.install(&crash_protection_ctx); + return fillMethod(frame, method_id, bci); +} +MethodInfo *Lookup::fillMethod(ASGCT_CallFrame &frame, jmethodID method_id, + jint bci) { // Resolve native method if (FrameType::isRawPointer(bci)) { method_id = JVMSupport::resolve(frame.method); } - // Inject fault to test siglongjmp protection + // Inject fault to test siglongjmp protection. Sits inside the window + // resolveMethod() arms around this function, which is the point: this is + // never compiled into a production build (it needs -PenableFaultInjection). INJECT_CRASH_LIKELY(); + // JVMSupport::resolve() above can yield null for a raw-pointer frame whose + // Method* no longer resolves; resolveMethod() screened out the null it was + // handed, but not this one. + if (method_id == nullptr) { + return unknownMethod(); + } + // BCI_VTABLE_RECEIVER: method holds a VMSymbol* (see vmEntry.h). Resolve // to a class_id via the per-dump cache once, then key MethodMap by the // resolved class_id so two distinct Symbol addresses for the same class @@ -616,26 +703,9 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { vtable_class_id = resolveVTableReceiverCached((void *)method_id); } - if (method_id == nullptr) { - key = MethodMap::makeKey(UNKNOWN); - } else if (bci == BCI_ERROR || bci == BCI_NATIVE_FRAME) { - key = MethodMap::makeKey(frame.native_function_name); - } else if (bci == BCI_NATIVE_FRAME_REMOTE) { - key = MethodMap::makeKey(frame.packed_remote_frame); - } else if (bci == BCI_VTABLE_RECEIVER) { - key = MethodMap::makeVTableReceiverKey(vtable_class_id); - } else { - FrameTypeId frame_type = FrameType::decode(bci); - assert(frame_type == FRAME_INTERPRETED || frame_type == FRAME_JIT_COMPILED || - frame_type == FRAME_INLINED || frame_type == FRAME_C1_COMPILED || - VM::isOpenJ9()); // OpenJ9 may have bugs that produce invalid frame types - key = MethodMap::makeKey(method_id); - } - - mi = &(*_method_map)[key]; + MethodInfo *mi = &(*_method_map)[methodKey(frame, method_id, bci, vtable_class_id)]; if (!mi->_mark) { - mi->_mark = true; bool first_time = mi->_key == 0; if (first_time) { // Allocate a method-pool id that is unique among live methods. Must not @@ -645,9 +715,7 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { // (PROF-15130). The allocator recycles ids freed on erase instead. mi->_key = _method_map->allocId(); } - if (method_id == nullptr) { - fillNativeMethodInfo(mi, UNKNOWN, nullptr); - } else if (bci == BCI_ERROR) { + if (bci == BCI_ERROR) { fillNativeMethodInfo(mi, (const char *)method_id, nullptr); } else if (bci == BCI_NATIVE_FRAME) { const char *name = (const char *)method_id; @@ -700,8 +768,18 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { } else { fillJavaMethodInfo(mi, method_id, first_time); } + // Mark last, never before the fill above. The fill walks VM metadata that a + // concurrent class unload may have freed, so it can siglongjmp straight out + // of this function (and fillJavaMethodInfo() also returns early when + // PushLocalFrame fails). Marking up front would leave the row marked but + // still default-constructed: writeMethods() serializes any marked row, so + // the chunk would gain a method with an empty class/name/sig typed as + // FRAME_INTERPRETED, and every later frame with this key would reuse it. + // Left unmarked, the row is skipped by writeMethods(), retried by the next + // frame that needs it, and eventually aged out by + // cleanupUnreferencedMethods(), which recycles its _key. + mi->_mark = true; } - prof_thread->setJmpCtx(prev_buf); return mi; } @@ -1726,6 +1804,18 @@ int Recording::writeStackTraces(Buffer *buf, Lookup *lookup) { return trace_count > 0 ? 1 : 0; } +// Serializes one method-pool entry and clears its mark, so the next chunk +// re-resolves it (symbol/class ids are per-chunk). +static void writeMethodEntry(Buffer *buf, MethodInfo &mi) { + mi._mark = false; + buf->putVar64(mi._key); + buf->putVar64(mi._class); + buf->putVar64(mi._name); + buf->putVar64(mi._sig); + buf->putVar64(mi._modifiers); + buf->putVar64(mi.isHidden()); +} + int Recording::writeMethods(Buffer *buf, Lookup *lookup) { MethodMap *method_map = lookup->_method_map; @@ -1736,6 +1826,13 @@ int Recording::writeMethods(Buffer *buf, Lookup *lookup) { marked_count++; } } + // Lookup::_unknown_method is deliberately not a map entry (see its + // declaration), so the walk above cannot see it. It still has to be emitted: + // writeStackTraces() wrote its _key for every frame that resolved to it, and a + // _key absent from this pool is a dangling reference in the chunk. + if (lookup->_unknown_method._mark) { + marked_count++; + } if (marked_count == 0) { return 0; @@ -1747,16 +1844,14 @@ int Recording::writeMethods(Buffer *buf, Lookup *lookup) { ++it) { MethodInfo &mi = it->second; if (mi._mark) { - mi._mark = false; - buf->putVar64(mi._key); - buf->putVar64(mi._class); - buf->putVar64(mi._name); - buf->putVar64(mi._sig); - buf->putVar64(mi._modifiers); - buf->putVar64(mi.isHidden()); + writeMethodEntry(buf, mi); flushIfNeeded(buf); } } + if (lookup->_unknown_method._mark) { + writeMethodEntry(buf, lookup->_unknown_method); + flushIfNeeded(buf); + } return 1; } diff --git a/ddprof-lib/src/main/cpp/flightRecorder.h b/ddprof-lib/src/main/cpp/flightRecorder.h index 09ab03f660..1de0ba7371 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.h +++ b/ddprof-lib/src/main/cpp/flightRecorder.h @@ -165,8 +165,23 @@ class MethodMap : public std::map { } } + // Pool id for Lookup::_unknown_method, the shared row for frames that could + // not be resolved. That row deliberately lives outside this map (see + // Lookup::unknownMethod()), so it needs an id that no map entry can ever be + // given: drawn from the same counter so it cannot collide, but drawn only + // once for the life of the recording and never recycled, since + // cleanupUnreferencedMethods() only ever erases -- and frees the ids of -- + // actual map entries. + u32 unknownMethodId() { + if (_unknown_method_id == 0) { + _unknown_method_id = allocId(); + } + return _unknown_method_id; + } + private: u32 _id_high_water = 0; + u32 _unknown_method_id = 0; std::vector _free_ids; }; @@ -368,6 +383,20 @@ class Lookup { Dictionary _packages; Dictionary _symbols; + // The single row every frame that could not be resolved collapses onto. + // + // Deliberately NOT a MethodMap entry: resolveMethod()'s siglongjmp landing pad + // hands this row back with crash protection already disarmed, so it must be + // reachable without touching the map, whose operator[] allocates a node and + // can throw std::bad_alloc. + // + // Because it is outside the map, the map walk in writeMethods() cannot see it, + // so writeMethods() emits it separately. It has to reach the method pool: + // writeStackTraces() writes its _key for every frame that resolved to it, and + // a _key with no matching pool entry is a dangling reference in the chunk. + // Public for that reason, matching _method_map/_symbols above. + MethodInfo _unknown_method; + private: void fillNativeMethodInfo(MethodInfo *mi, const char *name, const char *lib_name); @@ -401,6 +430,28 @@ class Lookup { // increments VTABLE_RECEIVER_RESOLVE_FAILED. u32 resolveVTableReceiverCached(void *sym); + // The MethodMap row `frame` belongs to. Factored out so resolveMethod()'s + // unprotected fast path and fillMethod()'s protected slow path can never + // disagree about which row a frame maps to -- ASGCT_CallFrame's method_id / + // native_function_name / packed_remote_frame / method fields are a union, so + // the bci branching below is the only thing that gives the payload a meaning. + // + // Reads the union's *value* only: MethodMap::makeKey() hashes a pointer, it + // never dereferences it. So for every bci except BCI_VTABLE_RECEIVER -- whose + // class_id the caller must resolve from a VMSymbol* first -- computing a key + // touches no VM metadata and cannot fault. + unsigned long methodKey(const ASGCT_CallFrame &frame, jmethodID method_id, + jint bci, u32 vtable_class_id); + + // Resolves and fills in the MethodInfo for `frame`. This is the part that + // reads VM metadata and may therefore fault; resolveMethod() wraps it in the + // sigsetjmp/siglongjmp window. + MethodInfo *fillMethod(ASGCT_CallFrame &frame, jmethodID method_id, jint bci); + + // Materializes _unknown_method for this dump (filling it on first use) and + // returns it. + MethodInfo *unknownMethod(); + public: Lookup(Recording *rec, MethodMap *method_map, StringDictionary *classes) : _rec(rec), _method_map(method_map), _classes(classes), _packages(), diff --git a/ddprof-lib/src/main/cpp/guards.cpp b/ddprof-lib/src/main/cpp/guards.cpp index 56e46a8cba..c81ca21316 100644 --- a/ddprof-lib/src/main/cpp/guards.cpp +++ b/ddprof-lib/src/main/cpp/guards.cpp @@ -27,7 +27,9 @@ int getInSignalDepth() { ProfiledThread *pt = ProfiledThread::current(); - return pt != nullptr ? static_cast(pt->signalDepth()) : 0; + // Deliberately returns the raw counter, negative values included, so a + // pairing bug is visible to tests and diagnostics rather than clamped away. + return pt != nullptr ? pt->signalDepth() : 0; } bool isInTrackedSignalContext() { @@ -35,7 +37,14 @@ bool isInTrackedSignalContext() { // null ProfiledThread = no thread context; the SignalHandlerScope // never ran, so we have no positive evidence of a signal frame. // See header comment for the rationale of returning false here. - return pt != nullptr && pt->signalDepth() != 0; + // + // `> 0`, not `!= 0`: exitSignalScope() asserts the depth never drops below + // zero, but that assert is compiled out under -DNDEBUG, so in a release + // build an unmatched decrement would leave the counter negative. Reading a + // negative depth as "not in a signal handler" keeps the blast radius of + // such a bug to the one bad decrement, instead of latching dlopen_hook onto + // the deferred-refresh path for the rest of the thread's life. + return pt != nullptr && pt->signalDepth() > 0; } SignalHandlerScope::SignalHandlerScope() : _active(true) { @@ -76,6 +85,16 @@ void signalHandlerUnwindAfterLongjmp() { } } +JmpCtxScope::JmpCtxScope(ProfiledThread *pt) : _pt(pt), _prev(pt->getJmpCtx()) { + assert(pt != nullptr); +} + +JmpCtxScope::~JmpCtxScope() { _pt->setJmpCtx(_prev); } + +void JmpCtxScope::install(sigjmp_buf *ctx) { _pt->setJmpCtx(ctx); } + +void JmpCtxScope::restore() { _pt->setJmpCtx(_prev); } + // Static bitmap storage for fallback cases uint64_t CriticalSection::_fallback_bitmap[CriticalSection::FALLBACK_BITMAP_WORDS] = {}; diff --git a/ddprof-lib/src/main/cpp/guards.h b/ddprof-lib/src/main/cpp/guards.h index f3ead4e259..c7aec08f69 100644 --- a/ddprof-lib/src/main/cpp/guards.h +++ b/ddprof-lib/src/main/cpp/guards.h @@ -19,6 +19,7 @@ #include #include +#include // sigjmp_buf (JmpCtxScope) #include #include @@ -105,6 +106,56 @@ class SignalHandlerScope { void signalHandlerUnwindAfterLongjmp(); #define SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP() signalHandlerUnwindAfterLongjmp() +// RAII for the per-thread siglongjmp landing pad (ProfiledThread::_jmp_buf) +// that Profiler::checkFault() jumps through. +// +// The previous landing pad must be reinstated on *every* exit from the frame +// that owns the sigjmp_buf -- normal return, a siglongjmp back into it, or an +// exception unwinding out of it -- because checkFault() will happily jump into +// a landing pad whose stack frame has already been popped. Hand-rolled +// "setJmpCtx(prev) before each return" only covers the returns the author +// remembered. +// +// Both members are const and initialised before the owning frame calls +// sigsetjmp(), and install()/restore() mutate only the ProfiledThread, so the +// guard's own state is never modified between sigsetjmp() and siglongjmp(). +// Reading it from the landing pad is therefore well defined -- unlike a plain +// non-volatile local, whose value after siglongjmp is indeterminate if it was +// assigned in the meantime. +// +// Usage: +// sigjmp_buf ctx; +// JmpCtxScope jmp_scope(prof_thread); // pt must be non-null +// if (sigsetjmp(ctx, 1) != 0) { // savemask=1: see note below +// SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); +// jmp_scope.restore(); // disarm before anything else +// return recovery_value; +// } +// jmp_scope.install(&ctx); +// ... risky work ... +// +// savemask must be 1: the siglongjmp originates inside the SIGSEGV handler, +// where the kernel has SIGSEGV blocked, so without restoring the saved mask the +// signal would stay blocked and the next fault on this thread would be fatal. +class JmpCtxScope { +public: + // `pt` must be non-null. + explicit JmpCtxScope(ProfiledThread* pt); + ~JmpCtxScope(); + // Publish `ctx` as this thread's landing pad; call after sigsetjmp() + // returns 0. + void install(sigjmp_buf* ctx); + // Reinstate the previous landing pad now. Idempotent with the destructor, + // so it is safe (and required) to call from the sigsetjmp landing pad + // before touching anything that could fault again. + void restore(); + JmpCtxScope(const JmpCtxScope&) = delete; + JmpCtxScope& operator=(const JmpCtxScope&) = delete; +private: + ProfiledThread* const _pt; + sigjmp_buf* const _prev; +}; + /** * Race-free critical section using atomic compare-and-swap. * diff --git a/ddprof-lib/src/main/cpp/threadLocalData.h b/ddprof-lib/src/main/cpp/threadLocalData.h index 645436f482..2ef818a6a3 100644 --- a/ddprof-lib/src/main/cpp/threadLocalData.h +++ b/ddprof-lib/src/main/cpp/threadLocalData.h @@ -140,6 +140,14 @@ class ProfiledThread : public ThreadLocalData { delete pt; NativeMem::record(NM_THREAD_LOCAL, -(long long)sizeof(ProfiledThread)); } + // Forces the signal-handler depth to an arbitrary value, including a negative + // one that exitSignalScope()'s assert would refuse to produce. Exists so + // signalSafety_ut can check that isInTrackedSignalContext() reads a corrupted + // (negative) depth as "not in a signal handler" -- the only guard a release + // build has left once that assert is compiled out by -DNDEBUG. + void setSignalDepthForTest(int depth) { + __atomic_store_n(&_signal_depth, depth, __ATOMIC_RELAXED); + } #endif // initCurrentThread() and release() are not async-signal-safe: // must be called outside of a signal handler with signal blocked @@ -247,7 +255,7 @@ class ProfiledThread : public ThreadLocalData { // Signal-handler depth counter used by SignalHandlerScope (guards.h). // But read-modify-store can be interrupted by other signals, so it has to be an atomic counter. - inline uint8_t signalDepth() const { return __atomic_load_n(&_signal_depth, __ATOMIC_RELAXED); } + inline int signalDepth() const { return __atomic_load_n(&_signal_depth, __ATOMIC_RELAXED); } inline void enterSignalScope() { __atomic_fetch_add(&_signal_depth, 1, __ATOMIC_RELAXED); } // Every real exitSignalScope() call is paired with a prior enterSignalScope(): // either the normal SignalHandlerScope destructor, or exactly one compensating @@ -256,6 +264,12 @@ class ProfiledThread : public ThreadLocalData { // reachable from segvHandler()/busHandler(), both of which open SIGNAL_HANDLER_GUARD() // first). A call here with depth already 0 means that pairing was broken elsewhere; // that is a real bug and must fail loudly, not be silently tolerated. + // + // The assert is compiled out under -DNDEBUG, so a release build would carry a + // negative depth instead of aborting. Nothing latches on that: the only + // production reader, isInTrackedSignalContext(), tests `> 0` precisely so a + // negative value reads as "not in a signal handler" rather than as "forever + // in one" (see guards.cpp). inline void exitSignalScope() { int depth = __atomic_fetch_sub(&_signal_depth, 1, __ATOMIC_RELAXED); assert(depth > 0); diff --git a/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp b/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp index c7eff90687..a6a04accd5 100644 --- a/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp +++ b/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp @@ -51,5 +51,10 @@ TEST(HotspotMethodIdTest, RejectedMethodIdStaysNonRawAndResolvesToUnknown) { ASSERT_NE(info, nullptr); EXPECT_EQ(info->_type, FRAME_NATIVE); - EXPECT_EQ(methods.size(), 1U); + // The sentinel is normalised to a null method_id, which resolves to the + // shared unknown row. That row lives outside the MethodMap (see + // Lookup::_unknown_method), so nothing is inserted for this frame. + EXPECT_EQ(info, &lookup._unknown_method); + EXPECT_TRUE(methods.empty()); + EXPECT_NE(info->_key, 0U); // still needs a pool id to be referenceable } diff --git a/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp b/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp index 5820f50be5..262fc950f3 100644 --- a/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp +++ b/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp @@ -69,25 +69,20 @@ class ResolveMethodFaultInjectionTest : public ::testing::Test { } }; -// resolveMethod() wraps its body in a sigsetjmp/siglongjmp jmp ctx specifically -// so that INJECT_CRASH_LIKELY() (a real SIGSEGV, not a poisoned pointer left -// for the caller to dereference) is recoverable: when it fires, control must -// land back at the sigsetjmp, producing the same "unknown" MethodInfo that a -// genuine resolution failure would, and the process must not crash. +// resolveMethod() arms a sigsetjmp/siglongjmp window around fillMethod() +// specifically so that INJECT_CRASH_LIKELY() (a real SIGSEGV, not a poisoned +// pointer left for the caller to dereference) is recoverable: when it fires, +// control must land back at the sigsetjmp, producing the same "unknown" +// MethodInfo that a genuine resolution failure would, and the process must not +// crash. // -// The frame here has a NULL method_id and bci == 0 (not a raw-pointer bci), -// so both the non-injected (~99%) and injected-and-recovered (~1%) paths -// converge on the exact same MethodMap key/fill (see flightRecorder.cpp: -// `if (method_id == nullptr) fillNativeMethodInfo(mi, UNKNOWN, nullptr);`), -// which keeps the assertions below valid regardless of which path any given -// call took. -// -// Note: on the non-recovering (success) path resolveMethod() deliberately -// leaves the ProfiledThread's jmp ctx pointing at its own (now-popped) stack -// frame rather than restoring the caller's prior context -- each call -// re-installs a fresh one via sigsetjmp before doing anything risky, so this -// is safe, but it does mean ProfiledThread::isProtected() cannot be used -// here to distinguish a recovered call from a normal one. +// The frame is a BCI_ERROR frame carrying a plain error string. That shape is +// chosen so the call actually enters the protected window: a null method_id +// short-circuits to the shared unknown row before any protection is armed, and +// BCI_ERROR resolution needs no live JVM (it goes to fillNativeMethodInfo, not +// fillJavaMethodInfo). Both the non-injected (~99%) and injected (~1%) paths +// therefore produce a FRAME_NATIVE MethodInfo, which keeps the per-iteration +// assertions valid regardless of which path any given call took. TEST_F(ResolveMethodFaultInjectionTest, RecoversFromInjectedCrashInsteadOfCrashing) { ProfiledThread* t = ProfiledThread::current(); ASSERT_NE(t, nullptr); @@ -98,8 +93,8 @@ TEST_F(ResolveMethodFaultInjectionTest, RecoversFromInjectedCrashInsteadOfCrashi Lookup lookup(nullptr, &methods, &classes); ASGCT_CallFrame frame{}; - frame.bci = 0; - frame.method_id = nullptr; + frame.bci = BCI_ERROR; + frame.native_function_name = "injected_fault_test_frame"; long long faultsBefore = Counters::getCounter(FAULTS_INJECTED); bool sawRecoveredInjection = false; @@ -107,31 +102,49 @@ TEST_F(ResolveMethodFaultInjectionTest, RecoversFromInjectedCrashInsteadOfCrashi // LIKELY tier fires ~1% of the time; 5000 tries makes seeing at least one // recovery astronomically likely without hardcoding an exact iteration. for (int i = 0; i < 5000; i++) { - long long recoveredBefore = Counters::getCounter(STACKWALK_LONGJMP_RECOVERED); + // Start each iteration from an empty map so the call is forced down + // resolveMethod()'s protected slow path. Its fast path deliberately + // short-circuits an already-marked row without arming any protection, and + // INJECT_CRASH_LIKELY() lives inside the protected window -- so without the + // reset only the very first iteration would ever reach the injection site. + methods.clear(); + + long long recoveredBefore = + Counters::getCounter(METHOD_RESOLVE_LONGJMP_RECOVERED); MethodInfo* info = lookup.resolveMethod(frame); ASSERT_NE(info, nullptr); EXPECT_EQ(info->_type, FRAME_NATIVE); - - if (Counters::getCounter(STACKWALK_LONGJMP_RECOVERED) > recoveredBefore) { + // Every exit path restores the jmp ctx the thread had on entry (nullptr + // here), recovery included -- a landing pad left published would point into + // resolveMethod()'s already-popped stack frame. Asserting it per iteration + // covers both the success and the recovery path. + EXPECT_FALSE(t->isProtected()); + + if (Counters::getCounter(METHOD_RESOLVE_LONGJMP_RECOVERED) > recoveredBefore) { + // The recovery path must hand back the shared unknown row, and must not + // have touched the MethodMap: it runs with protection already disarmed, so + // an allocating map insert there is exactly what it must avoid. The + // injection fires ahead of the map lookup in fillMethod(), so on a + // recovering iteration the map is still empty. + EXPECT_EQ(info, &lookup._unknown_method); + EXPECT_TRUE(methods.empty()); + EXPECT_NE(info->_key, 0U); // must still be referenceable from a trace sawRecoveredInjection = true; break; } + + // Non-injected iteration: resolved normally into its own map row, not the + // shared unknown one. + EXPECT_NE(info, &lookup._unknown_method); + EXPECT_EQ(methods.size(), 1U); } EXPECT_TRUE(sawRecoveredInjection) - << "expected INJECT_CRASH_LIKELY() in Lookup::resolveMethod() to fire " + << "expected INJECT_CRASH_LIKELY() in Lookup::fillMethod() to fire " "and be recovered via siglongjmp within 5000 calls"; EXPECT_GT(Counters::getCounter(FAULTS_INJECTED), faultsBefore); - - // Every call -- injected-and-recovered or not -- resolves the same NULL - // method_id frame to the single shared "unknown" MethodInfo row. - EXPECT_EQ(methods.size(), 1U); - - // Defensive cleanup: leaving the stale post-call jmp ctx (see note above) - // live into TearDown()/ProfiledThread::release() serves no purpose here. - t->setJmpCtx(nullptr); } #else // __FAULT_INJECTION__ not defined (the default release/debug build). @@ -150,13 +163,38 @@ TEST(ResolveMethodFaultInjectionTest, DisabledBuildResolvesNormally) { MethodMap methods; Lookup lookup(nullptr, &methods, &classes); - ASGCT_CallFrame frame{}; - frame.bci = 0; - frame.method_id = nullptr; + // A null method_id short-circuits to the shared unknown row before any + // protection is armed. That row lives outside the MethodMap + // (Lookup::_unknown_method), so nothing is inserted for it. + ASGCT_CallFrame unresolvable{}; + unresolvable.bci = 0; + unresolvable.method_id = nullptr; + + MethodInfo* unknown = lookup.resolveMethod(unresolvable); + ASSERT_NE(unknown, nullptr); + EXPECT_EQ(unknown, &lookup._unknown_method); + EXPECT_EQ(unknown->_type, FRAME_NATIVE); + EXPECT_TRUE(unknown->_mark); + EXPECT_NE(unknown->_key, 0U); // must still be referenceable from a trace + EXPECT_TRUE(methods.empty()); + + // A BCI_ERROR frame does go through the protected slow path, so this covers + // the sigsetjmp window itself being inert here rather than just the + // short-circuit above. + ASGCT_CallFrame error_frame{}; + error_frame.bci = BCI_ERROR; + error_frame.native_function_name = "disabled_build_test_frame"; + + MethodInfo* resolved = lookup.resolveMethod(error_frame); + ASSERT_NE(resolved, nullptr); + EXPECT_NE(resolved, &lookup._unknown_method); + EXPECT_EQ(resolved->_type, FRAME_NATIVE); + EXPECT_EQ(methods.size(), 1U); - MethodInfo* info = lookup.resolveMethod(frame); - ASSERT_NE(info, nullptr); - EXPECT_EQ(info->_type, FRAME_NATIVE); + // The jmp ctx is restored on the normal path, not just on recovery. + ProfiledThread* t = ProfiledThread::current(); + ASSERT_NE(t, nullptr); // the protected path creates one if the thread had none + EXPECT_FALSE(t->isProtected()); } #endif // __FAULT_INJECTION__ diff --git a/ddprof-lib/src/test/cpp/signalSafety_ut.cpp b/ddprof-lib/src/test/cpp/signalSafety_ut.cpp index 8b3330650b..c19660cae9 100644 --- a/ddprof-lib/src/test/cpp/signalSafety_ut.cpp +++ b/ddprof-lib/src/test/cpp/signalSafety_ut.cpp @@ -127,16 +127,31 @@ TEST_F(SignalSafetyTest, SignalHandlerUnwindAfterLongjmpDecrementsOnce) { // segvHandler()/busHandler(), both of which open a SIGNAL_HANDLER_GUARD() // first -- so every real compensating call is paired with a prior // enterSignalScope(). Calling it here with no matching scope at all (depth -// already 0) is exactly the kind of pairing bug that must never be -// tolerated silently: it has to abort loudly instead of underflowing the -// counter (which signalDepth() truncates to uint8_t, so an underflow would -// silently wrap to a large positive value and corrupt -// isInTrackedSignalContext() for the rest of the thread's life). +// already 0) is exactly the kind of pairing bug that must never be tolerated +// silently, so exitSignalScope() asserts on it. TEST_F(SignalSafetyTest, SignalHandlerUnwindAfterLongjmpAbortsOnUnmatchedCall) { EXPECT_EQ(0, getInSignalDepth()); EXPECT_DEATH({ signalHandlerUnwindAfterLongjmp(); }, "Assertion .*"); } +// Release-build backstop for the same bug: -DNDEBUG compiles the assert above +// out, so a release binary carries the negative depth instead of aborting. +// isInTrackedSignalContext() tests `> 0` rather than `!= 0` precisely so a +// negative depth reads as "not in a signal handler"; reading it as "in one" +// would pin Profiler::dlopen_hook to the deferred-refresh path for the rest of +// the thread's life. Injected directly rather than through exitSignalScope() so +// this stays a test of the reader, not of the assert. +TEST_F(SignalSafetyTest, NegativeDepthIsNotTrackedSignalContext) { + ProfiledThread* pt = ProfiledThread::current(); + ASSERT_NE(nullptr, pt); + + pt->setSignalDepthForTest(-1); + EXPECT_EQ(-1, getInSignalDepth()); + EXPECT_FALSE(isInTrackedSignalContext()); + + pt->setSignalDepthForTest(0); // TearDown asserts the depth is back to 0 +} + TEST(SignalSafetyTestNoContext, NullProfiledThreadIsNotTrackedSignal) { // isInTrackedSignalContext() returns false on null because the // SignalHandlerScope never ran — used by Profiler::dlopen_hook so diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/metadata/MethodIdReuseTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/metadata/MethodIdReuseTest.java index 146d28c71b..6b54edfe60 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/metadata/MethodIdReuseTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/metadata/MethodIdReuseTest.java @@ -62,6 +62,13 @@ * out and get erased) while touching a DIFFERENT persistent set plus brand-new lambdas (which draw * recycled ids). The oracle asserts no {@code jdk.types.Method} id maps to two distinct method * definitions within any chunk. + * + *

The same raw-chunk walk also enforces the complementary property: method-pool + * referential integrity — every method id a stack frame points at was actually emitted in + * that chunk's method pool. A missing entry is the mirror image of a duplicate one and is equally + * invisible to lenient parsers (jafar returns {@code null} and the frame renders nameless rather + * than failing), so this is the only place it is checked. See + * {@link Chunk#danglingMethodRefs()}. */ public class MethodIdReuseTest extends AbstractProfilerTest { @@ -238,18 +245,33 @@ public void methodPoolIdsAreUniquePerChunk() throws Exception { System.out.println("[PROF-15130] " + p.toAbsolutePath()); } - // Oracle: across every chunk in every dump, no method-pool id maps to two distinct method - // definitions. + // Two oracles over every chunk in every dump: + // 1. no method-pool id maps to two distinct method definitions (PROF-15130); + // 2. every method id a stack frame references was actually emitted in that chunk's method + // pool. This is referential integrity of the method pool, and the same raw-chunk walk + // answers it for free -- see Chunk.danglingMethodRefs() for why no other test can. int totalDuplicates = 0; + List allDangling = new ArrayList<>(); for (Path dump : dumps) { - int dups = countDuplicateMethodIds(dump); - System.out.println("[PROF-15130] " + dump.getFileName() + " duplicate method-pool ids: " + dups); - totalDuplicates += dups; + PoolAudit audit = auditMethodPools(dump); + System.out.println("[PROF-15130] " + dump.getFileName() + + " duplicate method-pool ids: " + audit.duplicateIds + + ", dangling frame->method refs: " + audit.danglingRefs.size()); + for (String d : audit.danglingRefs) { + System.out.println("[PROF-15130] DANGLING " + d); + } + totalDuplicates += audit.duplicateIds; + allDangling.addAll(audit.danglingRefs); } assertEquals(0, totalDuplicates, "Found " + totalDuplicates + " jdk.types.Method constant-pool id(s) mapping to two " + "distinct method definitions (PROF-15130). See stdout for the dump files."); + + assertEquals(0, allDangling.size(), + "Found " + allDangling.size() + " stack frame reference(s) to a jdk.types.Method id " + + "that the chunk's method constant pool never emitted, which lenient " + + "parsers render as a nameless frame instead of rejecting: " + allDangling); } @Override @@ -267,8 +289,15 @@ protected String getProfilerCommand() { // two DISTINCT method definitions within the same chunk. This is the precise duplicate-id // oracle (JMC's last-wins loader would silently hide it). - /** @return number of method-pool ids in the file that map to >1 distinct definition. */ - static int countDuplicateMethodIds(Path file) throws IOException { + /** Findings from auditing every chunk's method constant pool in one file. */ + static final class PoolAudit { + /** Method-pool ids mapping to >1 distinct definition (PROF-15130). */ + int duplicateIds; + /** Human-readable description of each stack frame reference with no pool entry. */ + final List danglingRefs = new ArrayList<>(); + } + + static PoolAudit auditMethodPools(Path file) throws IOException { byte[] all; try (FileChannel ch = FileChannel.open(file, StandardOpenOption.READ)) { long size = ch.size(); @@ -276,8 +305,9 @@ static int countDuplicateMethodIds(Path file) throws IOException { while (bb.hasRemaining() && ch.read(bb) > 0) { /* read fully */ } all = bb.array(); } - int duplicates = 0; + PoolAudit audit = new PoolAudit(); long pos = 0; + int chunkIndex = 0; while (pos + 8 <= all.length) { // Chunk magic "FLR\0" if (!(all[(int) pos] == 'F' && all[(int) pos + 1] == 'L' && all[(int) pos + 2] == 'R' @@ -285,13 +315,19 @@ static int countDuplicateMethodIds(Path file) throws IOException { break; } Chunk chunk = new Chunk(all, (int) pos); - duplicates += chunk.countDuplicateMethodIds(); + audit.duplicateIds += chunk.countDuplicateMethodIds(); + for (Long missing : chunk.danglingMethodRefs()) { + audit.danglingRefs.add(file.getFileName() + " chunk#" + chunkIndex + + " frame references method id=" + missing + + " which the chunk's method pool never emitted"); + } if (chunk.chunkSize <= 0) { break; } pos += chunk.chunkSize; + chunkIndex++; } - return duplicates; + return audit; } private static final class Chunk { @@ -322,6 +358,15 @@ private static final class Chunk { // id -> set of DISTINCT raw [type,name,descriptor] ref-tuples seen for that method id. // size() > 1 ⇒ the id carried two different method definitions in this chunk ⇒ the bug. private final Map> methodRefTuples = new LinkedHashMap<>(); + // Every jdk.types.Method id referenced by a stack frame in this chunk. Any id in here but + // NOT in methodRefTuples.keySet() is a dangling reference: the frame points at a method + // constant-pool entry the chunk never emitted. + private final Set referencedMethodIds = new HashSet<>(); + // Set when an unknown class layout forced the checkpoint walk to stop early. The method + // pool is written AFTER the stack-trace pool (writeCpool: writeStackTraces then + // writeMethods), so a truncated walk can see frame references without their definitions — + // which looks exactly like a dangling reference. Suppress that check rather than report it. + private boolean walkTruncated; int countDuplicateMethodIds() { // Follow the checkpoint delta chain. @@ -344,6 +389,7 @@ int countDuplicateMethodIds() { ClassDef cd = classes.get(classId); if (cd == null) { // Unknown layout — cannot safely parse further in this checkpoint. + walkTruncated = true; return duplicateCount(); } boolean isMethod = classId == methodTypeId; @@ -391,6 +437,29 @@ int countDuplicateMethodIds() { return duplicateCount(); } + /** + * Method ids referenced by a stack frame in this chunk with no matching entry in the + * chunk's method constant pool. Empty when the walk was truncated (see walkTruncated). + * + *

A dangling reference is invisible to lenient parsers: jafar's ConstantPool.get() + * returns null, so the frame silently renders with no class/method name instead of failing. + * That makes this raw-chunk check the only reliable oracle for it. It is what catches a + * MethodInfo being handed to writeStackTraces() but never serialized by writeMethods() — + * e.g. a row that lives outside MethodMap, or one left unmarked by a mid-fill failure. + */ + List danglingMethodRefs() { + List missing = new ArrayList<>(); + if (walkTruncated) { + return missing; + } + for (Long ref : referencedMethodIds) { + if (!methodRefTuples.containsKey(ref)) { + missing.add(ref); + } + } + return missing; + } + private int duplicateCount() { int dups = 0; for (Map.Entry> en : methodRefTuples.entrySet()) { @@ -436,7 +505,16 @@ private Object readField(long[] p, FieldDef fd) { private Object readScalar(long[] p, FieldDef fd) { if (fd.constantPool) { - return readVarLong(p); // a constant-pool reference id + long ref = readVarLong(p); // a constant-pool reference id + // Every reference to jdk.types.Method from anywhere in the chunk, whatever the + // enclosing type. In practice that is jdk.types.StackFrame.method (see + // jfrMetadata.cpp), reached through the generic skip-this-entry path below. + // Keying off the field's declared type rather than the enclosing type means this + // keeps working if another type ever gains a method reference. Ref 0 is JFR's null. + if (fd.typeId == methodTypeId && ref != 0) { + referencedMethodIds.add(ref); + } + return ref; } ClassDef t = classes.get(fd.typeId); String tn = t != null ? t.name : null; From 2d18b23362904bb6f0d4469766d5bdab02635b0f Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Fri, 21 Aug 2026 13:28:48 -0400 Subject: [PATCH 06/19] Fix merge --- ddprof-lib/src/main/cpp/counters.h | 33 +- ddprof-lib/src/main/cpp/flightRecorder.cpp | 42 +- ddprof-lib/src/main/cpp/guards.cpp | 47 ++- ddprof-lib/src/main/cpp/guards.h | 105 ++--- .../src/main/cpp/hotspot/hotspotSupport.cpp | 250 +++++++++--- .../src/test/cpp/hotspotMethodId_ut.cpp | 379 ++++++++++++++++++ .../test/cpp/hotspot_crash_protection_ut.cpp | 158 ++++++++ 7 files changed, 857 insertions(+), 157 deletions(-) diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index d69d431bec..c3b8b3c555 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/ddprof-lib/src/main/cpp/counters.h @@ -136,24 +136,31 @@ /* Every siglongjmp recovery, from any protected window, counted centrally \ * in Profiler::checkFault(). */ \ X(STACKWALK_LONGJMP_RECOVERED, "stackwalk_longjmp_recovered") \ - /* Subset of the above: recoveries that landed in Lookup::resolveMethod(), \ - * i.e. faults while symbolicating at dump time rather than while walking a \ - * stack in a signal handler. Counted separately because the two have \ - * different root causes (stale jmethodID / class unload vs. a bad frame \ - * pointer) and would otherwise be indistinguishable. */ \ + /* Dump-time raw-Method* resolution (HotspotSupport::resolve, reached only \ + * for cstack=vm + fjmethodid=false frames). NOT additive with \ + * STACKWALK_LONGJMP_RECOVERED: checkFault() bumps that one unconditionally \ + * before every siglongjmp, so each fault counted here is counted there too. \ + * Subtract, never sum. Non-zero means stale HotSpot metadata (GC or class \ + * unloading) reached the dump thread; the frame serializes as "unknown". */ \ + X(METHOD_RESOLVE_FAULT_RECOVERED, "method_resolve_fault_recovered") \ + /* Subset of the above: recoveries that landed in Lookup::resolveMethod() \ + * (flightRecorder.cpp), i.e. faults while symbolicating a Java frame at \ + * dump time via fillMethod() rather than during HotspotSupport::resolve()'s\ + * raw-Method* walk. Counted separately because the two protected windows \ + * cover different code and have different root causes. */ \ X(METHOD_RESOLVE_LONGJMP_RECOVERED, "method_resolve_longjmp_recovered") \ - /* Lookup::resolveMethod() calls that ran without siglongjmp protection \ - * because no ProfiledThread could be allocated for the dump thread (OOM): \ - * there is nowhere to publish a landing pad. Expected to stay at 0. */ \ - X(METHOD_RESOLVE_UNPROTECTED, "method_resolve_unprotected") \ + /* Symbol length/body rejected during the same resolution: unreadable, \ + * empty, or longer than the fixed dump-time buffers in hotspotSupport.cpp. \ + * The frame serializes as "unknown". */ \ + X(METHOD_RESOLVE_SYMBOL_UNREADABLE, "method_resolve_symbol_unreadable") \ /* Strict subset of SAMPLES_DROPPED_THREAD_LOCAL, not an independent count: \ * ThreadLocalDataPool::claim() increments this on capacity exhaustion, and \ * every acquireCurrent() caller that gets nullptr back -- for this or any \ * other reason -- separately increments SAMPLES_DROPPED_THREAD_LOCAL too. \ - * So every pool-exhaustion drop bumps both counters together; the two \ - * should be subtracted (thread_local_pool_exhausted from \ - * samples_dropped_thread_local) to isolate non-pool priming drops, never \ - * summed. */ \ + * So every pool-exhaustion drop bumps both counters together; the two \ + * should be subtracted (thread_local_pool_exhausted from \ + * samples_dropped_thread_local) to isolate non-pool priming drops, never \ + * summed. */ \ X(SAMPLES_DROPPED_TLS_POOL_EXHAUSTED, "thread_local_pool_exhausted") \ /* writeElement() guards against a corrupted/dangling JfrMetadata tree. \ * Root cause is still unconfirmed, so these counters are the durable \ diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp index ee6c5793b5..3e0a9169c1 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.cpp +++ b/ddprof-lib/src/main/cpp/flightRecorder.cpp @@ -624,14 +624,14 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { // initCurrentThreadSignalSafe() can only fail on OOM. ProfiledThread *prof_thread = ProfiledThread::initCurrentThreadSignalSafe(); if (prof_thread == nullptr) { - // No thread context means nowhere to publish a landing pad. Resolve - // unprotected rather than returning nullptr: both call sites in - // writeStackTraces() dereference the result unconditionally, so a nullptr - // return would convert a transient allocation failure into a SIGSEGV on the - // dump thread. Unprotected is also exactly what this code did before the - // protection was added. - Counters::increment(METHOD_RESOLVE_UNPROTECTED); - return fillMethod(frame, method_id, bci); + // No thread context means nowhere to publish a landing pad. Resolve to + // the shared unknown row rather than touching VM metadata unprotected -- + // both call sites in writeStackTraces() dereference the result + // unconditionally, so a nullptr return would convert a transient + // allocation failure into a SIGSEGV on the dump thread. Same counter + // HotspotSupport::resolve() uses for the identical no-landing-pad case. + Counters::increment(SAMPLES_DROPPED_THREAD_LOCAL); + return unknownMethod(); } // Fill the shared "unknown" row *before* arming. The recovery branch below @@ -648,15 +648,17 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { JmpCtxScope jmp_scope(prof_thread); sigjmp_buf crash_protection_ctx; + // savemask must be 1: the siglongjmp originates inside segvHandler, where + // the kernel has SIGSEGV blocked, so without restoring the saved mask the + // signal would stay blocked and the next fault on this thread would be + // fatal. if (sigsetjmp(crash_protection_ctx, 1) != 0) { - // checkFault() absorbed a fault raised somewhere in fillMethod() and jumped - // back here, bypassing the SIGNAL_HANDLER_GUARD() destructor in + // checkFault() absorbed a fault raised somewhere in fillMethod() and + // jumped back here, bypassing the SIGNAL_HANDLER_GUARD() destructor in // segvHandler()/busHandler(); compensate for it, then disarm before // touching anything else. SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); jmp_scope.restore(); - // Note: the counter cannot be reported accurately, see the comments in - // finishChunk() just above the writeCounters() call. Counters::increment(METHOD_RESOLVE_LONGJMP_RECOVERED); // A member, already filled above -- no map lookup, no allocation, and no // reliance on a local surviving siglongjmp (the value of a non-volatile @@ -664,6 +666,7 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { return &_unknown_method; } jmp_scope.install(&crash_protection_ctx); + return fillMethod(frame, method_id, bci); } @@ -672,19 +675,12 @@ MethodInfo *Lookup::fillMethod(ASGCT_CallFrame &frame, jmethodID method_id, // Resolve native method if (FrameType::isRawPointer(bci)) { method_id = JVMSupport::resolve(frame.method); + if (method_id == nullptr) { + return unknownMethod(); + } } - // Inject fault to test siglongjmp protection. Sits inside the window - // resolveMethod() arms around this function, which is the point: this is - // never compiled into a production build (it needs -PenableFaultInjection). - INJECT_CRASH_LIKELY(); - - // JVMSupport::resolve() above can yield null for a raw-pointer frame whose - // Method* no longer resolves; resolveMethod() screened out the null it was - // handed, but not this one. - if (method_id == nullptr) { - return unknownMethod(); - } + assert(method_id != nullptr && "Already filtered by caller"); // BCI_VTABLE_RECEIVER: method holds a VMSymbol* (see vmEntry.h). Resolve // to a class_id via the per-dump cache once, then key MethodMap by the diff --git a/ddprof-lib/src/main/cpp/guards.cpp b/ddprof-lib/src/main/cpp/guards.cpp index 942211b09d..fb710980e8 100644 --- a/ddprof-lib/src/main/cpp/guards.cpp +++ b/ddprof-lib/src/main/cpp/guards.cpp @@ -89,17 +89,6 @@ void signalHandlerUnwindAfterLongjmp() { } } -JmpCtxScope::JmpCtxScope(ProfiledThread *pt) : _pt(pt), _prev(pt->getJmpCtx()) { - assert(pt != nullptr); -} - -JmpCtxScope::~JmpCtxScope() { _pt->setJmpCtx(_prev); } - -void JmpCtxScope::install(sigjmp_buf *ctx) { _pt->setJmpCtx(ctx); } - -void JmpCtxScope::restore() { _pt->setJmpCtx(_prev); } - - CriticalSection::CriticalSection(ProfiledThread* pt) : _entered(false), _thread_ptr(pt) { // acquireCurrent() falls back to ThreadLocalDataPool::acquire() (a // pre-allocated, async-signal-safe pool) when the calling thread has @@ -120,3 +109,39 @@ CriticalSection::~CriticalSection() { _thread_ptr->exitCriticalSection(); } } + + +// Reads the currently installed landing pad, asserting the non-null contract +// *before* the pointer is dereferenced. This lives in a helper rather than the +// constructor body because the mem-initialiser for _prev runs first, so an +// assert in the body would only fire after the deref it is meant to guard. +static sigjmp_buf* prevJmpCtxOf(ProfiledThread* pt) { + assert(pt != nullptr); + return pt->getJmpCtx(); +} + +JmpCtxScope::JmpCtxScope(ProfiledThread* pt) : _pt(pt), _prev(prevJmpCtxOf(pt)) {} + +// Unconditional store, deliberately not guarded by an "already restored" flag; +// see restore(). +JmpCtxScope::~JmpCtxScope() { + restore(); +} + +void JmpCtxScope::install(sigjmp_buf* ctx) { + _pt->setJmpCtx(ctx); +} + +// Idempotent with the destructor by construction: _prev is const, so this is +// the same store every time it runs. No mutable "restored" flag is used -- and +// none may be added -- because this object is an automatic local of the frame +// that owns the sigjmp_buf, so any member mutated between sigsetjmp() and +// siglongjmp() would have an indeterminate value at the landing pad. +// +// Nesting stays correct without a flag: scopes are automatic objects, so +// construction/destruction is strictly LIFO and each constructor snapshots +// getJmpCtx() at its own construction time. An outer scope's destructor can +// therefore never clobber a context installed by an inner one. +void JmpCtxScope::restore() { + _pt->setJmpCtx(_prev); +} diff --git a/ddprof-lib/src/main/cpp/guards.h b/ddprof-lib/src/main/cpp/guards.h index 7c8c1d9d14..3889c04d24 100644 --- a/ddprof-lib/src/main/cpp/guards.h +++ b/ddprof-lib/src/main/cpp/guards.h @@ -139,56 +139,6 @@ class SignalHandlerScope { void signalHandlerUnwindAfterLongjmp(); #define SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP() signalHandlerUnwindAfterLongjmp() -// RAII for the per-thread siglongjmp landing pad (ProfiledThread::_jmp_buf) -// that Profiler::checkFault() jumps through. -// -// The previous landing pad must be reinstated on *every* exit from the frame -// that owns the sigjmp_buf -- normal return, a siglongjmp back into it, or an -// exception unwinding out of it -- because checkFault() will happily jump into -// a landing pad whose stack frame has already been popped. Hand-rolled -// "setJmpCtx(prev) before each return" only covers the returns the author -// remembered. -// -// Both members are const and initialised before the owning frame calls -// sigsetjmp(), and install()/restore() mutate only the ProfiledThread, so the -// guard's own state is never modified between sigsetjmp() and siglongjmp(). -// Reading it from the landing pad is therefore well defined -- unlike a plain -// non-volatile local, whose value after siglongjmp is indeterminate if it was -// assigned in the meantime. -// -// Usage: -// sigjmp_buf ctx; -// JmpCtxScope jmp_scope(prof_thread); // pt must be non-null -// if (sigsetjmp(ctx, 1) != 0) { // savemask=1: see note below -// SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); -// jmp_scope.restore(); // disarm before anything else -// return recovery_value; -// } -// jmp_scope.install(&ctx); -// ... risky work ... -// -// savemask must be 1: the siglongjmp originates inside the SIGSEGV handler, -// where the kernel has SIGSEGV blocked, so without restoring the saved mask the -// signal would stay blocked and the next fault on this thread would be fatal. -class JmpCtxScope { -public: - // `pt` must be non-null. - explicit JmpCtxScope(ProfiledThread* pt); - ~JmpCtxScope(); - // Publish `ctx` as this thread's landing pad; call after sigsetjmp() - // returns 0. - void install(sigjmp_buf* ctx); - // Reinstate the previous landing pad now. Idempotent with the destructor, - // so it is safe (and required) to call from the sigsetjmp landing pad - // before touching anything that could fault again. - void restore(); - JmpCtxScope(const JmpCtxScope&) = delete; - JmpCtxScope& operator=(const JmpCtxScope&) = delete; -private: - ProfiledThread* const _pt; - sigjmp_buf* const _prev; -}; - /** * Race-free critical section using atomic compare-and-swap. * @@ -244,6 +194,61 @@ class CriticalSection { bool entered() const { return _entered; } }; +// RAII for the per-thread siglongjmp landing pad (ProfiledThread::_jmp_buf) +// that Profiler::checkFault() jumps through. +// +// The previous landing pad must be reinstated on *every* exit from the frame +// that owns the sigjmp_buf -- normal return, a siglongjmp back into it, or an +// exception unwinding out of it -- because checkFault() will happily jump into +// a landing pad whose stack frame has already been popped. Hand-rolled +// "setJmpCtx(prev) before each return" only covers the returns the author +// remembered. +// +// Both members are const and initialised before the owning frame calls +// sigsetjmp(), and install()/restore() mutate only the ProfiledThread, so the +// guard's own state is never modified between sigsetjmp() and siglongjmp(). +// Reading it from the landing pad is therefore well defined -- unlike a plain +// non-volatile local, whose value after siglongjmp is indeterminate if it was +// assigned in the meantime. +// +// Usage: +// sigjmp_buf ctx; +// JmpCtxScope jmp_scope(prof_thread); // pt must be non-null +// if (sigsetjmp(ctx, 1) != 0) { // savemask=1: see note below +// SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); +// jmp_scope.restore(); // disarm before anything else +// return recovery_value; +// } +// jmp_scope.install(&ctx); +// ... risky work ... +// +// The one unsupported pattern: calling restore() and then installing a +// different sigjmp_buf in the same frame without a fresh JmpCtxScope. The +// guard only ever reinstates the context that was live when it was +// constructed, so use one scope per sigjmp_buf. +// +// savemask must be 1: the siglongjmp originates inside the SIGSEGV handler, +// where the kernel has SIGSEGV blocked, so without restoring the saved mask the +// signal would stay blocked and the next fault on this thread would be fatal. +class JmpCtxScope { +public: + // `pt` must be non-null. + explicit JmpCtxScope(ProfiledThread* pt); + ~JmpCtxScope(); + // Publish `ctx` as this thread's landing pad; call after sigsetjmp() + // returns 0. + void install(sigjmp_buf* ctx); + // Reinstate the previous landing pad now. Idempotent with the destructor, + // so it is safe (and required) to call from the sigsetjmp landing pad + // before touching anything that could fault again. + void restore(); + JmpCtxScope(const JmpCtxScope&) = delete; + JmpCtxScope& operator=(const JmpCtxScope&) = delete; +private: + ProfiledThread* const _pt; + sigjmp_buf* const _prev; +}; + /** * RAII guard to block profiling signals during critical operations. * diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 3a3d58b2bd..3354688963 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -1437,36 +1437,84 @@ bool HotspotSupport::loadMethodIDsIfNeededImpl(jvmtiEnv *jvmti, JNIEnv *jni, jcl return JVMSupport::loadMethodIDsImpl(jvmti, jni, klass); } -// This method only resolves methods that are loaded by system class loaders -jmethodID HotspotSupport::resolve(const void* method) { - assert(VM::isHotspot()); - NO_INJECTION_ASSERT(method != nullptr); - // fillJavaFrame stores the sentinel without the raw flag, so this should - // never reach the raw-pointer resolve path. Map it to nullptr so the dump - // thread serializes it as the shared unknown method. - if ((jmethodID)method == JMETHODID_NOT_WALKABLE) { - return nullptr; +// Fixed dump-time buffers, replacing the malloc/free trio this function used to +// carry. HotSpot's Symbol length field is a u2, so the VM permits up to 65535 +// bytes; these caps trade that theoretical maximum for zero heap traffic on the +// dump path and, more importantly, for having no cleanup obligation inside the +// crash-protected region -- a siglongjmp out of resolve() cannot leak anything. +// 4096 mirrors Lookup::resolveVTableReceiverCached (flightRecorder.cpp), which +// reads the same Symbol bodies on the same thread. A symbol over its cap reports +// METHOD_RESOLVE_SYMBOL_UNREADABLE and serializes as "unknown" -- the same +// outcome as a class FindClass cannot see. +static const size_t MAX_KLASS_NAME_LEN = 4096; // internal names; realistically < 256 +static const size_t MAX_METHOD_NAME_LEN = 512; // realistically < 64 +// A descriptor can in principle exceed this (255 argument *slots*, each able to +// carry an arbitrarily long L...; type name), so this cap is a fidelity choice, +// not a proof: over-cap descriptors serialize as "unknown" rather than being +// truncated. METHOD_RESOLVE_SYMBOL_UNREADABLE makes it visible if that bites. +static const size_t MAX_SIGNATURE_LEN = 4096; + +// Copies a Symbol's body into a fixed buffer, NUL-terminating it. +// Must be called with crash protection installed: length() and body() +// dereference the Symbol directly with no safefetch. +static bool copySymbolBody(VMSymbol* sym, char* dst, size_t cap) { + unsigned len = sym->length(); // raw u2 deref; PC stays inside this library + // A method name, descriptor or class name is never empty; 0 means the Symbol + // slot has been recycled. `>=` leaves room for the NUL. + if (len == 0 || len >= cap) { + return false; + } + const char* body = sym->body(); + if (!SafeAccess::safeCopy(dst, body, len)) { + return false; } + dst[len] = '\0'; + return true; +} + +// The three names resolve() needs, owned by resolve()'s frame so that the +// metadata walk has no cleanup obligation of its own. +struct ResolvedNames { + char method_name[MAX_METHOD_NAME_LEN]; + char method_signature[MAX_SIGNATURE_LEN]; + char klass_name[MAX_KLASS_NAME_LEN]; +}; + +// PHASE 1 -- the raw HotSpot metadata walk. MUST run with a jmp ctx installed: +// every step is a raw *(void**)(this + offset) whose target may have been freed +// by GC or class unloading since the sample was taken. Deliberately contains no +// JNI, no JVMTI and no allocation, so the whole protected region stays inside +// this library, where Profiler::checkFault() can actually recover. +// +// Returns false if the metadata is unusable. On success either *out_id holds an +// already-valid jmethodID (and `names` is untouched), or *out_id is null and +// `names` has been filled in for the JNI lookup the caller does afterwards. +static bool readMethodNames(const void* method, VMMethod** out_vm_method, + jmethodID* out_id, ResolvedNames* names) { + *out_vm_method = nullptr; + *out_id = nullptr; VMMethod* vm_method = VMMethod::cast_or_null(method); if (vm_method == nullptr) { - return nullptr; + return false; } + *out_vm_method = vm_method; // May have been populated by following code or JMETHODID_NOT_WALKABLE jmethodID method_id = vm_method->validatedId(); if (isValidJMethodID(method_id)) { - return method_id; + *out_id = method_id; + return true; } VMConstMethod* const_method = vm_method->constMethod_or_null(); if (const_method == nullptr) { - return nullptr; + return false; } VMConstantPool* const_pool = const_method->constants_or_null(); if (const_pool == nullptr) { - return nullptr; + return false; } VMSymbol* name_sym = const_method->name(); @@ -1474,66 +1522,148 @@ jmethodID HotspotSupport::resolve(const void* method) { VMKlass* klass = const_pool->holder_or_null(); if (name_sym == nullptr || sig_sym == nullptr || klass == nullptr) { - return nullptr; + return false; } VMSymbol* klass_sym = klass->name(); if (klass_sym == nullptr) { - return nullptr; + return false; } - method_id = nullptr; - char* method_name = (char*)malloc(name_sym->length() + 1); - char* method_signature = (char*)malloc(sig_sym->length() + 1); - int klass_name_len = klass_sym->length(); - char* klass_name = (char*)malloc(klass_name_len + 1); - if (method_name !=nullptr && method_signature != nullptr && klass_name != nullptr) { - memcpy(method_name, name_sym->body(), name_sym->length()); - method_name[name_sym->length()] = '\0'; - memcpy(method_signature, sig_sym->body(), sig_sym->length()); - method_signature[sig_sym->length()] = '\0'; - memcpy(klass_name, klass_sym->body(), klass_name_len); - klass_name[klass_name_len] = '\0'; - - JNIEnv *jni = VM::jni(); - jclass clz = jni->FindClass(klass_name); - if (clz == nullptr) { + if (!copySymbolBody(name_sym, names->method_name, sizeof(names->method_name)) || + !copySymbolBody(sig_sym, names->method_signature, sizeof(names->method_signature)) || + !copySymbolBody(klass_sym, names->klass_name, sizeof(names->klass_name))) { + Counters::increment(METHOD_RESOLVE_SYMBOL_UNREADABLE); + return false; + } + return true; +} + +// PHASE 2 -- the JNI/JVMTI lookup, reading only the buffers phase 1 filled. +// MUST run with crash protection *off*. Two reasons, both about siglongjmp +// unwinding frames it must not: +// - A fault inside libjvm.so is unrecoverable anyway (checkFault() only +// recovers PCs inside this library), so a landing pad buys nothing here. +// - FindClass() loads the class when it is not already loaded, which +// synchronously runs our own JVMTI ClassPrepare callback -> +// patchClassLoaderData(), which holds the JVM's ClassLoaderData mutex. That +// code *is* in this library, so with a pad installed a fault there would +// siglongjmp out of a JVMTI callback with a JVM lock held, an abandoned JNI +// local frame and unbalanced safepoint state -- trading a crash for a +// JVM-wide deadlock. +// vm_method->validatedId() below is safefetch-based, so it is safe unprotected. +static jmethodID lookupMethodIdViaJni(VMMethod* vm_method, const ResolvedNames& names) { + jmethodID method_id = nullptr; + const char* method_name = names.method_name; + const char* method_signature = names.method_signature; + const char* klass_name = names.klass_name; + + JNIEnv *jni = VM::jni(); + jclass clz = jni->FindClass(klass_name); + if (clz == nullptr) { + jni->ExceptionClear(); + } else { + method_id = jni->GetMethodID(clz, method_name, method_signature); + if (method_id == nullptr) { + jni->ExceptionClear(); + method_id = jni->GetStaticMethodID(clz, method_name, method_signature); + if (method_id == nullptr) { jni->ExceptionClear(); - } else { - method_id = jni->GetMethodID(clz, method_name, method_signature); - if (method_id == nullptr) { - jni->ExceptionClear(); - method_id = jni->GetStaticMethodID(clz, method_name, method_signature); - if (method_id == nullptr) { - jni->ExceptionClear(); - // JNI GetMethodID/GetStaticMethodID cannot look up because - // the JVM intentionally hides class initializers from JNI callers. - // Fall back to JVMTI GetClassMethods, which covers all methods - // including and forces jmethodID slot allocation for them. - // After the call, re-read the ID directly from VM metadata. - if (strcmp(method_name, "") == 0) { - jvmtiEnv* jvmti = VM::jvmti(); - if (jvmti != nullptr) { - jint count = 0; - jmethodID* methods = nullptr; - if (jvmti->GetClassMethods(clz, &count, &methods) == JVMTI_ERROR_NONE) { - jmethodID validated = vm_method->validatedId(); - if (isValidJMethodID(validated)) { - method_id = validated; - } - jvmti->Deallocate((unsigned char*)methods); - } + // JNI GetMethodID/GetStaticMethodID cannot look up because + // the JVM intentionally hides class initializers from JNI callers. + // Fall back to JVMTI GetClassMethods, which covers all methods + // including and forces jmethodID slot allocation for them. + // After the call, re-read the ID directly from VM metadata. + if (strcmp(method_name, "") == 0) { + jvmtiEnv* jvmti = VM::jvmti(); + if (jvmti != nullptr) { + jint count = 0; + jmethodID* methods = nullptr; + if (jvmti->GetClassMethods(clz, &count, &methods) == JVMTI_ERROR_NONE) { + jmethodID validated = vm_method->validatedId(); + if (isValidJMethodID(validated)) { + method_id = validated; } + jvmti->Deallocate((unsigned char*)methods); } } } - jni->DeleteLocalRef(clz); } + } + jni->DeleteLocalRef(clz); } - free(method_name); - free(method_signature); - free(klass_name); - return method_id; } + +// This method only resolves methods that are loaded by system class loaders +jmethodID HotspotSupport::resolve(const void* method) { + assert(VM::isHotspot()); + NO_INJECTION_ASSERT(method != nullptr); + // fillJavaFrame stores the sentinel without the raw flag, so this should + // never reach the raw-pointer resolve path. Map it to nullptr so the dump + // thread serializes it as the shared unknown method. + if ((jmethodID)method == JMETHODID_NOT_WALKABLE) { + return nullptr; + } + + // The Method* was captured at sample time; GC or class unloading may have + // freed the metadata since, so every dereference below can fault. Install a + // landing pad and report the method as unresolved instead of taking the JVM + // down mid-dump -- nullptr is already a first-class result for our caller + // (Lookup::resolveMethod serializes it as the shared unknown method). + // + // Runs on the JFR dump thread (Profiler::dump/stop), not in a signal handler. + // acquireCurrent() rather than current() because JNI_OnUnload reaches + // Profiler::stop() without priming TLS. + ProfiledThread* prof_thread = ProfiledThread::acquireCurrent(); + if (prof_thread == nullptr) { + // No landing pad available, so refuse to touch metadata that may be stale + // rather than risk crashing. Reached only on an unprimed shutdown path or + // when the thread-local pool is exhausted. + Counters::increment(SAMPLES_DROPPED_THREAD_LOCAL); + return nullptr; + } + + // Buffers live in this frame so phase 1 has nothing to clean up. None of the + // locals below are read on the recovery path, so none of them need to be + // volatile despite being assigned between sigsetjmp() and a possible + // siglongjmp. + ResolvedNames names; + VMMethod* vm_method = nullptr; + jmethodID existing_id = nullptr; + bool walked = false; + + { + sigjmp_buf crash_protection_ctx; + // Chained via JmpCtxScope: the dump thread can be interrupted by a sampling + // signal whose walkVM() installs its own context, so the previous landing + // pad must be reinstated on every exit path from this frame. + JmpCtxScope jmp_scope(prof_thread); + // savemask must be 1: the siglongjmp originates inside segvHandler, where + // the kernel has SIGSEGV blocked, so without restoring the saved mask the + // signal would stay blocked and the next fault on this thread would be + // fatal. + if (sigsetjmp(crash_protection_ctx, 1) != 0) { + // checkFault() does a siglongjmp from inside segvHandler, bypassing + // segvHandler's SignalHandlerScope destructor. Compensate, then disarm + // before touching anything that could fault again. + SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); + jmp_scope.restore(); + Counters::increment(METHOD_RESOLVE_FAULT_RECOVERED); + return nullptr; + } + jmp_scope.install(&crash_protection_ctx); + + walked = readMethodNames(method, &vm_method, &existing_id, &names); + } + // --- crash protection is off from here on; see lookupMethodIdViaJni() --- + + if (!walked) { + return nullptr; + } + if (existing_id != nullptr) { + return existing_id; + } + return lookupMethodIdViaJni(vm_method, names); +} diff --git a/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp b/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp index 14a44be06b..dbc4f25b0d 100644 --- a/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp +++ b/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp @@ -5,12 +5,23 @@ #include +#include "../../main/cpp/counters.h" #include "../../main/cpp/flightRecorder.h" #include "../../main/cpp/hotspot/hotspotSupport.h" #include "../../main/cpp/hotspot/vmStructs.h" +#include "../../main/cpp/os.h" +#include "../../main/cpp/profiler.h" +#include "../../main/cpp/threadLocalData.inline.h" +#include #include #include +#include + +#ifdef __linux__ +#include +#include +#endif // Test-only friend accessor for VM internals. It exists solely so this test // can exercise HotSpot's rejected-jmethodID handling. @@ -44,6 +55,70 @@ class VMStructsTestAccessor { ~VMStructsTestAccessor() { _restore(_saved); } + // Offsets beyond the id() set, needed to walk a fake Method* all the way to + // its name/signature/class Symbols the way HotspotSupport::resolve() does. + struct SymbolOffsets { + int constmethod_name_index; + int constmethod_sig_index; + int klass_name; + int symbol_length; + int symbol_body; + }; + + // cast_or_null() / VMConstantPool::base() need non-zero type sizes, which + // normally come from gHotSpotVMTypes and therefore stay 0 without a live + // JVM. Every field is asserted > 0 by the code under test, so tests that + // reach cast_or_null must set them. + struct TypeSizes { + uint64_t method; + uint64_t const_method; + uint64_t constant_pool; + uint64_t klass; + uint64_t symbol; + }; + + // Scoped override for the symbol-walk offsets and the type sizes, kept + // separate from the ctor above so the existing id() tests are unaffected. + class SymbolLayout { + public: + SymbolLayout(const SymbolOffsets& o, const TypeSizes& t) { + _saved_offsets = { + VMStructs::_constmethod_name_index_offset, + VMStructs::_constmethod_sig_index_offset, + VMStructs::_klass_name_offset, + VMStructs::_symbol_length_offset, + VMStructs::_symbol_body_offset, + }; + _saved_sizes = { + VMStructs::TYPE_SIZE_NAME(VMMethod), + VMStructs::TYPE_SIZE_NAME(VMConstMethod), + VMStructs::TYPE_SIZE_NAME(VMConstantPool), + VMStructs::TYPE_SIZE_NAME(VMKlass), + VMStructs::TYPE_SIZE_NAME(VMSymbol), + }; + _apply(o, t); + } + + ~SymbolLayout() { _apply(_saved_offsets, _saved_sizes); } + + private: + SymbolOffsets _saved_offsets; + TypeSizes _saved_sizes; + + static void _apply(const SymbolOffsets& o, const TypeSizes& t) { + VMStructs::_constmethod_name_index_offset = o.constmethod_name_index; + VMStructs::_constmethod_sig_index_offset = o.constmethod_sig_index; + VMStructs::_klass_name_offset = o.klass_name; + VMStructs::_symbol_length_offset = o.symbol_length; + VMStructs::_symbol_body_offset = o.symbol_body; + VMStructs::TYPE_SIZE_NAME(VMMethod) = t.method; + VMStructs::TYPE_SIZE_NAME(VMConstMethod) = t.const_method; + VMStructs::TYPE_SIZE_NAME(VMConstantPool) = t.constant_pool; + VMStructs::TYPE_SIZE_NAME(VMKlass) = t.klass; + VMStructs::TYPE_SIZE_NAME(VMSymbol) = t.symbol; + } + }; + private: struct SavedOffsets { int method_constmethod; @@ -242,3 +317,307 @@ TEST(HotspotMethodIdTest, IdReturnsValidIdForPopulatedSlot) { VMMethod* vm_method = reinterpret_cast(&md.method); EXPECT_EQ(vm_method->id(), expected); } + +#ifdef __linux__ + +// --------------------------------------------------------------------------- +// HotspotSupport::resolve() crash protection +// +// resolve() turns a raw Method* captured at sample time into a jmethodID on the +// JFR dump thread. GC or class unloading can free that metadata in between, so +// every dereference in the walk (constMethod -> constants -> name/signature -> +// holder -> klass->name, then Symbol length/body) can fault. resolve() installs +// a sigsetjmp landing pad via JmpCtxScope so Profiler::checkFault() recovers and +// resolve() reports the method as unresolved (nullptr) instead of taking the JVM +// down mid-dump. +// +// checkFault() gates recovery on the faulting pc lying inside the profiler +// library's address range. setupSignalHandlers() never runs in this gtest binary +// (the library sources are linked straight into the executable), so the range +// stays (0, 0) and checkFault takes its "not initialized" fallback, recovering +// unconditionally without ever evaluating the comparison. SetUp() therefore +// installs a fabricated range via the UNIT_TEST-only +// Profiler::setAddressRangeForTest(), anchored on two exported symbols from +// hotspotSupport.cpp so it brackets that translation unit's whole text -- +// including the file-static helpers, whose addresses a test cannot take. This +// covers the acceptance half of the gate only; the rejection half is already +// tested for real by +// StackWalkerCrashRecoveryTest.CheckFaultRejectsFaultOutsideProfilerRange. +// +// In a DEBUG build these tests additionally pin the crashProtectionActive() +// interaction (vmStructs.h:33-45): VMStructs::at() asserts +// `crashProtectionActive() || SafeAccess::isReadable(ptr)`, which without an +// installed jmp ctx raises SIGABRT -- uncatchable by crash protection -- so +// DEBUG builds used to die here where release survived. Passing in both +// gtestDebug_ and gtestRelease_ is the evidence that the two now converge. +// --------------------------------------------------------------------------- + +namespace { + +// Two adjacent pages; the second is PROT_NONE. Fake metadata lives at the start +// of the first, so an offset of kPageSize lands on the guard page. +constexpr size_t kPageSize = 4096; + +// Layout of the fake metadata used by the resolve() tests. Sizes are the values +// VMConstantPool::base() and cast_or_null() need; they only have to be non-zero +// and consistent with the struct layouts below. +constexpr VMStructsTestAccessor::TypeSizes RESOLVE_TYPE_SIZES = { + /*method*/ sizeof(void*), + /*const_method*/ 2 * sizeof(void*), + /*constant_pool*/ sizeof(void*), // base() = cpool + this, i.e. &symbols[0] + /*klass*/ sizeof(void*), + /*symbol*/ 8, +}; + +constexpr VMStructsTestAccessor::SymbolOffsets RESOLVE_SYMBOL_OFFSETS = { + /*constmethod_name_index*/ 8, + /*constmethod_sig_index*/ 10, + /*klass_name*/ 0, + /*symbol_length*/ 0, + /*symbol_body*/ 2, +}; + +// jmethod_ids deliberately points at a separate, always-null field rather than +// aliasing klass_name at offset 0: VMMethod::id() reads the jmethodID cache +// through that offset, and pointing it at the class-name Symbol would make id() +// read a length and a "jmethodID" out of the Symbol's body. That value passes +// isValidJMethodID(), so resolve() would early-return with garbage and never +// reach the symbol-copy code these tests are about. +constexpr VMStructsTestAccessor::Offsets RESOLVE_OFFSETS = { + /*method_constmethod*/ 0, + /*constmethod_constants*/ 0, + /*constmethod_idnum*/ 12, + /*pool_holder*/ 0, + /*jmethod_ids*/ sizeof(void*), +}; + +// Mirrors RESOLVE_*_OFFSETS above. `symbols` must directly follow `holder` +// because VMConstantPool::base() is `this + VMConstantPool::type_size()`. +struct ResolveFakes { + struct Method { + void* const_method; + } method; + struct ConstMethod { + void* cpool; + uint16_t name_index; + uint16_t sig_index; + uint16_t idnum; + uint16_t pad; + } const_method; + struct ConstantPool { + void* holder; + intptr_t symbols[4]; + } cpool; + struct Klass { + void* name_symbol; // klass_name offset 0 + void* jmethod_ids; // stays null so VMMethod::id() reports "no cache" + } klass; + struct Symbol { + uint16_t length; + char body[64]; + } name_sym, sig_sym, klass_sym; + + // Wires the chain up so resolve() walks Method -> ConstMethod -> ConstantPool + // -> {name, signature} Symbols and ConstantPool -> Klass -> class-name Symbol. + void link() { + method.const_method = &const_method; + const_method.cpool = &cpool; + const_method.name_index = 1; + const_method.sig_index = 2; + const_method.idnum = 0; + cpool.holder = &klass; + cpool.symbols[1] = (intptr_t)&name_sym; + cpool.symbols[2] = (intptr_t)&sig_sym; + klass.name_symbol = &klass_sym; + } + + static void setSymbol(Symbol& s, const char* text) { + s.length = (uint16_t)strlen(text); + memcpy(s.body, text, s.length); + } +}; + +} // namespace + +class HotspotResolveCrashProtectionTest : public ::testing::Test { +protected: + // Generous enough to bracket hotspotSupport.cpp's compiled text in any build + // config, while staying far below the distance to unrelated libraries. + static constexpr uintptr_t kRangeMargin = 512 * 1024; + + void SetUp() override { + ProfiledThread::initCurrentThread(); + _pt = ProfiledThread::current(); + ASSERT_NE(nullptr, _pt); + ASSERT_FALSE(_pt->isProtected()); + + _orig_segv = OS::replaceSigsegvHandler(Profiler::segvHandler); + _orig_bus = OS::replaceSigbusHandler(Profiler::busHandler); + + _region = mmap(nullptr, 2 * kPageSize, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + ASSERT_NE(MAP_FAILED, _region); + ASSERT_EQ(0, mprotect((char*)_region + kPageSize, kPageSize, PROT_NONE)); + + // Two exported symbols from hotspotSupport.cpp, far apart in that TU. + uintptr_t a = (uintptr_t)&HotspotSupport::resolve; + uintptr_t b = (uintptr_t)&HotspotSupport::walkJavaStack; + _range_lo = std::min(a, b) - kRangeMargin; + _range_hi = std::max(a, b) + kRangeMargin; + Profiler::setAddressRangeForTest(_range_lo, _range_hi); + } + + void TearDown() override { + Profiler::resetAddressRangeForTest(); + munmap(_region, 2 * kPageSize); + OS::replaceSigsegvHandler(_orig_segv); + OS::replaceSigbusHandler(_orig_bus); + ProfiledThread::release(); + } + + ProfiledThread* _pt = nullptr; + void* _region = nullptr; + SigAction _orig_segv = nullptr; + SigAction _orig_bus = nullptr; + uintptr_t _range_lo = 0; + uintptr_t _range_hi = 0; +}; + +// The core guarantee: a SIGSEGV on stale metadata inside resolve() comes back as +// nullptr ("unknown method") rather than killing the process. +// +// How this test fails if the protection is ever removed differs by config, so +// don't read a clean release run as the only evidence: in DEBUG the at() assert +// aborts, while in release neither orig_segvHandler nor OS::getSegvChainTarget() +// is set in a gtest binary, so segvHandler returns, the faulting instruction +// re-executes and the test hangs instead of failing. +TEST_F(HotspotResolveCrashProtectionTest, ResolveRecoversFromFaultInsteadOfCrashing) { + HotspotMethodIdVMHotspotGuard hotspot; + // method_constmethod = kPageSize puts the very first raw dereference of the + // walk on the guard page. cast_or_null() still succeeds beforehand because it + // only validates [ptr, ptr + VMMethod::type_size()), which stays on the + // readable page -- the fault comes from the offset, not the pointer. + VMStructsTestAccessor::Offsets faulting = RESOLVE_OFFSETS; + faulting.method_constmethod = (int)kPageSize; + VMStructsTestAccessor offsets(faulting); + VMStructsTestAccessor::SymbolLayout layout(RESOLVE_SYMBOL_OFFSETS, RESOLVE_TYPE_SIZES); + + long long before = Counters::getCounter(METHOD_RESOLVE_FAULT_RECOVERED); + + // VMMethod::id() reads the same offset through SafeAccess first, so it + // returns the sentinel rather than faulting; resolve() then falls through to + // constMethod_or_null(), whose raw *(void**) deref is what actually faults. + EXPECT_EQ(nullptr, HotspotSupport::resolve(_region)); + + EXPECT_EQ(before + 1, Counters::getCounter(METHOD_RESOLVE_FAULT_RECOVERED)); + // The landing pad must hand the previous (here: absent) context back. + EXPECT_FALSE(_pt->isProtected()); +} + +// Recovery must be repeatable: checkFault() calls resetCrashHandler() before +// jumping precisely because the siglongjmp skips exitCrashHandler(), so a run of +// faults must not exhaust CRASH_HANDLER_NESTING_LIMIT. +TEST_F(HotspotResolveCrashProtectionTest, ResolveRecoversRepeatedly) { + HotspotMethodIdVMHotspotGuard hotspot; + VMStructsTestAccessor::Offsets faulting = RESOLVE_OFFSETS; + faulting.method_constmethod = (int)kPageSize; + VMStructsTestAccessor offsets(faulting); + VMStructsTestAccessor::SymbolLayout layout(RESOLVE_SYMBOL_OFFSETS, RESOLVE_TYPE_SIZES); + + long long before = Counters::getCounter(METHOD_RESOLVE_FAULT_RECOVERED); + for (int i = 0; i < 10; i++) { + EXPECT_EQ(nullptr, HotspotSupport::resolve(_region)); + EXPECT_FALSE(_pt->isProtected()); + } + EXPECT_EQ(before + 10, Counters::getCounter(METHOD_RESOLVE_FAULT_RECOVERED)); +} + +// A Symbol whose body straddles the guard page is rejected by copySymbolBody's +// SafeAccess::isReadableRange() probe before memcpy() is reached -- a fault +// inside an out-of-line libc memcpy would have a pc outside the profiler range +// and so would NOT be recoverable. No JNI is reached on this path, which is why +// the test is safe without a live JVM. +TEST_F(HotspotResolveCrashProtectionTest, ResolveReturnsNullForUnreadableSymbolBody) { + HotspotMethodIdVMHotspotGuard hotspot; + VMStructsTestAccessor offsets(RESOLVE_OFFSETS); + VMStructsTestAccessor::SymbolLayout layout(RESOLVE_SYMBOL_OFFSETS, RESOLVE_TYPE_SIZES); + + ResolveFakes* f = new (_region) ResolveFakes{}; + f->link(); + ResolveFakes::setSymbol(f->name_sym, "someMethod"); + ResolveFakes::setSymbol(f->sig_sym, "()V"); + // Put the class-name Symbol's header flush against the end of the readable + // page: exactly VMSymbol::type_size() bytes, so VMSymbol::cast_or_null()'s + // own isReadableRange() check still passes and the rejection has to come from + // copySymbolBody(). The declared length then runs the body off the page. + char* edge = (char*)_region + kPageSize - RESOLVE_TYPE_SIZES.symbol; + *(uint16_t*)(edge + RESOLVE_SYMBOL_OFFSETS.symbol_length) = 100; + f->klass.name_symbol = edge; + + long long before = Counters::getCounter(METHOD_RESOLVE_SYMBOL_UNREADABLE); + long long faults_before = Counters::getCounter(METHOD_RESOLVE_FAULT_RECOVERED); + + EXPECT_EQ(nullptr, HotspotSupport::resolve(&f->method)); + + EXPECT_EQ(before + 1, Counters::getCounter(METHOD_RESOLVE_SYMBOL_UNREADABLE)); + // Rejected by the readability probe, not by recovering from a real fault. + EXPECT_EQ(faults_before, Counters::getCounter(METHOD_RESOLVE_FAULT_RECOVERED)); + EXPECT_FALSE(_pt->isProtected()); +} + +// A Symbol longer than the fixed dump-time buffer is reported as unresolved +// rather than truncated or copied out of bounds. Pins the cap behaviour so a +// future change to the buffer sizes is a deliberate test edit. +TEST_F(HotspotResolveCrashProtectionTest, ResolveReturnsNullForOverlongSymbol) { + HotspotMethodIdVMHotspotGuard hotspot; + VMStructsTestAccessor offsets(RESOLVE_OFFSETS); + VMStructsTestAccessor::SymbolLayout layout(RESOLVE_SYMBOL_OFFSETS, RESOLVE_TYPE_SIZES); + + ResolveFakes* f = new (_region) ResolveFakes{}; + f->link(); + ResolveFakes::setSymbol(f->sig_sym, "()V"); + ResolveFakes::setSymbol(f->klass_sym, "java/lang/Object"); + f->name_sym.length = 0xFFFF; // far above MAX_METHOD_NAME_LEN + + long long before = Counters::getCounter(METHOD_RESOLVE_SYMBOL_UNREADABLE); + + EXPECT_EQ(nullptr, HotspotSupport::resolve(&f->method)); + + EXPECT_EQ(before + 1, Counters::getCounter(METHOD_RESOLVE_SYMBOL_UNREADABLE)); + EXPECT_FALSE(_pt->isProtected()); +} + +// An empty Symbol means the slot has been recycled; treat it as unresolvable +// rather than handing FindClass an empty string. +TEST_F(HotspotResolveCrashProtectionTest, ResolveReturnsNullForEmptySymbol) { + HotspotMethodIdVMHotspotGuard hotspot; + VMStructsTestAccessor offsets(RESOLVE_OFFSETS); + VMStructsTestAccessor::SymbolLayout layout(RESOLVE_SYMBOL_OFFSETS, RESOLVE_TYPE_SIZES); + + ResolveFakes* f = new (_region) ResolveFakes{}; + f->link(); + ResolveFakes::setSymbol(f->sig_sym, "()V"); + ResolveFakes::setSymbol(f->klass_sym, "java/lang/Object"); + f->name_sym.length = 0; + + long long before = Counters::getCounter(METHOD_RESOLVE_SYMBOL_UNREADABLE); + + EXPECT_EQ(nullptr, HotspotSupport::resolve(&f->method)); + + EXPECT_EQ(before + 1, Counters::getCounter(METHOD_RESOLVE_SYMBOL_UNREADABLE)); + EXPECT_FALSE(_pt->isProtected()); +} + +// The sentinel is mapped to nullptr before any metadata is touched, so it must +// not acquire a ProfiledThread or install a landing pad at all. +TEST_F(HotspotResolveCrashProtectionTest, ResolveShortCircuitsSentinelWithoutProtection) { + HotspotMethodIdVMHotspotGuard hotspot; + long long before = Counters::getCounter(METHOD_RESOLVE_FAULT_RECOVERED); + + EXPECT_EQ(nullptr, HotspotSupport::resolve((const void*)JMETHODID_NOT_WALKABLE)); + + EXPECT_EQ(before, Counters::getCounter(METHOD_RESOLVE_FAULT_RECOVERED)); + EXPECT_FALSE(_pt->isProtected()); +} + +#endif // __linux__ 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 2131ecac04..cba11fad81 100644 --- a/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp +++ b/ddprof-lib/src/test/cpp/hotspot_crash_protection_ut.cpp @@ -25,6 +25,8 @@ * A. ProfiledThread thread-type classification (isJavaThread fast path) * B. Crash-handler nesting depth (ProfiledThread crash handler state) * C. sigjmp_buf chaining across nested/interrupted walkVM() calls + * C2. JmpCtxScope, the RAII form of that protocol (used by + * HotspotSupport::resolve()) * F. HotspotSupport::walkJavaStack()'s AsyncSampleMutex release on a * recovered fault */ @@ -34,6 +36,7 @@ #include "profiler.h" #include "asyncSampleMutex.h" +#include "guards.h" #include "jvmThread.h" #include "safeAccess.h" #include "os.h" @@ -327,6 +330,161 @@ TEST_F(JmpCtxChainingTest, FaultInInnerFrameDoesNotDisturbOuterFrame) { EXPECT_FALSE(_pt->isProtected()); } +// --------------------------------------------------------------------------- +// C2. JmpCtxScope — the RAII form of the protocol section C tests by hand. +// +// HotspotSupport::resolve() uses it; walkVM/walkJavaStack/walkFP/walkDwarf still +// hand-roll the same save/install/restore. These tests assert the guard +// reproduces that protocol store-for-store, which is what makes migrating those +// four call sites a mechanical change rather than a risky one. +// +// The class's members are both const by design (guards.h): they are initialised +// before the owning frame calls sigsetjmp() and are never written afterwards, so +// reading them from a landing pad is well defined -- unlike a plain non-volatile +// local assigned in between, whose value after siglongjmp is indeterminate. +// --------------------------------------------------------------------------- + +// Construction only snapshots the current landing pad; it must not arm anything. +TEST_F(JmpCtxChainingTest, JmpCtxScopeCtorDoesNotInstall) { + sigjmp_buf ctx; + { + JmpCtxScope scope(_pt); + EXPECT_FALSE(_pt->isProtected()); + EXPECT_EQ(nullptr, _pt->getJmpCtx()); + scope.install(&ctx); + EXPECT_EQ(&ctx, _pt->getJmpCtx()); + } + EXPECT_FALSE(_pt->isProtected()); +} + +// The whole point of the guard: scope exit reinstates the previous context even +// though no explicit restore() call was written. +TEST_F(JmpCtxChainingTest, JmpCtxScopeExitRestoresWithoutExplicitCall) { + sigjmp_buf outer; + _pt->setJmpCtx(&outer); + + { + sigjmp_buf inner; + JmpCtxScope scope(_pt); + scope.install(&inner); + EXPECT_EQ(&inner, _pt->getJmpCtx()); + } + + EXPECT_EQ(&outer, _pt->getJmpCtx()); + _pt->setJmpCtx(nullptr); +} + +// restore() and the destructor must be interchangeable and safe to run in +// sequence: _prev is const, so both make the identical store. +TEST_F(JmpCtxChainingTest, JmpCtxScopeRestoreThenDtorIsIdempotent) { + sigjmp_buf outer; + _pt->setJmpCtx(&outer); + + { + sigjmp_buf inner; + JmpCtxScope scope(_pt); + scope.install(&inner); + scope.restore(); + EXPECT_EQ(&outer, _pt->getJmpCtx()); + scope.restore(); // explicitly redundant + EXPECT_EQ(&outer, _pt->getJmpCtx()); + } // destructor makes the same store a third time + + EXPECT_EQ(&outer, _pt->getJmpCtx()); + _pt->setJmpCtx(nullptr); +} + +// Nested scopes must unwind strictly LIFO, each handing back exactly the context +// that was live when it was constructed. +TEST_F(JmpCtxChainingTest, JmpCtxScopeNestedScopesUnwindLIFO) { + sigjmp_buf outer_ctx; + sigjmp_buf inner_ctx; + + { + JmpCtxScope outer(_pt); + outer.install(&outer_ctx); + EXPECT_EQ(&outer_ctx, _pt->getJmpCtx()); + + { + JmpCtxScope inner(_pt); + inner.install(&inner_ctx); + EXPECT_EQ(&inner_ctx, _pt->getJmpCtx()); + } + + EXPECT_EQ(&outer_ctx, _pt->getJmpCtx()) + << "inner scope must hand the outer's context back"; + } + + EXPECT_FALSE(_pt->isProtected()); +} + +// Real sigsetjmp/siglongjmp: the RAII port of +// FaultInInnerFrameDoesNotDisturbOuterFrame above. +TEST_F(JmpCtxChainingTest, JmpCtxScopeFaultInInnerScopeDoesNotDisturbOuter) { + sigjmp_buf outer_ctx; + int outer_landed = 0; + int inner_landed = 0; + + JmpCtxScope outer_scope(_pt); + if (sigsetjmp(outer_ctx, 1) != 0) { + outer_landed++; + outer_scope.restore(); + } else { + outer_scope.install(&outer_ctx); + + // --- inner protected call, interrupted mid-flight by a fault --- + { + sigjmp_buf inner_ctx; + JmpCtxScope inner_scope(_pt); + if (sigsetjmp(inner_ctx, 1) != 0) { + inner_landed++; + inner_scope.restore(); + } else { + inner_scope.install(&inner_ctx); + // Simulate checkFault(): siglongjmp through whatever is + // installed. This must land in the inner frame, not the outer. + siglongjmp(*_pt->getJmpCtx(), 1); + FAIL() << "unreachable: siglongjmp does not return"; + } + } + + EXPECT_EQ(&outer_ctx, _pt->getJmpCtx()) + << "outer frame's context must survive the inner frame's fault"; + outer_scope.restore(); + } + + EXPECT_EQ(1, inner_landed); + EXPECT_EQ(0, outer_landed) << "the fault must not have unwound past the inner frame"; + EXPECT_FALSE(_pt->isProtected()); +} + +// The failure mode the guard exists to prevent: a landing pad that forgets to +// disarm. checkFault() would happily jump into a landing pad whose frame has +// already been popped, so the destructor must cover the omission. +TEST_F(JmpCtxChainingTest, JmpCtxScopeDestructorAloneRestoresAfterLongjmp) { + sigjmp_buf outer; + _pt->setJmpCtx(&outer); + int landed = 0; + + { + sigjmp_buf ctx; + JmpCtxScope scope(_pt); + if (sigsetjmp(ctx, 1) != 0) { + landed++; + // Deliberately no scope.restore() here. + } else { + scope.install(&ctx); + siglongjmp(*_pt->getJmpCtx(), 1); + FAIL() << "unreachable: siglongjmp does not return"; + } + } + + EXPECT_EQ(1, landed); + EXPECT_EQ(&outer, _pt->getJmpCtx()) + << "destructor must disarm even when the landing pad did not"; + _pt->setJmpCtx(nullptr); +} + // --------------------------------------------------------------------------- // D. Profiler::checkFault() guard clauses // From bd574498efc5bbd6b7eee16fa7baa6e475821a97 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 19 Aug 2026 16:36:02 +0000 Subject: [PATCH 07/19] fix: profiler looks at wrong fjmethodid flag --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 18 ++-- .../src/main/cpp/hotspot/hotspotSupport.h | 7 ++ ddprof-lib/src/main/cpp/jvmSupport.cpp | 4 - ddprof-lib/src/main/cpp/jvmSupport.h | 1 + ddprof-lib/src/main/cpp/profiler.cpp | 1 + ddprof-lib/src/main/cpp/profiler.h | 6 ++ ddprof-lib/src/test/cpp/hotspotSupport_ut.cpp | 75 +++++++++++++ ddprof-lib/src/test/cpp/jvmSupport_ut.cpp | 100 +++++++++++++++++- 8 files changed, 198 insertions(+), 14 deletions(-) create mode 100644 ddprof-lib/src/test/cpp/hotspotSupport_ut.cpp diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 3354688963..503d289062 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -202,7 +202,7 @@ void HotspotSupport::fillJavaFrame(ASGCT_CallFrame& frame, FrameTypeId type, int fillFrame(frame, type, bci, method_id); } else if (method_id != nullptr) { fillFrame(frame, type, bci, method_id); - } else if (!VM::arguments()._force_jmethodID) { + } else if (!Profiler::instance()->forceJmethodID()) { // fjmethodid=false: the user opted into the raw Method* path. nullptr // means no jmethodID is available — either the klass was deliberately // not primed (ids == NULL) or the cache was shrunk by a redefine @@ -1411,7 +1411,7 @@ bool HotspotSupport::loadMethodIDsIfNeededImpl(jvmtiEnv *jvmti, JNIEnv *jni, jcl jobject cl = nullptr; // Hidden/lambda classes can be unloaded, fallback to use jmethodIDs, so preload them. if (!isHiddenClass(jvmti, klass) && - jvmti->GetClassLoader(klass, &cl) == JVMTI_ERROR_NONE && + jvmti->GetClassLoader(klass, &cl) == JVMTI_ERROR_NONE && isSystemClassLoader(jni, cl)) { char* signature_ptr = nullptr; if (jvmti->GetClassSignature(klass, &signature_ptr, nullptr) == JVMTI_ERROR_NONE) { @@ -1552,7 +1552,7 @@ static bool readMethodNames(const void* method, VMMethod** out_vm_method, // local frame and unbalanced safepoint state -- trading a crash for a // JVM-wide deadlock. // vm_method->validatedId() below is safefetch-based, so it is safe unprotected. -static jmethodID lookupMethodIdViaJni(VMMethod* vm_method, const ResolvedNames& names) { +jmethodID lookupMethodIdViaJni(VMMethod* vm_method, const ResolvedNames& names) { jmethodID method_id = nullptr; const char* method_name = names.method_name; const char* method_signature = names.method_signature; @@ -1571,20 +1571,20 @@ static jmethodID lookupMethodIdViaJni(VMMethod* vm_method, const ResolvedNames& jni->ExceptionClear(); // JNI GetMethodID/GetStaticMethodID cannot look up because // the JVM intentionally hides class initializers from JNI callers. - // Fall back to JVMTI GetClassMethods, which covers all methods - // including and forces jmethodID slot allocation for them. + // Fall back to loadMethodIDsIfNeededImpl(), which covers all methods + // including and forces jmethodID slot allocation for them + // (going through this helper, rather than calling GetClassMethods + // directly, ensures the JDK-8062116 patchClassLoaderData() workaround + // is applied here too, same as every other jmethodID-preload path). // After the call, re-read the ID directly from VM metadata. if (strcmp(method_name, "") == 0) { jvmtiEnv* jvmti = VM::jvmti(); if (jvmti != nullptr) { - jint count = 0; - jmethodID* methods = nullptr; - if (jvmti->GetClassMethods(clz, &count, &methods) == JVMTI_ERROR_NONE) { + if (HotspotSupport::loadMethodIDsIfNeededImpl(jvmti, jni, clz, true /*load all*/)) { jmethodID validated = vm_method->validatedId(); if (isValidJMethodID(validated)) { method_id = validated; } - jvmti->Deallocate((unsigned char*)methods); } } } diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h index bbe241e047..7af722345c 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h @@ -18,9 +18,16 @@ class ProfiledThread; class VMMethod; +struct ResolvedNames; class HotspotSupport { friend class JVMSupport; + friend class HotspotSupportTestAccessor; + // lookupMethodIdViaJni() is a free function (not a HotspotSupport member) + // so that HotspotSupport::resolve() can install/tear down crash protection + // around it without exposing that split as public API; it still needs + // loadMethodIDsIfNeededImpl() for the fallback below. + friend jmethodID lookupMethodIdViaJni(VMMethod* vm_method, const ResolvedNames& names); private: static int walkVM(void* ucontext, ASGCT_CallFrame* frames, int max_depth, diff --git a/ddprof-lib/src/main/cpp/jvmSupport.cpp b/ddprof-lib/src/main/cpp/jvmSupport.cpp index cde4962e56..4dc2be966e 100644 --- a/ddprof-lib/src/main/cpp/jvmSupport.cpp +++ b/ddprof-lib/src/main/cpp/jvmSupport.cpp @@ -71,10 +71,6 @@ void JVMSupport::setLoadState(JMethodIDLoadStats state) { void JVMSupport::initExecution(Arguments& args, jvmtiEnv* jvmti, JNIEnv* jni) { JMethodIDLoadStats current_state = getLoadState(); - // Already setup by previous execution - if (current_state == Fully_loaded) { - return; - } bool load_all = true; if (VM::isHotspot()) { diff --git a/ddprof-lib/src/main/cpp/jvmSupport.h b/ddprof-lib/src/main/cpp/jvmSupport.h index 8d652fed80..49b8a9193f 100644 --- a/ddprof-lib/src/main/cpp/jvmSupport.h +++ b/ddprof-lib/src/main/cpp/jvmSupport.h @@ -31,6 +31,7 @@ class JVMSupport { }; friend class HotspotSupport; + friend class JVMSupportTestAccessor; static Mutex _initialization_lock; static volatile JMethodIDLoadStats jmethodID_load_state; diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp index 7b8e54bf3d..7f9f208bc0 100644 --- a/ddprof-lib/src/main/cpp/profiler.cpp +++ b/ddprof-lib/src/main/cpp/profiler.cpp @@ -1586,6 +1586,7 @@ Error Profiler::start(Arguments &args, bool reset) { _cpu_engine = selectCpuEngine(args); _wall_engine = selectWallEngine(args); _cstack = args._cstack; + _force_jmethodID = args._force_jmethodID; if (_cstack == CSTACK_DEFAULT) { if (VMStructs::hasStackStructs() && OS::isLinux()) { _cstack = CSTACK_VM; diff --git a/ddprof-lib/src/main/cpp/profiler.h b/ddprof-lib/src/main/cpp/profiler.h index df9c6cf53b..226c278678 100644 --- a/ddprof-lib/src/main/cpp/profiler.h +++ b/ddprof-lib/src/main/cpp/profiler.h @@ -141,6 +141,7 @@ class alignas(alignof(SpinLock)) Profiler { StackWalkFeatures _features; int _safe_mode; CStack _cstack; + bool _force_jmethodID; volatile jvmtiEventMode _thread_events_state; @@ -228,6 +229,7 @@ class alignas(alignof(SpinLock)) Profiler { _start_time(0), _stop_time(0), _epoch(0), _timer_id(NULL), _total_samples(0), _sample_seq(0), _failures(), _class_map_lock(), _max_stack_depth(0), _features(), _safe_mode(0), _cstack(CSTACK_NO), + _force_jmethodID(true), _thread_events_state(JVMTI_DISABLE), _libs(Libraries::instance()), _num_context_attributes(0), _omit_stacktraces(false), _remote_symbolication(false), _sanity_check_failed(false), @@ -279,6 +281,10 @@ class alignas(alignof(SpinLock)) Profiler { return _cstack; } + inline bool forceJmethodID() const { + return _force_jmethodID; + } + inline const StackWalkFeatures& stackWalkFeatures() const { return _features; } diff --git a/ddprof-lib/src/test/cpp/hotspotSupport_ut.cpp b/ddprof-lib/src/test/cpp/hotspotSupport_ut.cpp new file mode 100644 index 0000000000..42ba2d7176 --- /dev/null +++ b/ddprof-lib/src/test/cpp/hotspotSupport_ut.cpp @@ -0,0 +1,75 @@ +/* + * Copyright 2026, Datadog, Inc + */ + +#include +#include "../../main/cpp/hotspot/hotspotSupport.h" +#include "../../main/cpp/gtest_crash_handler.h" + +static constexpr char HOTSPOT_SUPPORT_TEST_NAME[] = "HotspotSupportTest"; +class HotspotSupportGlobalSetup { +public: + HotspotSupportGlobalSetup() { installGtestCrashHandler(); } + ~HotspotSupportGlobalSetup() { restoreDefaultSignalHandlers(); } +}; +static HotspotSupportGlobalSetup hotspot_support_global_setup; + +// --------------------------------------------------------------------------- +// HotspotSupportTestAccessor — friend of HotspotSupport, exposes the private +// loadMethodIDsIfNeededImpl() so the regression test for the +// resolve() fallback (which now routes through it, instead of calling +// jvmti->GetClassMethods directly) can exercise it without a live JVM. +// --------------------------------------------------------------------------- +class HotspotSupportTestAccessor { +public: + static bool loadMethodIDsIfNeededImpl(jvmtiEnv *jvmti, JNIEnv *jni, jclass klass, bool load_all) { + return HotspotSupport::loadMethodIDsIfNeededImpl(jvmti, jni, klass, load_all); + } +}; + +// --------------------------------------------------------------------------- +// Mock JVMTI infrastructure. With load_all=true, loadMethodIDsIfNeededImpl() +// skips its hidden-class/system-classloader checks entirely and calls +// patchClassLoaderData() (a no-op here, since VM::hotspot_version() defaults +// to 0 -- not 8 -- in this gtest binary with no live JVM attached) followed +// by JVMSupport::loadMethodIDsImpl(), whose only JVMTI calls are +// GetClassMethods and Deallocate. +// --------------------------------------------------------------------------- +static int g_get_class_methods_calls = 0; + +static jvmtiError JNICALL mock_GetClassMethods_ok(jvmtiEnv*, jclass, jint* method_count_ptr, jmethodID** methods_ptr) { + g_get_class_methods_calls++; + *method_count_ptr = 0; + *methods_ptr = nullptr; + return JVMTI_ERROR_NONE; +} +static jvmtiError JNICALL mock_Deallocate_noop(jvmtiEnv*, unsigned char*) { + return JVMTI_ERROR_NONE; +} + +class HotspotSupportLoadMethodIDsTest : public ::testing::Test { +protected: + jvmtiInterface_1_ tbl{}; + _jvmtiEnv mock_jvmti{}; + + void SetUp() override { + g_get_class_methods_calls = 0; + tbl = jvmtiInterface_1_{}; + tbl.GetClassMethods = &mock_GetClassMethods_ok; + tbl.Deallocate = &mock_Deallocate_noop; + mock_jvmti.functions = &tbl; + } +}; + +TEST_F(HotspotSupportLoadMethodIDsTest, LoadAllSucceedsAndCallsGetClassMethodsOnce) { + jclass fake_klass = reinterpret_cast(0x1); + + bool result = HotspotSupportTestAccessor::loadMethodIDsIfNeededImpl( + &mock_jvmti, /*jni=*/nullptr, fake_klass, /*load_all=*/true); + + EXPECT_TRUE(result); + EXPECT_EQ(1, g_get_class_methods_calls) + << "the resolve() fallback must go through this exact path " + "(load_all=true), which is what now applies patchClassLoaderData() " + "before allocating jmethodIDs"; +} diff --git a/ddprof-lib/src/test/cpp/jvmSupport_ut.cpp b/ddprof-lib/src/test/cpp/jvmSupport_ut.cpp index c203d296e3..98e375135d 100644 --- a/ddprof-lib/src/test/cpp/jvmSupport_ut.cpp +++ b/ddprof-lib/src/test/cpp/jvmSupport_ut.cpp @@ -40,12 +40,31 @@ static JvmSupportGlobalSetup jvm_support_global_setup; // --------------------------------------------------------------------------- // VMTestAccessor — friend of VM, lets tests swap VM::_jvmti for a mock so -// JVMThread::currentThreadSlow() can be exercised without a live JVM. +// JVMThread::currentThreadSlow() can be exercised without a live JVM. Also +// lets tests force VM::isHotspot() so JVMSupport::initExecution()'s HotSpot +// branch can be exercised deterministically. // --------------------------------------------------------------------------- class VMTestAccessor { public: static jvmtiEnv* getJvmti() { return VM::_jvmti; } static void setJvmti(jvmtiEnv* env) { VM::_jvmti = env; } + static bool getHotspot() { return VM::_hotspot; } + static void setHotspot(bool v) { VM::_hotspot = v; } +}; + +// --------------------------------------------------------------------------- +// JVMSupportTestAccessor — friend of JVMSupport, lets tests read/reset the +// private jmethodID_load_state directly instead of only observing its +// effects through initExecution()'s side effects. +// --------------------------------------------------------------------------- +class JVMSupportTestAccessor { +public: + using LoadState = JVMSupport::JMethodIDLoadStats; + static LoadState getLoadState() { return JVMSupport::getLoadState(); } + static void setLoadState(LoadState s) { JVMSupport::setLoadState(s); } + static constexpr LoadState NoLoaded() { return JVMSupport::No_loaded; } + static constexpr LoadState PartialLoaded() { return JVMSupport::Partial_loaded; } + static constexpr LoadState FullyLoaded() { return JVMSupport::Fully_loaded; } }; // --------------------------------------------------------------------------- @@ -193,3 +212,82 @@ TEST(JvmSupportErrorLatchTest, CheckStateStaysBlockedOnceInError) { EXPECT_STREQ("Profiler encountered fatal error", error.message()); EXPECT_EQ(ERROR, ProfilerTestAccessor::getState(p)); } + +// --------------------------------------------------------------------------- +// Regression tests for JVMSupport::initExecution(): a stale jmethodID_load_state +// left over from a previous execution must not short-circuit before the +// current Arguments are evaluated (otherwise a restart with fjmethodid=false +// after a full-preload session silently keeps full-preload mode). +// +// initExecution() with load_all=false calls HotspotSupport::initClassloaderInfo(jni), +// which calls jni->FindClass("java/lang/ClassLoader") first; mocking FindClass +// to return nullptr makes it take the early-exit path (jni->ExceptionClear(); return;) +// without touching any other JNI/VMStructs machinery. +// --------------------------------------------------------------------------- +static jclass JNICALL mock_FindClass_returns_null(JNIEnv*, const char*) { + return nullptr; +} +static void JNICALL mock_ExceptionClear_noop(JNIEnv*) {} + +static jvmtiError JNICALL mock_GetLoadedClasses_empty(jvmtiEnv*, jint* class_count_ptr, jclass** classes_ptr) { + *class_count_ptr = 0; + *classes_ptr = nullptr; + return JVMTI_ERROR_NONE; +} +static jvmtiError JNICALL mock_GetClassMethods_empty(jvmtiEnv*, jclass, jint* method_count_ptr, jmethodID** methods_ptr) { + *method_count_ptr = 0; + *methods_ptr = nullptr; + return JVMTI_ERROR_NONE; +} +static jvmtiError JNICALL mock_Deallocate_noop(jvmtiEnv*, unsigned char*) { + return JVMTI_ERROR_NONE; +} + +class JVMSupportRestartTest : public ::testing::Test { +protected: + jvmtiInterface_1_ jvmti_tbl{}; + _jvmtiEnv mock_jvmti{}; + JNINativeInterface_ jni_tbl{}; + JNIEnv_ mock_jni{}; + bool _orig_hotspot; + + void SetUp() override { + _orig_hotspot = VMTestAccessor::getHotspot(); + VMTestAccessor::setHotspot(true); + JVMSupportTestAccessor::setLoadState(JVMSupportTestAccessor::NoLoaded()); + + jvmti_tbl = jvmtiInterface_1_{}; + jvmti_tbl.GetLoadedClasses = &mock_GetLoadedClasses_empty; + jvmti_tbl.GetClassMethods = &mock_GetClassMethods_empty; + jvmti_tbl.Deallocate = &mock_Deallocate_noop; + mock_jvmti.functions = &jvmti_tbl; + + jni_tbl = JNINativeInterface_{}; + jni_tbl.FindClass = &mock_FindClass_returns_null; + jni_tbl.ExceptionClear = &mock_ExceptionClear_noop; + mock_jni.functions = &jni_tbl; + } + + void TearDown() override { + VMTestAccessor::setHotspot(_orig_hotspot); + } +}; + +TEST_F(JVMSupportRestartTest, SecondStartWithPartialPreloadIsNotBlockedByStaleFullyLoaded) { + Arguments full_args; + full_args._force_jmethodID = true; // -> shouldPreloadJmethodIDs()==true -> Fully_loaded + + JVMSupport::initExecution(full_args, &mock_jvmti, reinterpret_cast(&mock_jni)); + EXPECT_EQ(JVMSupportTestAccessor::FullyLoaded(), JVMSupportTestAccessor::getLoadState()); + + Arguments partial_args; + partial_args._force_jmethodID = false; + partial_args._cstack = CSTACK_VM; // -> shouldPreloadJmethodIDs()==false -> Partial_loaded + + JVMSupport::initExecution(partial_args, &mock_jvmti, reinterpret_cast(&mock_jni)); + + // Fails today: the stale Fully_loaded state short-circuits initExecution() + // before shouldPreloadJmethodIDs(partial_args) is ever evaluated, so the + // state incorrectly stays Fully_loaded instead of downgrading. + EXPECT_EQ(JVMSupportTestAccessor::PartialLoaded(), JVMSupportTestAccessor::getLoadState()); +} From 112db50d72cfdf6e77a07486a2d91e73fa0f89b9 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 19 Aug 2026 23:51:28 +0000 Subject: [PATCH 08/19] Fixed buffers backed my malloc --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 165 ++++++++++++++---- 1 file changed, 128 insertions(+), 37 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 503d289062..e056c78e3c 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -1437,15 +1437,20 @@ bool HotspotSupport::loadMethodIDsIfNeededImpl(jvmtiEnv *jvmti, JNIEnv *jni, jcl return JVMSupport::loadMethodIDsImpl(jvmti, jni, klass); } -// Fixed dump-time buffers, replacing the malloc/free trio this function used to -// carry. HotSpot's Symbol length field is a u2, so the VM permits up to 65535 -// bytes; these caps trade that theoretical maximum for zero heap traffic on the -// dump path and, more importantly, for having no cleanup obligation inside the -// crash-protected region -- a siglongjmp out of resolve() cannot leak anything. -// 4096 mirrors Lookup::resolveVTableReceiverCached (flightRecorder.cpp), which -// reads the same Symbol bodies on the same thread. A symbol over its cap reports -// METHOD_RESOLVE_SYMBOL_UNREADABLE and serializes as "unknown" -- the same -// outcome as a class FindClass cannot see. +// Fixed-size fast-path buffers for the common case, sized so that a +// realistic name/signature/class name never needs to allocate. 4096 mirrors +// Lookup::resolveVTableReceiverCached (flightRecorder.cpp), which reads the +// same Symbol bodies on the same thread; it is a precedent to stay +// consistent with, not a JVM-spec limit. +// HotSpot's Symbol length field is a u2, so the VM permits up to 65535 bytes. +// A name/descriptor that overflows its fixed buffer falls back to malloc +// (see ResolvedNames::setImpl), up to MAX_SYMBOL_LEN below; beyond that it is +// rejected and the frame serializes as "unknown" -- METHOD_RESOLVE_SYMBOL_UNREADABLE +// makes that visible if it bites. Unlike the old stack-only design, the malloc +// fallback means this is no longer a leak-proof region: siglongjmp out of +// resolve() bypasses ~ResolvedNames(), so the fault-recovery path must call +// ResolvedNames::release() explicitly (see resolve()) to free any buffer +// allocated before the fault. static const size_t MAX_KLASS_NAME_LEN = 4096; // internal names; realistically < 256 static const size_t MAX_METHOD_NAME_LEN = 512; // realistically < 64 // A descriptor can in principle exceed this (255 argument *slots*, each able to @@ -1454,37 +1459,124 @@ static const size_t MAX_METHOD_NAME_LEN = 512; // realistically < 64 // truncated. METHOD_RESOLVE_SYMBOL_UNREADABLE makes it visible if that bites. static const size_t MAX_SIGNATURE_LEN = 4096; -// Copies a Symbol's body into a fixed buffer, NUL-terminating it. -// Must be called with crash protection installed: length() and body() -// dereference the Symbol directly with no safefetch. -static bool copySymbolBody(VMSymbol* sym, char* dst, size_t cap) { +// The three names resolve() needs, owned by resolve()'s frame. Each name has +// a fixed-size inline buffer for the common case, with a malloc'd fallback +// (up to MAX_SYMBOL_LEN) for names that don't fit -- see release() for why +// that fallback needs explicit cleanup on the crash-recovery path. +class ResolvedNames { + // Hard ceiling for the malloc fallback in setImpl(), independent of which + // field is being resolved. Not a JVM/class-file limit (Symbol::length() is + // a u2, so up to 65535 is legal) -- chosen to match MAX_KLASS_NAME_LEN and + // MAX_SIGNATURE_LEN above, so a name that would already have been rejected + // as too long for those fields' fixed buffers doesn't get an unbounded + // allocation just because it went through the malloc path instead. + static constexpr size_t MAX_SYMBOL_LEN = 4 * 1024; +private: + char* _long_method_name; + char* _long_method_signature; + char* _long_klass_name; + + char _method_name[MAX_METHOD_NAME_LEN]; + char _method_signature[MAX_SIGNATURE_LEN]; + char _klass_name[MAX_KLASS_NAME_LEN]; + + bool setImpl(char* short_name, char*& long_name, size_t short_limit, VMSymbol* sym); +public: + ResolvedNames(); + ~ResolvedNames(); + void release(); + + bool setMethodName(VMSymbol* sym); + bool setMethodSignature(VMSymbol* sym); + bool setKlassName(VMSymbol* sym); + + const char* methodName() const { + return _long_method_name != nullptr ? _long_method_name : _method_name; + } + + const char* methodSignature() const { + return _long_method_signature != nullptr ? _long_method_signature : _method_signature; + } + + const char* klassName() const { + return _long_klass_name != nullptr ? _long_klass_name : _klass_name; + } +}; + +ResolvedNames::ResolvedNames() : + _long_method_name(nullptr), + _long_method_signature(nullptr), + _long_klass_name(nullptr) { +} + +ResolvedNames::~ResolvedNames() { + release(); +} + +void ResolvedNames::release() { + // Must be idempotent: resolve()'s fault-recovery path calls this explicitly + // (siglongjmp bypasses ~ResolvedNames()), and then the destructor runs it + // again on the same object when resolve() returns normally afterward. + // Nulling out each pointer after freeing makes the second call a no-op + // instead of a double free. + if (_long_method_name != nullptr) { + free(_long_method_name); + _long_method_name = nullptr; + } + if (_long_method_signature != nullptr) { + free(_long_method_signature); + _long_method_signature = nullptr; + } + if (_long_klass_name != nullptr) { + free(_long_klass_name); + _long_klass_name = nullptr; + } +} + +bool ResolvedNames::setImpl(char* short_name, char*& long_name, size_t short_limit, VMSymbol* sym) { unsigned len = sym->length(); // raw u2 deref; PC stays inside this library // A method name, descriptor or class name is never empty; 0 means the Symbol // slot has been recycled. `>=` leaves room for the NUL. - if (len == 0 || len >= cap) { + if (len == 0 || len >= MAX_SYMBOL_LEN) { return false; } - const char* body = sym->body(); - if (!SafeAccess::safeCopy(dst, body, len)) { + + char* dest = short_name; + if (len >= short_limit) { + long_name = (char*)malloc(len + 1); + dest = long_name; + } + + if (SafeAccess::safeCopy(dest, sym->body(), len)) { + dest[len] = '\0'; + return true; + } else { return false; } - dst[len] = '\0'; - return true; } -// The three names resolve() needs, owned by resolve()'s frame so that the -// metadata walk has no cleanup obligation of its own. -struct ResolvedNames { - char method_name[MAX_METHOD_NAME_LEN]; - char method_signature[MAX_SIGNATURE_LEN]; - char klass_name[MAX_KLASS_NAME_LEN]; -}; +bool ResolvedNames::setMethodName(VMSymbol* sym) { + return setImpl(_method_name, _long_method_name, MAX_METHOD_NAME_LEN, sym); + +} +bool ResolvedNames::setMethodSignature(VMSymbol* sym) { + return setImpl(_method_signature, _long_method_signature, MAX_SIGNATURE_LEN, sym); +} + +bool ResolvedNames::setKlassName(VMSymbol* sym) { + return setImpl(_klass_name, _long_klass_name, MAX_KLASS_NAME_LEN, sym); +} + // PHASE 1 -- the raw HotSpot metadata walk. MUST run with a jmp ctx installed: // every step is a raw *(void**)(this + offset) whose target may have been freed // by GC or class unloading since the sample was taken. Deliberately contains no -// JNI, no JVMTI and no allocation, so the whole protected region stays inside -// this library, where Profiler::checkFault() can actually recover. +// JNI and no JVMTI, so the whole protected region stays inside this library, +// where Profiler::checkFault() can actually recover. It can allocate, via +// ResolvedNames::setImpl()'s malloc fallback for over-sized names -- that +// allocation is self-contained (glibc malloc doesn't call back into JNI/JVMTI), +// but it does mean a fault after the allocation needs explicit freeing, since +// siglongjmp out of this scope bypasses ~ResolvedNames() (see resolve()). // // Returns false if the metadata is unusable. On success either *out_id holds an // already-valid jmethodID (and `names` is untouched), or *out_id is null and @@ -1530,9 +1622,7 @@ static bool readMethodNames(const void* method, VMMethod** out_vm_method, return false; } - if (!copySymbolBody(name_sym, names->method_name, sizeof(names->method_name)) || - !copySymbolBody(sig_sym, names->method_signature, sizeof(names->method_signature)) || - !copySymbolBody(klass_sym, names->klass_name, sizeof(names->klass_name))) { + if (!names->setMethodName(name_sym) || !names->setMethodSignature(sig_sym) || !names->setKlassName(klass_sym)) { Counters::increment(METHOD_RESOLVE_SYMBOL_UNREADABLE); return false; } @@ -1554,9 +1644,9 @@ static bool readMethodNames(const void* method, VMMethod** out_vm_method, // vm_method->validatedId() below is safefetch-based, so it is safe unprotected. jmethodID lookupMethodIdViaJni(VMMethod* vm_method, const ResolvedNames& names) { jmethodID method_id = nullptr; - const char* method_name = names.method_name; - const char* method_signature = names.method_signature; - const char* klass_name = names.klass_name; + const char* method_name = names.methodName(); + const char* method_signature = names.methodSignature(); + const char* klass_name = names.klassName(); JNIEnv *jni = VM::jni(); jclass clz = jni->FindClass(klass_name); @@ -1625,10 +1715,6 @@ jmethodID HotspotSupport::resolve(const void* method) { return nullptr; } - // Buffers live in this frame so phase 1 has nothing to clean up. None of the - // locals below are read on the recovery path, so none of them need to be - // volatile despite being assigned between sigsetjmp() and a possible - // siglongjmp. ResolvedNames names; VMMethod* vm_method = nullptr; jmethodID existing_id = nullptr; @@ -1650,6 +1736,11 @@ jmethodID HotspotSupport::resolve(const void* method) { // before touching anything that could fault again. SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); jmp_scope.restore(); + // siglongjmp bypasses ~ResolvedNames(), so a name that was already + // malloc'd (in setImpl()'s over-sized-name fallback) before the fault + // would otherwise leak. release() is idempotent, so it's safe that the + // destructor also runs it when resolve() returns below. + names.release(); Counters::increment(METHOD_RESOLVE_FAULT_RECOVERED); return nullptr; } From 3b70dfb0cc48686407fed2510d40d38ad4cc11bd Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Thu, 20 Aug 2026 00:47:09 +0000 Subject: [PATCH 09/19] Fix --- ddprof-lib/src/main/cpp/guards.cpp | 2 +- .../src/main/cpp/hotspot/hotspotSupport.cpp | 21 ++++++++++++------- .../src/main/cpp/hotspot/hotspotSupport.h | 2 +- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/ddprof-lib/src/main/cpp/guards.cpp b/ddprof-lib/src/main/cpp/guards.cpp index fb710980e8..504b8386cc 100644 --- a/ddprof-lib/src/main/cpp/guards.cpp +++ b/ddprof-lib/src/main/cpp/guards.cpp @@ -125,7 +125,7 @@ JmpCtxScope::JmpCtxScope(ProfiledThread* pt) : _pt(pt), _prev(prevJmpCtxOf(pt)) // Unconditional store, deliberately not guarded by an "already restored" flag; // see restore(). JmpCtxScope::~JmpCtxScope() { - restore(); + restore(); } void JmpCtxScope::install(sigjmp_buf* ctx) { diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index e056c78e3c..49ebed46be 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -1451,13 +1451,13 @@ bool HotspotSupport::loadMethodIDsIfNeededImpl(jvmtiEnv *jvmti, JNIEnv *jni, jcl // resolve() bypasses ~ResolvedNames(), so the fault-recovery path must call // ResolvedNames::release() explicitly (see resolve()) to free any buffer // allocated before the fault. -static const size_t MAX_KLASS_NAME_LEN = 4096; // internal names; realistically < 256 +static const size_t MAX_KLASS_NAME_LEN = 1024; // internal names; realistically < 256 static const size_t MAX_METHOD_NAME_LEN = 512; // realistically < 64 // A descriptor can in principle exceed this (255 argument *slots*, each able to // carry an arbitrarily long L...; type name), so this cap is a fidelity choice, // not a proof: over-cap descriptors serialize as "unknown" rather than being // truncated. METHOD_RESOLVE_SYMBOL_UNREADABLE makes it visible if that bites. -static const size_t MAX_SIGNATURE_LEN = 4096; +static const size_t MAX_SIGNATURE_LEN = 1024; // The three names resolve() needs, owned by resolve()'s frame. Each name has // a fixed-size inline buffer for the common case, with a malloc'd fallback @@ -1472,15 +1472,22 @@ class ResolvedNames { // allocation just because it went through the malloc path instead. static constexpr size_t MAX_SYMBOL_LEN = 4 * 1024; private: - char* _long_method_name; - char* _long_method_signature; - char* _long_klass_name; + // volatile: resolve() mutates these (via setMethodName/setMethodSignature/ + // setKlassName, called through readMethodNames()) between sigsetjmp() and a + // possible siglongjmp() out of a fault, then release() reads them back at + // the landing pad to decide what to free. Per the setjmp/longjmp rules + // (C11 7.13.2.1p3, inherited by C++), a non-volatile automatic local + // modified in that window has an indeterminate value after longjmp; + // volatile is what makes release()'s reads on the recovery path defined. + char* volatile _long_method_name; + char* volatile _long_method_signature; + char* volatile _long_klass_name; char _method_name[MAX_METHOD_NAME_LEN]; char _method_signature[MAX_SIGNATURE_LEN]; char _klass_name[MAX_KLASS_NAME_LEN]; - bool setImpl(char* short_name, char*& long_name, size_t short_limit, VMSymbol* sym); + bool setImpl(char* short_name, char* volatile& long_name, size_t short_limit, VMSymbol* sym); public: ResolvedNames(); ~ResolvedNames(); @@ -1533,7 +1540,7 @@ void ResolvedNames::release() { } } -bool ResolvedNames::setImpl(char* short_name, char*& long_name, size_t short_limit, VMSymbol* sym) { +bool ResolvedNames::setImpl(char* short_name, char* volatile& long_name, size_t short_limit, VMSymbol* sym) { unsigned len = sym->length(); // raw u2 deref; PC stays inside this library // A method name, descriptor or class name is never empty; 0 means the Symbol // slot has been recycled. `>=` leaves room for the NUL. diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h index 7af722345c..f2822d3573 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h @@ -18,7 +18,7 @@ class ProfiledThread; class VMMethod; -struct ResolvedNames; +class ResolvedNames; class HotspotSupport { friend class JVMSupport; From 1a94faecabccef09285f4857299a366b0dc5524f Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 19 Aug 2026 16:21:24 +0200 Subject: [PATCH 10/19] Test case: for verifying main(String[]), run() method of Thread and its subclasses are marked entry frames (#742) --- .../datadoghq/profiler/ExternalLauncher.java | 92 +++++++++ .../com/datadoghq/profiler/JfrStackTrace.java | 5 +- .../profiler/jfr/EntryFrameTest.java | 178 ++++++++++++++++++ 3 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 ddprof-test/src/test/java/com/datadoghq/profiler/jfr/EntryFrameTest.java diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/ExternalLauncher.java b/ddprof-test/src/test/java/com/datadoghq/profiler/ExternalLauncher.java index cb83816df5..2dbb429668 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/ExternalLauncher.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/ExternalLauncher.java @@ -26,6 +26,10 @@ *

  • profiler-sequence [';'-delimited steps] - runs a sequence of start/stop calls in this * process; each step is either the literal {@code STOP} (calls {@link JavaProfiler#stop()}) * or a comma delimited profiler command list (calls {@link JavaProfiler#execute(String)})
    • + *
  • entry-frames <comma delimited profiler command list, required> - starts the profiler, then burns + * CPU concurrently on the main thread, on a plain {@code new Thread(Runnable)} and on a + * two-level {@link Thread} subclass, and stops the profiler again. The resulting recording + * holds samples rooted at each of the three thread entry points; see {@code EntryFrameTest}
    • * */ public class ExternalLauncher { @@ -41,6 +45,80 @@ private static Thread startVirtualThread(Runnable task) throws Exception { return (Thread) start.invoke(builder, task); } + /** How long each {@code entry-frames} thread burns CPU for. */ + private static final long ENTRY_FRAME_WORKLOAD_MILLIS = 1000; + + private static volatile long entryFrameSink; + + /** + * A {@link Thread} subclass that does not override {@code run()}, so that + * {@link EntryFrameThread} below sits two levels below {@link Thread} and its {@code run()} + * frame can only be recognised as a thread entry point by walking the whole superclass chain. + */ + private static class BaseEntryFrameThread extends Thread { + BaseEntryFrameThread(String name) { + super(name); + } + } + + private static final class EntryFrameThread extends BaseEntryFrameThread { + EntryFrameThread() { + super("entry-frame-subclass"); + } + + @Override + public void run() { + entryFrameWorkLevel1(ENTRY_FRAME_WORKLOAD_MILLIS); + } + } + + private static final class EntryFrameRunnable implements Runnable { + @Override + public void run() { + entryFrameWorkLevel1(ENTRY_FRAME_WORKLOAD_MILLIS); + } + } + + /** + * Burns CPU on the main thread and on the two worker threads at the same time, so that all + * three entry points ({@code ExternalLauncher.main(String[])}, {@code Thread.run()} for the + * {@link EntryFrameRunnable} thread and {@code EntryFrameThread.run()}) are the bottom frame + * of some samples. + */ + private static void runEntryFrameWorkload() throws InterruptedException { + Thread runnableThread = new Thread(new EntryFrameRunnable(), "entry-frame-runnable"); + Thread subclassThread = new EntryFrameThread(); + runnableThread.start(); + subclassThread.start(); + entryFrameWorkLevel1(ENTRY_FRAME_WORKLOAD_MILLIS); + runnableThread.join(); + subclassThread.join(); + } + + // entryFrameWorkLevel1/2 pad the call chain below every entry point, so that a recording + // taken with a small jstackdepth roots its samples inside the chain rather than at the + // entry frame itself - that is how EntryFrameTest gets its negative control. + private static void entryFrameWorkLevel1(long millis) { + entryFrameWorkLevel2(millis); + } + + private static void entryFrameWorkLevel2(long millis) { + entryFrameBurn(millis); + } + + private static void entryFrameBurn(long millis) { + // nanoTime() is monotonic: a wall-clock adjustment mid-burn cannot cut the workload short + // (which would starve the recording of samples) or stretch it past the launcher's timeout. + long deadline = System.nanoTime() + millis * 1_000_000L; + long acc = 0; + while (System.nanoTime() - deadline < 0) { + for (int i = 0; i < 100000; i++) { + acc += i * 31 + (acc >>> 3); + } + } + entryFrameSink = acc; + } + public static void main(String[] args) throws Exception { Thread worker = null; try { @@ -80,6 +158,20 @@ public static void main(String[] args) throws Exception { } } } + } else if (args[0].equals("entry-frames")) { + // Unlike the modes above, this one is only meaningful with a running profiler: + // silently skipping the start would leave the parent process parsing an empty + // recording and reporting a missing entry frame instead of a missing command. + if (args.length < 2 || args[1].isEmpty()) { + throw new IllegalArgumentException( + "entry-frames requires a profiler command list"); + } + JavaProfiler instance = JavaProfiler.getInstance(); + instance.execute(args[1]); + runEntryFrameWorkload(); + // Stop explicitly rather than leaving it to JVM shutdown: the parent process + // starts reading the recording as soon as this process exits. + instance.stop(); } else if (args[0].startsWith("profiler-work:")) { long expectedCpuTime = Long.parseLong(args[0].substring("profiler-work:".length())); ThreadMXBean thrdBean = ManagementFactory.getThreadMXBean(); diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/JfrStackTrace.java b/ddprof-test/src/test/java/com/datadoghq/profiler/JfrStackTrace.java index 548230701c..bdb36edb04 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/JfrStackTrace.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/JfrStackTrace.java @@ -54,7 +54,10 @@ static JfrStackTrace of(Object rawStackTrace) { return new JfrStackTrace(frames, truncated); } - /** This stack trace's frames, outermost (root) frame first. */ + /** + * This stack trace's frames in the order {@code Recording::writeStackTraces} wrote them: + * topmost (leaf) frame first, so the thread entry point is the last element. + */ public List frames() { return frames; } diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/jfr/EntryFrameTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/jfr/EntryFrameTest.java new file mode 100644 index 0000000000..76dc28e0b8 --- /dev/null +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/jfr/EntryFrameTest.java @@ -0,0 +1,178 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package com.datadoghq.profiler.jfr; + +import com.datadoghq.profiler.AbstractProcessProfilerTest; +import com.datadoghq.profiler.JfrEvent; +import com.datadoghq.profiler.JfrEvents; +import com.datadoghq.profiler.JfrFrame; +import com.datadoghq.profiler.JfrStackTrace; + +import org.junitpioneer.jupiter.RetryingTest; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Verifies which bottom-of-stack methods {@code Lookup::fillJavaMethodInfo} (flightRecorder.cpp) + * recognises as thread entry points: {@code void main(String[])}, {@code java.lang.Thread.run()} + * and the {@code void run()} override of any {@link Thread} subclass. + * + *

    Entry-point recognition is not exposed to Java directly. It surfaces in the recording as the + * {@code truncated} flag of {@code jdk.types.StackTrace}: {@code Recording::writeStackTraces} + * writes {@code truncated = !isEntry(bottomFrame)} whenever that bottom frame is a Java frame + * (the native-frame case falls back to the unwinder's own truncation flag instead). So a stack + * that bottoms out in a recognised entry point is reported as complete, and any other Java bottom + * frame is reported as truncated — the profiler cannot tell a stack that genuinely started there + * from one whose remaining frames were lost. + * + *

    The workload runs in a forked JVM ({@link #launch}) because {@code main(String[])} can only + * be the bottom frame of the process' primordial thread; inside the test JVM that frame belongs to + * the build tool's launcher, several dozen frames below the test method. + * + *

    The {@code cstack} mode is deliberately left at its default. Native frames are collected from + * the signal context down to the topmost Java frame only, so in every non-mixed mode the bottom + * frame of a Java thread's sample is still its Java entry point. Forcing a mode would only narrow + * the test: {@code cstack=vmx} appends the native frames below the Java stack and would + * route the bottom frame through the native branch instead, {@code cstack=vm} fails startup outside + * Linux/HotSpot, and {@code cstack=no} leaves {@code StackContext::sp} unset, which makes + * {@code HotspotSupport::getJavaTraceAsync} reject in-Java threads outright + * (AGCT_NATIVE_NO_JAVA_CONTEXT) and yields nothing but {@code no_Java_frame} samples. + */ +public class EntryFrameTest extends AbstractProcessProfilerTest { + + private static final String LAUNCHER = "com.datadoghq.profiler.ExternalLauncher"; + + /** {@code public static void main(String[] args)} of the forked JVM's main class. */ + private static final String MAIN_ROOT = LAUNCHER + ".main([Ljava/lang/String;)V"; + /** {@code new Thread(runnable)} bottoms out in {@code Thread}'s own {@code run()}. */ + private static final String THREAD_RUN_ROOT = "java.lang.Thread.run()V"; + /** A {@code run()} override two levels below {@link Thread}. */ + private static final String SUBCLASS_RUN_ROOT = LAUNCHER + "$EntryFrameThread.run()V"; + + /** Both engines are enabled, and both their event types read back, for sampling headroom. */ + private static final String PROFILER_COMMAND = "start,cpu=10ms,wall=10ms,jfr,file="; + + private static final String[] SAMPLE_EVENT_TYPES = { + "datadog.ExecutionSample", "datadog.MethodSample" + }; + + /** + * Every method the workload's padding call chain consists of. With a depth-limited recording + * these become bottom frames, and none of them is an entry point. + */ + private static final String[] NON_ENTRY_ROOTS = { + LAUNCHER + ".entryFrameWorkLevel1(J)V", + LAUNCHER + ".entryFrameWorkLevel2(J)V", + LAUNCHER + ".entryFrameBurn(J)V", + }; + + @RetryingTest(3) + void entryFramesAreNotMarkedTruncated() throws Exception { + Path recording = newRecordingPath(); + try { + runWorkload(recording, PROFILER_COMMAND + recording.toAbsolutePath()); + + Map roots = truncationCountsByRootFrame(recording); + for (String root : new String[] {MAIN_ROOT, THREAD_RUN_ROOT, SUBCLASS_RUN_ROOT}) { + long[] counts = roots.get(root); + assertNotNull(counts, root + " was never sampled as a bottom frame; bottom frames" + + " seen: " + roots.keySet()); + assertEquals(0L, counts[1], root + " is a thread entry point, but " + + counts[1] + " of its " + (counts[0] + counts[1]) + + " samples were marked truncated"); + } + } finally { + Files.deleteIfExists(recording); + } + } + + /** + * The counterpart of the assertions above: with {@code jstackdepth=2} the same workload's + * samples bottom out inside its padding call chain instead of at a thread entry point, and + * must then be marked truncated. Without this, a build in which nothing is ever recognised as + * an entry point — the {@code truncated} flag stuck at {@code false} — would still pass. + */ + @RetryingTest(3) + void nonEntryFramesAreMarkedTruncated() throws Exception { + Path recording = newRecordingPath(); + try { + runWorkload(recording, + PROFILER_COMMAND + recording.toAbsolutePath() + ",jstackdepth=2"); + + Map roots = truncationCountsByRootFrame(recording); + long notTruncated = 0; + long truncated = 0; + for (String root : NON_ENTRY_ROOTS) { + long[] counts = roots.get(root); + if (counts != null) { + notTruncated += counts[0]; + truncated += counts[1]; + } + } + assertTrue(truncated > 0, "no sample bottomed out inside the workload's call chain;" + + " bottom frames seen: " + roots.keySet()); + assertEquals(0L, notTruncated, notTruncated + " samples bottoming out inside the" + + " workload's call chain were reported as complete stacks"); + } finally { + Files.deleteIfExists(recording); + } + } + + /** + * Allocates the recording in the JVM's own temp directory ({@code java.io.tmpdir}) rather than + * a hard-coded {@code /tmp/recordings}, so the test carries no assumption about a POSIX + * filesystem layout or about {@code /tmp} being writable. The {@code finally} blocks above + * delete it either way. + */ + private Path newRecordingPath() throws Exception { + return Files.createTempFile("entry-frame-test", ".jfr"); + } + + private void runWorkload(Path recording, String commands) throws Exception { + LaunchResult result = launch("entry-frames", Collections.emptyList(), commands, + line -> LineConsumerResult.CONTINUE, line -> LineConsumerResult.CONTINUE); + assertTrue(result.inTime, "forked JVM did not exit in time"); + assertEquals(0, result.exitCode, "forked JVM exited with a non-zero code"); + assertTrue(Files.size(recording) > 0, "forked JVM wrote an empty recording"); + } + + /** + * Buckets every sample in {@code recording} by its bottom frame, counting how many of them + * were reported as complete (index 0) and how many as truncated (index 1). + * + *

    {@code Recording::writeStackTraces} emits the frames of a trace in the order the unwinder + * produced them - topmost frame first - so the entry point the {@code truncated} flag was + * derived from is the last frame of the list. + */ + private static Map truncationCountsByRootFrame(Path recording) throws Exception { + Map counts = new LinkedHashMap<>(); + for (String eventType : SAMPLE_EVENT_TYPES) { + for (JfrEvent sample : JfrEvents.load(recording, eventType)) { + JfrStackTrace stackTrace = sample.getStackTrace(); + if (stackTrace.isEmpty()) { + continue; + } + JfrFrame root = stackTrace.frames().get(stackTrace.frames().size() - 1); + String key = root.className() + "." + root.methodName() + root.methodDescriptor(); + long[] bucket = counts.get(key); + if (bucket == null) { + bucket = new long[2]; + counts.put(key, bucket); + } + bucket[stackTrace.isTruncated() ? 1 : 0]++; + } + } + return counts; + } +} From 4c3c8cc431d17262b98c53f3ac8a160738ff974b Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Thu, 20 Aug 2026 01:32:52 +0000 Subject: [PATCH 11/19] Fix --- .../src/test/cpp/hotspotMethodId_ut.cpp | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp b/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp index dbc4f25b0d..fa39f6248e 100644 --- a/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp +++ b/ddprof-lib/src/test/cpp/hotspotMethodId_ut.cpp @@ -355,8 +355,15 @@ TEST(HotspotMethodIdTest, IdReturnsValidIdForPopulatedSlot) { namespace { // Two adjacent pages; the second is PROT_NONE. Fake metadata lives at the start -// of the first, so an offset of kPageSize lands on the guard page. -constexpr size_t kPageSize = 4096; +// of the first, so an offset of kPageSize() lands on the guard page. +// Must be the real OS page size, not a hardcoded 4096: mmap()/mprotect() need +// page-aligned addresses and sizes, and on arm64 Linux the page size can be +// 16384 or 65536, not 4096. A function rather than a namespace-scope const: +// OS::page_size is itself a dynamically-initialized static in another +// translation unit, and cross-TU static init order is unspecified, so caching +// it in another static here could read it before it's set. Calling through a +// function defers the read to test-run time, well after all static init. +size_t kPageSize() { return OS::page_size; } // Layout of the fake metadata used by the resolve() tests. Sizes are the values // VMConstantPool::base() and cast_or_null() need; they only have to be non-zero @@ -454,10 +461,10 @@ class HotspotResolveCrashProtectionTest : public ::testing::Test { _orig_segv = OS::replaceSigsegvHandler(Profiler::segvHandler); _orig_bus = OS::replaceSigbusHandler(Profiler::busHandler); - _region = mmap(nullptr, 2 * kPageSize, PROT_READ | PROT_WRITE, + _region = mmap(nullptr, 2 * kPageSize(), PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); ASSERT_NE(MAP_FAILED, _region); - ASSERT_EQ(0, mprotect((char*)_region + kPageSize, kPageSize, PROT_NONE)); + ASSERT_EQ(0, mprotect((char*)_region + kPageSize(), kPageSize(), PROT_NONE)); // Two exported symbols from hotspotSupport.cpp, far apart in that TU. uintptr_t a = (uintptr_t)&HotspotSupport::resolve; @@ -469,7 +476,7 @@ class HotspotResolveCrashProtectionTest : public ::testing::Test { void TearDown() override { Profiler::resetAddressRangeForTest(); - munmap(_region, 2 * kPageSize); + munmap(_region, 2 * kPageSize()); OS::replaceSigsegvHandler(_orig_segv); OS::replaceSigbusHandler(_orig_bus); ProfiledThread::release(); @@ -498,7 +505,7 @@ TEST_F(HotspotResolveCrashProtectionTest, ResolveRecoversFromFaultInsteadOfCrash // only validates [ptr, ptr + VMMethod::type_size()), which stays on the // readable page -- the fault comes from the offset, not the pointer. VMStructsTestAccessor::Offsets faulting = RESOLVE_OFFSETS; - faulting.method_constmethod = (int)kPageSize; + faulting.method_constmethod = (int)kPageSize(); VMStructsTestAccessor offsets(faulting); VMStructsTestAccessor::SymbolLayout layout(RESOLVE_SYMBOL_OFFSETS, RESOLVE_TYPE_SIZES); @@ -520,7 +527,7 @@ TEST_F(HotspotResolveCrashProtectionTest, ResolveRecoversFromFaultInsteadOfCrash TEST_F(HotspotResolveCrashProtectionTest, ResolveRecoversRepeatedly) { HotspotMethodIdVMHotspotGuard hotspot; VMStructsTestAccessor::Offsets faulting = RESOLVE_OFFSETS; - faulting.method_constmethod = (int)kPageSize; + faulting.method_constmethod = (int)kPageSize(); VMStructsTestAccessor offsets(faulting); VMStructsTestAccessor::SymbolLayout layout(RESOLVE_SYMBOL_OFFSETS, RESOLVE_TYPE_SIZES); @@ -550,7 +557,7 @@ TEST_F(HotspotResolveCrashProtectionTest, ResolveReturnsNullForUnreadableSymbolB // page: exactly VMSymbol::type_size() bytes, so VMSymbol::cast_or_null()'s // own isReadableRange() check still passes and the rejection has to come from // copySymbolBody(). The declared length then runs the body off the page. - char* edge = (char*)_region + kPageSize - RESOLVE_TYPE_SIZES.symbol; + char* edge = (char*)_region + kPageSize() - RESOLVE_TYPE_SIZES.symbol; *(uint16_t*)(edge + RESOLVE_SYMBOL_OFFSETS.symbol_length) = 100; f->klass.name_symbol = edge; From 775d1d772c51796d806b59bb9d740ab51c53e7c8 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Thu, 20 Aug 2026 12:39:03 +0000 Subject: [PATCH 12/19] Cleanup --- .../src/main/cpp/hotspot/hotspotSupport.cpp | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp index 49ebed46be..cee326d381 100644 --- a/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp +++ b/ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp @@ -1437,28 +1437,6 @@ bool HotspotSupport::loadMethodIDsIfNeededImpl(jvmtiEnv *jvmti, JNIEnv *jni, jcl return JVMSupport::loadMethodIDsImpl(jvmti, jni, klass); } -// Fixed-size fast-path buffers for the common case, sized so that a -// realistic name/signature/class name never needs to allocate. 4096 mirrors -// Lookup::resolveVTableReceiverCached (flightRecorder.cpp), which reads the -// same Symbol bodies on the same thread; it is a precedent to stay -// consistent with, not a JVM-spec limit. -// HotSpot's Symbol length field is a u2, so the VM permits up to 65535 bytes. -// A name/descriptor that overflows its fixed buffer falls back to malloc -// (see ResolvedNames::setImpl), up to MAX_SYMBOL_LEN below; beyond that it is -// rejected and the frame serializes as "unknown" -- METHOD_RESOLVE_SYMBOL_UNREADABLE -// makes that visible if it bites. Unlike the old stack-only design, the malloc -// fallback means this is no longer a leak-proof region: siglongjmp out of -// resolve() bypasses ~ResolvedNames(), so the fault-recovery path must call -// ResolvedNames::release() explicitly (see resolve()) to free any buffer -// allocated before the fault. -static const size_t MAX_KLASS_NAME_LEN = 1024; // internal names; realistically < 256 -static const size_t MAX_METHOD_NAME_LEN = 512; // realistically < 64 -// A descriptor can in principle exceed this (255 argument *slots*, each able to -// carry an arbitrarily long L...; type name), so this cap is a fidelity choice, -// not a proof: over-cap descriptors serialize as "unknown" rather than being -// truncated. METHOD_RESOLVE_SYMBOL_UNREADABLE makes it visible if that bites. -static const size_t MAX_SIGNATURE_LEN = 1024; - // The three names resolve() needs, owned by resolve()'s frame. Each name has // a fixed-size inline buffer for the common case, with a malloc'd fallback // (up to MAX_SYMBOL_LEN) for names that don't fit -- see release() for why @@ -1466,12 +1444,34 @@ static const size_t MAX_SIGNATURE_LEN = 1024; class ResolvedNames { // Hard ceiling for the malloc fallback in setImpl(), independent of which // field is being resolved. Not a JVM/class-file limit (Symbol::length() is - // a u2, so up to 65535 is legal) -- chosen to match MAX_KLASS_NAME_LEN and - // MAX_SIGNATURE_LEN above, so a name that would already have been rejected + // a u2, so up to 65535 is legal) -- a name that would already have been rejected // as too long for those fields' fixed buffers doesn't get an unbounded // allocation just because it went through the malloc path instead. - static constexpr size_t MAX_SYMBOL_LEN = 4 * 1024; -private: + static constexpr size_t MAX_SYMBOL_LEN = 64 * 1024; + + // Fixed-size fast-path buffers for the common case, sized so that a + // realistic name/signature/class name never needs to allocate. 4096 mirrors + // Lookup::resolveVTableReceiverCached (flightRecorder.cpp), which reads the + // same Symbol bodies on the same thread; it is a precedent to stay + // consistent with, not a JVM-spec limit. + // HotSpot's Symbol length field is a u2, so the VM permits up to 65535 bytes. + // A name/descriptor that overflows its fixed buffer falls back to malloc + // (see ResolvedNames::setImpl), up to MAX_SYMBOL_LEN below; beyond that it is + // rejected and the frame serializes as "unknown" -- METHOD_RESOLVE_SYMBOL_UNREADABLE + // makes that visible if it bites. Unlike the old stack-only design, the malloc + // fallback means this is no longer a leak-proof region: siglongjmp out of + // resolve() bypasses ~ResolvedNames(), so the fault-recovery path must call + // ResolvedNames::release() explicitly (see resolve()) to free any buffer + // allocated before the fault. + static constexpr size_t MAX_KLASS_NAME_LEN = 1024; // internal names; realistically < 256 + static constexpr size_t MAX_METHOD_NAME_LEN = 512; // realistically < 64 + // A descriptor can in principle exceed this (255 argument *slots*, each able to + // carry an arbitrarily long L...; type name), so this cap is a fidelity choice, + // not a proof: over-cap descriptors serialize as "unknown" rather than being + // truncated. METHOD_RESOLVE_SYMBOL_UNREADABLE makes it visible if that bites. + static constexpr size_t MAX_SIGNATURE_LEN = 1024; + + private: // volatile: resolve() mutates these (via setMethodName/setMethodSignature/ // setKlassName, called through readMethodNames()) between sigsetjmp() and a // possible siglongjmp() out of a fault, then release() reads them back at From 5e10c012c2a3d5c5ffb084b08a60a2c7726843ba Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 12 Aug 2026 12:50:42 +0000 Subject: [PATCH 13/19] v0 --- ddprof-lib/src/main/cpp/counters.h | 5 +++-- ddprof-lib/src/main/cpp/faultInjection.cpp | 7 +++++++ ddprof-lib/src/main/cpp/faultInjection.h | 6 ++++++ 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index c3b8b3c555..bf8f6bd704 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/ddprof-lib/src/main/cpp/counters.h @@ -135,8 +135,9 @@ X(SAFEFETCH_FAILED, "safefetch_failed") \ /* Every siglongjmp recovery, from any protected window, counted centrally \ * in Profiler::checkFault(). */ \ - X(STACKWALK_LONGJMP_RECOVERED, "stackwalk_longjmp_recovered") \ - /* Dump-time raw-Method* resolution (HotspotSupport::resolve, reached only \ + X(STACKWALK_LONGJMP_RECOVERED, "stackwalk_longjmp_recovered") \ + X(METHOD_RESOLUTION_FAILED, "method_resolution_failed") \ + /* Dump-time raw-Method* resolution (HotspotSupport::resolve, reached only \ * for cstack=vm + fjmethodid=false frames). NOT additive with \ * STACKWALK_LONGJMP_RECOVERED: checkFault() bumps that one unconditionally \ * before every siglongjmp, so each fault counted here is counted there too. \ diff --git a/ddprof-lib/src/main/cpp/faultInjection.cpp b/ddprof-lib/src/main/cpp/faultInjection.cpp index 478c1089e6..0c3ac52516 100644 --- a/ddprof-lib/src/main/cpp/faultInjection.cpp +++ b/ddprof-lib/src/main/cpp/faultInjection.cpp @@ -110,6 +110,13 @@ bool shouldFire(u64 threshold, const char* fn) { return false; } +void crashNow() { + volatile uintptr_t* p = (volatile uintptr_t*)poisonAddress(); + *p = 0xBAD; + __builtin_unreachable(); // PROT_NONE guard page: the store above never returns. +} + + uintptr_t poisonAddress() { u64 r = nextRandom(); if (g_guard_ok.load(std::memory_order_acquire)) { diff --git a/ddprof-lib/src/main/cpp/faultInjection.h b/ddprof-lib/src/main/cpp/faultInjection.h index b9ead472ee..f8b08f73e4 100644 --- a/ddprof-lib/src/main/cpp/faultInjection.h +++ b/ddprof-lib/src/main/cpp/faultInjection.h @@ -93,6 +93,12 @@ bool shouldFire(u64 threshold, const char* fn); // SIGSEGV). If init() failed, it falls back to a best-effort garbage address. uintptr_t poisonAddress(); +// Deliberately dereferences poisonAddress() to raise a real SIGSEGV right now, +// unconditionally (no probability gate, no shouldFire() draw). For exercising +// crash-handler / recovery paths on demand (e.g. from a test), never from a +// production code path. +[[noreturn]] void crashNow(); + // Returns ptr unchanged, or a poison address (cast to T) when the tier fires. // Templated so the wrapped expression's static type (void**, const char*, // uintptr_t, ...) is preserved exactly. From a730d2c6afa558195d3a22ce54791a01af028cd3 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 12 Aug 2026 16:41:03 +0000 Subject: [PATCH 14/19] Cleanup and fix tests --- ddprof-lib/src/main/cpp/faultInjection.cpp | 7 ------ ddprof-lib/src/main/cpp/faultInjection.h | 6 ----- ddprof-lib/src/main/cpp/guards.cpp | 6 ++--- ddprof-lib/src/main/cpp/threadLocalData.h | 2 +- .../cpp/resolveMethodFaultInjection_ut.cpp | 25 +++++++++++++++++++ 5 files changed, 29 insertions(+), 17 deletions(-) diff --git a/ddprof-lib/src/main/cpp/faultInjection.cpp b/ddprof-lib/src/main/cpp/faultInjection.cpp index 0c3ac52516..478c1089e6 100644 --- a/ddprof-lib/src/main/cpp/faultInjection.cpp +++ b/ddprof-lib/src/main/cpp/faultInjection.cpp @@ -110,13 +110,6 @@ bool shouldFire(u64 threshold, const char* fn) { return false; } -void crashNow() { - volatile uintptr_t* p = (volatile uintptr_t*)poisonAddress(); - *p = 0xBAD; - __builtin_unreachable(); // PROT_NONE guard page: the store above never returns. -} - - uintptr_t poisonAddress() { u64 r = nextRandom(); if (g_guard_ok.load(std::memory_order_acquire)) { diff --git a/ddprof-lib/src/main/cpp/faultInjection.h b/ddprof-lib/src/main/cpp/faultInjection.h index f8b08f73e4..b9ead472ee 100644 --- a/ddprof-lib/src/main/cpp/faultInjection.h +++ b/ddprof-lib/src/main/cpp/faultInjection.h @@ -93,12 +93,6 @@ bool shouldFire(u64 threshold, const char* fn); // SIGSEGV). If init() failed, it falls back to a best-effort garbage address. uintptr_t poisonAddress(); -// Deliberately dereferences poisonAddress() to raise a real SIGSEGV right now, -// unconditionally (no probability gate, no shouldFire() draw). For exercising -// crash-handler / recovery paths on demand (e.g. from a test), never from a -// production code path. -[[noreturn]] void crashNow(); - // Returns ptr unchanged, or a poison address (cast to T) when the tier fires. // Templated so the wrapped expression's static type (void**, const char*, // uintptr_t, ...) is preserved exactly. diff --git a/ddprof-lib/src/main/cpp/guards.cpp b/ddprof-lib/src/main/cpp/guards.cpp index 504b8386cc..826e109b72 100644 --- a/ddprof-lib/src/main/cpp/guards.cpp +++ b/ddprof-lib/src/main/cpp/guards.cpp @@ -46,9 +46,8 @@ int getInSignalDepth() { bool isInTrackedSignalContext() { ProfiledThread *pt = ProfiledThread::current(); - // null ProfiledThread = no thread context; - // the SignalHandlerScope never ran, so we have no positive evidence - // of a signal frame. + // null ProfiledThread = no thread context; the SignalHandlerScope + // never ran, so we have no positive evidence of a signal frame. // See header comment for the rationale of returning false here. return pt != nullptr && pt->signalDepth() != 0; } @@ -56,6 +55,7 @@ bool isInTrackedSignalContext() { SignalHandlerScope::SignalHandlerScope(bool shouldRunPriming) : _current(nullptr), _active(true) { ProfiledThread *pt = shouldRunPriming ? ProfiledThread::acquireCurrent() : ProfiledThread::current(); if (pt != nullptr) { + DEBUG_ONLY(_signal_depth = pt->signalDepth();) _current = pt; pt->enterSignalScope(); } else { diff --git a/ddprof-lib/src/main/cpp/threadLocalData.h b/ddprof-lib/src/main/cpp/threadLocalData.h index 4801fd3eaf..52989cceeb 100644 --- a/ddprof-lib/src/main/cpp/threadLocalData.h +++ b/ddprof-lib/src/main/cpp/threadLocalData.h @@ -85,7 +85,7 @@ class ProfiledThread : public ThreadLocalData { u32 _wall_epoch; u64 _call_trace_id; u32 _recording_epoch; - u32 _misc_flags; + volatile u32 _misc_flags; u64 _park_block_token; int _filter_slot_id; // Slot ID for thread filtering uint8_t _init_window; // Countdown for JVM thread init race window (PROF-13072) diff --git a/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp b/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp index 262fc950f3..ca32cead22 100644 --- a/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp +++ b/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp @@ -197,4 +197,29 @@ TEST(ResolveMethodFaultInjectionTest, DisabledBuildResolvesNormally) { EXPECT_FALSE(t->isProtected()); } +#else // __FAULT_INJECTION__ not defined (the default release/debug build). + +// INJECT_CRASH_LIKELY() in resolveMethod() compiles to nothing here (see +// faultInjection.h), so there is nothing to inject -- this is a plain smoke +// test of the same call, kept for two reasons: (1) it documents that the +// call site is inert in this configuration, and (2) a translation unit that +// registers zero gtest tests fails to *link* as its own binary: with no +// TEST/TEST_F in this object file, nothing here pulls a member out of +// -lgtest before -lgtest_main's gtest_main.cc.o (which needs +// testing::InitGoogleTest() etc. from that same archive) is processed, and +// -lgtest is never revisited afterwards. +TEST(ResolveMethodFaultInjectionTest, DisabledBuildResolvesNormally) { + StringDictionary classes; + MethodMap methods; + Lookup lookup(nullptr, &methods, &classes); + + ASGCT_CallFrame frame{}; + frame.bci = 0; + frame.method_id = nullptr; + + MethodInfo* info = lookup.resolveMethod(frame); + ASSERT_NE(info, nullptr); + EXPECT_EQ(info->_type, FRAME_NATIVE); +} + #endif // __FAULT_INJECTION__ From 35c00f86726664296805832023089fc01c31729c Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 12 Aug 2026 23:52:07 +0000 Subject: [PATCH 15/19] v2 --- ddprof-lib/src/main/cpp/counters.h | 10 +++++ ddprof-lib/src/main/cpp/flightRecorder.cpp | 23 ++++++---- ddprof-lib/src/main/cpp/guards.h | 50 ++++++++++++++++++++++ 3 files changed, 74 insertions(+), 9 deletions(-) diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index bf8f6bd704..4fa5960164 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/ddprof-lib/src/main/cpp/counters.h @@ -163,6 +163,16 @@ * samples_dropped_thread_local) to isolate non-pool priming drops, never \ * summed. */ \ X(SAMPLES_DROPPED_TLS_POOL_EXHAUSTED, "thread_local_pool_exhausted") \ + /* Subset of the above: recoveries that landed in Lookup::resolveMethod(), \ + * i.e. faults while symbolicating at dump time rather than while walking a \ + * stack in a signal handler. Counted separately because the two have \ + * different root causes (stale jmethodID / class unload vs. a bad frame \ + * pointer) and would otherwise be indistinguishable. */ \ + X(METHOD_RESOLVE_LONGJMP_RECOVERED, "method_resolve_longjmp_recovered") \ + /* Lookup::resolveMethod() calls that ran without siglongjmp protection \ + * because no ProfiledThread could be allocated for the dump thread (OOM): \ + * there is nowhere to publish a landing pad. Expected to stay at 0. */ \ + X(METHOD_RESOLVE_UNPROTECTED, "method_resolve_unprotected") \ /* writeElement() guards against a corrupted/dangling JfrMetadata tree. \ * Root cause is still unconfirmed, so these counters are the durable \ * signal for spotting a recurrence. */ \ diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp index 3e0a9169c1..4e2e5a7e2e 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.cpp +++ b/ddprof-lib/src/main/cpp/flightRecorder.cpp @@ -624,14 +624,14 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { // initCurrentThreadSignalSafe() can only fail on OOM. ProfiledThread *prof_thread = ProfiledThread::initCurrentThreadSignalSafe(); if (prof_thread == nullptr) { - // No thread context means nowhere to publish a landing pad. Resolve to - // the shared unknown row rather than touching VM metadata unprotected -- - // both call sites in writeStackTraces() dereference the result - // unconditionally, so a nullptr return would convert a transient - // allocation failure into a SIGSEGV on the dump thread. Same counter - // HotspotSupport::resolve() uses for the identical no-landing-pad case. - Counters::increment(SAMPLES_DROPPED_THREAD_LOCAL); - return unknownMethod(); + // No thread context means nowhere to publish a landing pad. Resolve + // unprotected rather than returning nullptr: both call sites in + // writeStackTraces() dereference the result unconditionally, so a nullptr + // return would convert a transient allocation failure into a SIGSEGV on the + // dump thread. Unprotected is also exactly what this code did before the + // protection was added. + Counters::increment(METHOD_RESOLVE_UNPROTECTED); + return fillMethod(frame, method_id, bci); } // Fill the shared "unknown" row *before* arming. The recovery branch below @@ -666,7 +666,6 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) { return &_unknown_method; } jmp_scope.install(&crash_protection_ctx); - return fillMethod(frame, method_id, bci); } @@ -682,6 +681,12 @@ MethodInfo *Lookup::fillMethod(ASGCT_CallFrame &frame, jmethodID method_id, assert(method_id != nullptr && "Already filtered by caller"); + // Inject fault to test siglongjmp protection. Sits inside the window + // resolveMethod() arms around this function, which is the point: this is + // never compiled into a production build (it needs -PenableFaultInjection). + INJECT_CRASH_LIKELY(); + + // BCI_VTABLE_RECEIVER: method holds a VMSymbol* (see vmEntry.h). Resolve // to a class_id via the per-dump cache once, then key MethodMap by the // resolved class_id so two distinct Symbol addresses for the same class diff --git a/ddprof-lib/src/main/cpp/guards.h b/ddprof-lib/src/main/cpp/guards.h index 3889c04d24..506586bc5d 100644 --- a/ddprof-lib/src/main/cpp/guards.h +++ b/ddprof-lib/src/main/cpp/guards.h @@ -139,6 +139,56 @@ class SignalHandlerScope { void signalHandlerUnwindAfterLongjmp(); #define SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP() signalHandlerUnwindAfterLongjmp() +// RAII for the per-thread siglongjmp landing pad (ProfiledThread::_jmp_buf) +// that Profiler::checkFault() jumps through. +// +// The previous landing pad must be reinstated on *every* exit from the frame +// that owns the sigjmp_buf -- normal return, a siglongjmp back into it, or an +// exception unwinding out of it -- because checkFault() will happily jump into +// a landing pad whose stack frame has already been popped. Hand-rolled +// "setJmpCtx(prev) before each return" only covers the returns the author +// remembered. +// +// Both members are const and initialised before the owning frame calls +// sigsetjmp(), and install()/restore() mutate only the ProfiledThread, so the +// guard's own state is never modified between sigsetjmp() and siglongjmp(). +// Reading it from the landing pad is therefore well defined -- unlike a plain +// non-volatile local, whose value after siglongjmp is indeterminate if it was +// assigned in the meantime. +// +// Usage: +// sigjmp_buf ctx; +// JmpCtxScope jmp_scope(prof_thread); // pt must be non-null +// if (sigsetjmp(ctx, 1) != 0) { // savemask=1: see note below +// SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); +// jmp_scope.restore(); // disarm before anything else +// return recovery_value; +// } +// jmp_scope.install(&ctx); +// ... risky work ... +// +// savemask must be 1: the siglongjmp originates inside the SIGSEGV handler, +// where the kernel has SIGSEGV blocked, so without restoring the saved mask the +// signal would stay blocked and the next fault on this thread would be fatal. +class JmpCtxScope { +public: + // `pt` must be non-null. + explicit JmpCtxScope(ProfiledThread* pt); + ~JmpCtxScope(); + // Publish `ctx` as this thread's landing pad; call after sigsetjmp() + // returns 0. + void install(sigjmp_buf* ctx); + // Reinstate the previous landing pad now. Idempotent with the destructor, + // so it is safe (and required) to call from the sigsetjmp landing pad + // before touching anything that could fault again. + void restore(); + JmpCtxScope(const JmpCtxScope&) = delete; + JmpCtxScope& operator=(const JmpCtxScope&) = delete; +private: + ProfiledThread* const _pt; + sigjmp_buf* const _prev; +}; + /** * Race-free critical section using atomic compare-and-swap. * From 4ec70b8baec742ba7f0ecc2e2068dbca8dc1b718 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Thu, 20 Aug 2026 18:01:08 +0200 Subject: [PATCH 16/19] Serialize stale jmethodID frames as instead of jvmtiError (#744) --- ddprof-lib/src/main/cpp/flightRecorder.cpp | 2 +- .../profiler/cpu/CTimerSamplerTest.java | 8 +-- .../profiler/cpu/RemoteSymbolicationTest.java | 6 -- .../datadoghq/profiler/cpu/SmokeCpuTest.java | 3 - .../JMethodIDInvalidationStressTest.java | 60 +++++++++++++++++++ .../profiler/wallclock/SmokeWallTest.java | 7 --- 6 files changed, 63 insertions(+), 23 deletions(-) diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp index 4e2e5a7e2e..3f540d35a4 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.cpp +++ b/ddprof-lib/src/main/cpp/flightRecorder.cpp @@ -355,7 +355,7 @@ void Lookup::fillJavaMethodInfo(MethodInfo *mi, jmethodID method, } else { Counters::increment(JMETHODID_SKIPPED); class_name_id = _classes->lookupDuringDump("", 0, Profiler::maxClassMapSize()); - method_name_id = _symbols.lookup("jvmtiError"); + method_name_id = _symbols.lookup(""); method_sig_id = _symbols.lookup("()L;"); } diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java index 6fe6fe3073..123b015834 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java @@ -23,7 +23,6 @@ import java.util.Set; import java.util.concurrent.ExecutionException; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeFalse; import static org.junit.jupiter.api.Assumptions.assumeTrue; @@ -55,11 +54,8 @@ public void test(@CStack String cstack) throws ExecutionException, InterruptedEx verifyCStackSettings(); // Streamed rather than materialized: cpu=100us over this workload can produce tens of - // thousands of samples, and every check here is per-event with no need to retain them. - long sampleCount = streamEvents("datadog.ExecutionSample", sample -> { - String stackTrace = sample.getStackTraceString(); - assertFalse(stackTrace.contains("jvmtiError")); - }); + // thousands of samples; streamEvents counts them without retaining them in memory. + long sampleCount = streamEvents("datadog.ExecutionSample", sample -> { }); assertTrue(sampleCount > 0, "datadog.ExecutionSample was empty"); } diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/RemoteSymbolicationTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/RemoteSymbolicationTest.java index 89e07023bb..de6d18841a 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/RemoteSymbolicationTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/RemoteSymbolicationTest.java @@ -18,9 +18,7 @@ import com.datadoghq.profiler.JfrEvents; import com.datadoghq.profiler.JfrFrame; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; /** * Integration test for remote symbolication feature. @@ -110,11 +108,7 @@ public void testRemoteSymbolicationEnabled(@CStack String cstack) throws Excepti // Iterate through frames to check for test library frames for (JfrFrame frame : sample.getStackTrace().frames()) { - // Check for jvmtiError in method name String methodName = frame.methodName(); - if (methodName != null && methodName.contains("jvmtiError")) { - fail("Found jvmtiError in frame method name: " + methodName); - } // Get class name (contains build-id for remote symbolication frames) String className = frame.className(); diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java index 74a0ddf2b9..bd894e07e5 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java @@ -17,7 +17,6 @@ import java.util.concurrent.ExecutionException; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import com.datadoghq.profiler.Platform; @@ -44,7 +43,6 @@ public void testComputations(@CStack String cstack) throws Exception { // on mac the usage of itimer to drive the sampling provides very unreliable outputs for (JfrEvent sample : events) { String stackTrace = sample.getStackTraceString(); - assertFalse(stackTrace.contains("jvmtiError")); if ("vmx".equals(stackTrace)) { // extra checks to make sure we see the mixed stacktraces assertTrue(stackTrace.contains("JavaCalls::call_virtual()"), @@ -71,7 +69,6 @@ public void testIOBound(@CStack String cstack) throws Exception { // on mac the usage of itimer to drive the sampling provides very unreliable outputs for (JfrEvent sample : events) { String stackTrace = sample.getStackTraceString(); - assertFalse(stackTrace.contains("jvmtiError")); if ("vmx".equals(stackTrace)) { // extra checks to make sure we see the mixed stacktraces assertTrue(stackTrace.contains("JavaCalls::call_virtual()"), diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java index b098b46070..443fbd635b 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java @@ -15,6 +15,8 @@ */ package com.datadoghq.profiler.memleak; +import com.datadoghq.profiler.JfrEvent; +import com.datadoghq.profiler.JfrFrame; import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; @@ -33,6 +35,7 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -188,6 +191,22 @@ public void testProfilerSurvivesConcurrentClassUnloadDuringDump() throws Excepti + "churn, not just that the JVM didn't crash; if this keeps getting skipped, the " + "churn isn't racing unload against resolveMethod/fillJavaMethodInfo tightly " + "enough (consider more churn threads or a longer window)."); + + // The jmethodid_skipped_count counter is incremented in the exact branch of + // Lookup::fillJavaMethodInfo (the JVMTI-resolution-failure 'else') that serializes a + // stale-jmethodID frame as ''. line_number_table_unreadable is a separate, + // mutually-exclusive branch where the jmethodID *did* resolve to a real method/class name + // and only the line-number-table copy failed -- it never produces ''. So the + // recording assertion below is only meaningful when skippedDelta > 0; a run that only + // triggered the line-table race would otherwise fail spuriously. Gate it accordingly. + Assumptions.assumeTrue(skippedDelta > 0, + "Churn window hit only the line-number-table race (line_number_table_unreadable delta=" + + unreadableLineTableDelta + ", jmethodid_skipped_count delta=" + skippedDelta + + ") -- the '' label assertion is only meaningful when the" + + " JVMTI-resolution-failure branch ran; skipping to avoid a spurious failure."); + // Assert that the recording produced by that branch uses the '' label and + // never the legacy 'jvmtiError' one -- this fails if the label is reverted to 'jvmtiError'. + assertUnloadedFrameLabel(dumpFile); } finally { running.set(false); for (Thread t : churnThreads) { @@ -217,6 +236,47 @@ public void testProfilerSurvivesConcurrentClassUnloadDuringDump() throws Excepti } } + /** + * Asserts that the JFR recording produced by the churn window serializes stale-jmethodID + * frames as {@code ""} and never as the legacy {@code "jvmtiError"} label. + * This is the regression guard for the flightRecorder.cpp remap: reverting the label to + * {@code "jvmtiError"} makes this assertion fail. Only stack-trace-bearing event types + * are inspected; events without a {@code stackTrace} field are skipped. + */ + private void assertUnloadedFrameLabel(Path recording) throws Exception { + AtomicBoolean foundUnloaded = new AtomicBoolean(); + AtomicBoolean foundLegacy = new AtomicBoolean(); + AtomicReference legacySample = new AtomicReference<>(); + for (String eventType : new String[]{"datadog.ExecutionSample", "datadog.AllocationSample"}) { + streamEvents(recording, eventType, event -> { + if (!event.has(STACK_TRACE)) { + return; + } + for (JfrFrame frame : event.getStackTrace().frames()) { + String name = frame.methodName(); + if (name == null) { + continue; + } + if (name.equals("")) { + foundUnloaded.set(true); + } else if (name.equals("jvmtiError")) { + foundLegacy.set(true); + if (legacySample.get() == null) { + legacySample.set(event.getStackTraceString()); + } + } + } + }); + } + assertTrue(foundUnloaded.get(), + "Expected at least one frame serialized as '' in " + recording + + " (jmethodid_skipped_count fired, so the stale-jmethodID branch ran), " + + "but none was found -- the remap to '' may have been reverted."); + assertTrue(!foundLegacy.get(), + "Found a frame serialized as the legacy 'jvmtiError' label in " + recording + + "; expected ''. First offending sample: " + legacySample.get()); + } + private void churnLoop(AtomicBoolean running) { while (running.get()) { try { diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java index 91a717efea..d8cda8b4b3 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java @@ -12,12 +12,10 @@ import com.datadoghq.profiler.junit.RetryTest; import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.params.provider.ValueSource; -import com.datadoghq.profiler.JfrEvent; import com.datadoghq.profiler.JfrEvents; import java.util.concurrent.ExecutionException; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assumptions.assumeFalse; public class SmokeWallTest extends CStackAwareAbstractProfilerTest { @@ -44,11 +42,6 @@ public void test(@CStack String cstack) throws ExecutionException, InterruptedEx verifyCStackSettings(); JfrEvents events = verifyEvents("datadog.MethodSample"); - - for (JfrEvent sample : events) { - String stackTrace = sample.getStackTraceString(); - assertFalse(stackTrace.contains("jvmtiError")); - } } @Override From c95dd36f11edcaa80a7dbb180a8a36c2fd1fb890 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Fri, 21 Aug 2026 10:55:29 -0400 Subject: [PATCH 17/19] Rebase --- ddprof-lib/src/main/cpp/counters.h | 2 +- ddprof-lib/src/main/cpp/guards.h | 50 +----------------------------- 2 files changed, 2 insertions(+), 50 deletions(-) diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index 4fa5960164..6418437ee7 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/ddprof-lib/src/main/cpp/counters.h @@ -135,7 +135,7 @@ X(SAFEFETCH_FAILED, "safefetch_failed") \ /* Every siglongjmp recovery, from any protected window, counted centrally \ * in Profiler::checkFault(). */ \ - X(STACKWALK_LONGJMP_RECOVERED, "stackwalk_longjmp_recovered") \ + X(STACKWALK_LONGJMP_RECOVERED, "stackwalk_longjmp_recovered") \ X(METHOD_RESOLUTION_FAILED, "method_resolution_failed") \ /* Dump-time raw-Method* resolution (HotspotSupport::resolve, reached only \ * for cstack=vm + fjmethodid=false frames). NOT additive with \ diff --git a/ddprof-lib/src/main/cpp/guards.h b/ddprof-lib/src/main/cpp/guards.h index 506586bc5d..43b1a68075 100644 --- a/ddprof-lib/src/main/cpp/guards.h +++ b/ddprof-lib/src/main/cpp/guards.h @@ -24,6 +24,7 @@ #include #include "common.h" +#include "counters.h" class ProfiledThread; @@ -139,55 +140,6 @@ class SignalHandlerScope { void signalHandlerUnwindAfterLongjmp(); #define SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP() signalHandlerUnwindAfterLongjmp() -// RAII for the per-thread siglongjmp landing pad (ProfiledThread::_jmp_buf) -// that Profiler::checkFault() jumps through. -// -// The previous landing pad must be reinstated on *every* exit from the frame -// that owns the sigjmp_buf -- normal return, a siglongjmp back into it, or an -// exception unwinding out of it -- because checkFault() will happily jump into -// a landing pad whose stack frame has already been popped. Hand-rolled -// "setJmpCtx(prev) before each return" only covers the returns the author -// remembered. -// -// Both members are const and initialised before the owning frame calls -// sigsetjmp(), and install()/restore() mutate only the ProfiledThread, so the -// guard's own state is never modified between sigsetjmp() and siglongjmp(). -// Reading it from the landing pad is therefore well defined -- unlike a plain -// non-volatile local, whose value after siglongjmp is indeterminate if it was -// assigned in the meantime. -// -// Usage: -// sigjmp_buf ctx; -// JmpCtxScope jmp_scope(prof_thread); // pt must be non-null -// if (sigsetjmp(ctx, 1) != 0) { // savemask=1: see note below -// SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP(); -// jmp_scope.restore(); // disarm before anything else -// return recovery_value; -// } -// jmp_scope.install(&ctx); -// ... risky work ... -// -// savemask must be 1: the siglongjmp originates inside the SIGSEGV handler, -// where the kernel has SIGSEGV blocked, so without restoring the saved mask the -// signal would stay blocked and the next fault on this thread would be fatal. -class JmpCtxScope { -public: - // `pt` must be non-null. - explicit JmpCtxScope(ProfiledThread* pt); - ~JmpCtxScope(); - // Publish `ctx` as this thread's landing pad; call after sigsetjmp() - // returns 0. - void install(sigjmp_buf* ctx); - // Reinstate the previous landing pad now. Idempotent with the destructor, - // so it is safe (and required) to call from the sigsetjmp landing pad - // before touching anything that could fault again. - void restore(); - JmpCtxScope(const JmpCtxScope&) = delete; - JmpCtxScope& operator=(const JmpCtxScope&) = delete; -private: - ProfiledThread* const _pt; - sigjmp_buf* const _prev; -}; /** * Race-free critical section using atomic compare-and-swap. From e083f0053fc2944d17fb02b2a8a16bf324eddbf4 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Fri, 21 Aug 2026 14:15:29 -0400 Subject: [PATCH 18/19] Merge --- ddprof-lib/src/main/cpp/counters.h | 6 ----- ddprof-lib/src/main/cpp/threadLocalData.h | 2 +- .../cpp/resolveMethodFaultInjection_ut.cpp | 25 ------------------- 3 files changed, 1 insertion(+), 32 deletions(-) diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index 6418437ee7..84b38f8a60 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/ddprof-lib/src/main/cpp/counters.h @@ -163,12 +163,6 @@ * samples_dropped_thread_local) to isolate non-pool priming drops, never \ * summed. */ \ X(SAMPLES_DROPPED_TLS_POOL_EXHAUSTED, "thread_local_pool_exhausted") \ - /* Subset of the above: recoveries that landed in Lookup::resolveMethod(), \ - * i.e. faults while symbolicating at dump time rather than while walking a \ - * stack in a signal handler. Counted separately because the two have \ - * different root causes (stale jmethodID / class unload vs. a bad frame \ - * pointer) and would otherwise be indistinguishable. */ \ - X(METHOD_RESOLVE_LONGJMP_RECOVERED, "method_resolve_longjmp_recovered") \ /* Lookup::resolveMethod() calls that ran without siglongjmp protection \ * because no ProfiledThread could be allocated for the dump thread (OOM): \ * there is nowhere to publish a landing pad. Expected to stay at 0. */ \ diff --git a/ddprof-lib/src/main/cpp/threadLocalData.h b/ddprof-lib/src/main/cpp/threadLocalData.h index 52989cceeb..25848879e5 100644 --- a/ddprof-lib/src/main/cpp/threadLocalData.h +++ b/ddprof-lib/src/main/cpp/threadLocalData.h @@ -192,7 +192,7 @@ class ProfiledThread : public ThreadLocalData { u64 park_block_token; int filter_slot_id; uint8_t init_window; - uint8_t signal_depth; + int signal_depth; bool in_critical_section; bool otel_ctx_initialized; u64 otel_local_root_span_id; diff --git a/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp b/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp index ca32cead22..262fc950f3 100644 --- a/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp +++ b/ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp @@ -197,29 +197,4 @@ TEST(ResolveMethodFaultInjectionTest, DisabledBuildResolvesNormally) { EXPECT_FALSE(t->isProtected()); } -#else // __FAULT_INJECTION__ not defined (the default release/debug build). - -// INJECT_CRASH_LIKELY() in resolveMethod() compiles to nothing here (see -// faultInjection.h), so there is nothing to inject -- this is a plain smoke -// test of the same call, kept for two reasons: (1) it documents that the -// call site is inert in this configuration, and (2) a translation unit that -// registers zero gtest tests fails to *link* as its own binary: with no -// TEST/TEST_F in this object file, nothing here pulls a member out of -// -lgtest before -lgtest_main's gtest_main.cc.o (which needs -// testing::InitGoogleTest() etc. from that same archive) is processed, and -// -lgtest is never revisited afterwards. -TEST(ResolveMethodFaultInjectionTest, DisabledBuildResolvesNormally) { - StringDictionary classes; - MethodMap methods; - Lookup lookup(nullptr, &methods, &classes); - - ASGCT_CallFrame frame{}; - frame.bci = 0; - frame.method_id = nullptr; - - MethodInfo* info = lookup.resolveMethod(frame); - ASSERT_NE(info, nullptr); - EXPECT_EQ(info->_type, FRAME_NATIVE); -} - #endif // __FAULT_INJECTION__ From be8c8a5b61f31c7155e4a588b625869d0345d642 Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Fri, 21 Aug 2026 14:42:09 -0400 Subject: [PATCH 19/19] Fix merge --- ddprof-lib/src/main/cpp/guards.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ddprof-lib/src/main/cpp/guards.cpp b/ddprof-lib/src/main/cpp/guards.cpp index 826e109b72..23285702a9 100644 --- a/ddprof-lib/src/main/cpp/guards.cpp +++ b/ddprof-lib/src/main/cpp/guards.cpp @@ -49,7 +49,11 @@ bool isInTrackedSignalContext() { // null ProfiledThread = no thread context; the SignalHandlerScope // never ran, so we have no positive evidence of a signal frame. // See header comment for the rationale of returning false here. - return pt != nullptr && pt->signalDepth() != 0; + // `> 0` rather than `!= 0`: a negative depth is a pairing bug (an + // unmatched signalHandlerUnwindAfterLongjmp()), not evidence of being in + // a signal handler, and must not pin dlopen_hook to the deferred-refresh + // path for the rest of the thread's life. + return pt != nullptr && pt->signalDepth() > 0; } SignalHandlerScope::SignalHandlerScope(bool shouldRunPriming) : _current(nullptr), _active(true) {