From ca9643fbd4b216b8ac465b816271dc0b7106531e Mon Sep 17 00:00:00 2001 From: Zhengyu Gu Date: Wed, 12 Aug 2026 12:50:42 +0000 Subject: [PATCH 1/5] 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 2/5] 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 3/5] 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 4/5] 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 5/5] 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;