Protect Lookup::resolveMethod() with siglongjmp - #735
Draft
zhengyu123 wants to merge 5 commits into
Draft
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
Bits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready
View in Datadog | Reviewed commit 9187828 · Any feedback? Reach out in #deveng-pr-agent |
Contributor
CI Test ResultsRun: #31658121899 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-08-13 01:52:53 UTC |
zhengyu123
requested review from
jbachorik,
kaahos and
rkennke
and
a lite review from Copilot
and removed request for
jbachorik
August 12, 2026 23:50
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to improve profiler stability during JFR dump-time symbolication by recovering from faults in Lookup::resolveMethod() using sigsetjmp/siglongjmp instead of crashing the process, and adds fault-injection utilities/tests to exercise the recovery path.
Changes:
- Add crash-style fault injection (
INJECT_CRASH_*/crashNow()) and use it to test dump-time recovery. - Introduce
sigsetjmpprotection insideLookup::resolveMethod()and update signal-scope depth handling/assertions. - Add/adjust C++ gtest coverage around signal-safety invariants and resolveMethod crash recovery.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ddprof-lib/src/main/cpp/flightRecorder.cpp | Adds sigsetjmp/siglongjmp-based crash recovery around Lookup::resolveMethod() and extra debug logging. |
| ddprof-lib/src/main/cpp/threadLocalData.h | Changes signal-depth storage and updates enter/exit APIs to use atomic operations and stricter assertions. |
| ddprof-lib/src/main/cpp/guards.h | Adds debug-only state to SignalHandlerScope (for depth consistency checks). |
| ddprof-lib/src/main/cpp/guards.cpp | Adds debug asserts to validate signal-depth pairing and includes assertion support. |
| ddprof-lib/src/main/cpp/faultInjection.h | Adds crash-injection macros and declares crashNow(). |
| ddprof-lib/src/main/cpp/faultInjection.cpp | Implements crashNow() for deterministic SIGSEGV triggering. |
| ddprof-lib/src/main/cpp/profiler.h | Switches forced-crash path to use crashNow() instead of manual null deref. |
| ddprof-lib/src/main/cpp/common.h | Adds DEBUG_ONLY(...) helper macro. |
| ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp | New unit test validating resolveMethod recovery under crash-injection builds. |
| ddprof-lib/src/test/cpp/signalSafety_ut.cpp | Updates a signal-safety test to require abort on unmatched unwind call. |
Suppressed comments (4)
ddprof-lib/src/main/cpp/flightRecorder.cpp:589
- In the sigsetjmp recovery path, this uses MethodMap::operator[] to fetch/create the UNKNOWN row after restoring the previous jmp ctx. std::map::operator[] may allocate and throw (or fault) while no crash protection is armed, which undermines the goal of making the landing pad non-faulting/non-allocating.
}
[[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
ddprof-lib/src/main/cpp/flightRecorder.cpp:641
- mi->_mark is set before running fill*MethodInfo(). If a SIGSEGV occurs mid-fill (the stated motivation for adding siglongjmp protection), the longjmp will leave a marked but partially/default-filled MethodInfo in the map. Subsequent frames with the same key will skip filling (!mi->_mark is false) and writeMethods() may serialize a corrupted method entry.
// 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);
ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp:131
- This test asserts methods.size() == 1, which bakes in that resolving an "unknown" method creates an entry in MethodMap. The PR description states the recovery landing pad should avoid map insertion/allocations by using a shared unknown row outside the map; if that is still the goal, this assertion should be updated to validate the non-allocating behavior instead.
// 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);
ddprof-lib/src/main/cpp/flightRecorder.cpp:600
- ProfiledThread::_jmp_buf is set to point at the stack-local crash_protection_ctx here, and is only restored at the very end of resolveMethod() (flightRecorder.cpp:704). If any C++ exception is thrown between these points (e.g., std::map::operator[] allocation), the restore is skipped and the thread retains a dangling longjmp target.
// fillJavaMethodInfo(). Keep the frame structurally intact, but serialize it
// as the shared unknown method.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
ddprof-lib/src/test/cpp/resolveMethodFaultInjection_ut.cpp:16
- This test uses relative includes into src/main/cpp (e.g. "../../main/cpp/flightRecorder.h"), but other gtests include main headers via the configured include paths (e.g. ddprof-lib/src/test/cpp/jvmSupport_ut.cpp:8-13). Keeping relative paths here is brittle (directory moves break it) and inconsistent with existing tests.
#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"
ddprof-lib/src/main/cpp/faultInjection.cpp:29
- The comment says the translation unit is empty unless fault injection is enabled, but crashNow() is now defined unconditionally above. This makes the comment inaccurate and can confuse readers about what gets linked into non-fault-injection builds.
// The whole translation unit is empty unless fault injection is enabled, so a
// normal build links a no-op object file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
Dump-time symbolication reads VM metadata (jmethodID → Method* → class/name/signature) that a concurrent class unload may already have freed. Until now a fault there killed the process. This wraps the faulting region in a sigsetjmp/siglongjmp window that Profiler::checkFault() jumps back through, collapsing the frame onto a shared "unknown method" row instead of crashing.
How the protection is structured
resolveMethod() splits into three parts:
Restoring the previous landing pad is now RAII (JmpCtxScope, new in guards.h), not hand-rolled per-return. Hand-rolled restore misses the returns the author didn't think of — notably a std::bad_alloc from a map or dictionary insert, which would leave _jmp_buf pointing into a dead stack frame for the next checkFault() to jump into. The guard's members are const and set before sigsetjmp, and install()/restore() mutate only the ProfiledThread, so nothing the landing pad reads is subject to the "non-volatile local modified after setjmp is indeterminate" rule.
_unknown_method lives outside MethodMap — and that has a second half
The landing pad runs with protection already disarmed, so it must not allocate: a second fault there is unrecoverable. MethodMap::operator[] allocates a node and can throw, so the shared unknown row is a plain Lookup member, filled once per chunk before arming (one flag test per call thereafter). The landing pad just returns &_unknown_method.
Reviewers please check the coupling this creates: because the row is outside the map, writeMethods()' map walk cannot see it, but writeStackTraces() still writes its _key for every frame that resolved to it. writeMethods() therefore counts and emits it explicitly. Without that, every chunk containing an unresolvable frame would carry a dangling method-pool reference — and silently, since jafar's ConstantPool.get() returns null and such frames render nameless rather than failing. Its id comes from MethodMap::unknownMethodId(): drawn from the normal allocId() counter once per recording so it cannot collide with a map row, and never recycled since cleanupUnreferencedMethods() only frees ids of entries it erases.
_mark is now set last, after the fill
Previously mi->_mark = true preceded the fill. A fault mid-fillJavaMethodInfo() left the row marked but default-constructed — writeMethods() serializes any marked row, so the chunk gained a method with empty class/name/sig typed FRAME_INTERPRETED, and every later frame with that key reused it. Marked last, the row stays unmarked, is skipped by writeMethods(), retried by the next frame needing it, and aged out by cleanupUnreferencedMethods() (which recycles its _key).
This cannot itself produce a dangling reference: the only ways to skip the mark are siglongjmp or an exception, and in both cases mi never reaches the caller. (fillJavaMethodInfo()'s PushLocalFrame early-return still marks and emits a zero-filled row — pre-existing, returns normally, and yields a nameless rather than dangling frame.)
Signal-depth invariant
exitSignalScope() keeps its strict assert(depth > 0). The invariant is structurally guaranteed: checkFault() is the only thing that siglongjmps past a SignalHandlerScope, it has exactly one caller (crashHandlerInternal), reached only from segvHandler/busHandler, both of which open SIGNAL_HANDLER_GUARD() first.
That assert is compiled out under -DNDEBUG, so the reader is hardened instead: isInTrackedSignalContext() tests > 0 rather than != 0, bounding a hypothetical release underflow to the one bad decrement instead of latching dlopen_hook onto the deferred-refresh path for the thread's life.
Perf
The fast path (!isRawPointer && bci != BCI_VTABLE_RECEIVER, row already marked) skips protection entirely, removing three syscalls per frame per trace — initCurrentThreadSignalSafe()'s signal block/unblock plus sigsetjmp(..., 1)'s mask read. Safe because MethodMap::makeKey() reads the union's pointer value and never dereferences it, so key computation touches no VM metadata for any bci except the two excluded ones.
I deliberately did not hoist the context to writeStackTraces(), which was suggested during review: it would trade away the per-frame granularity this PR exists to provide, and partial frame data is already in the buffer by then, so recovery would corrupt the chunk.
New counters
METHOD_RESOLVE_LONGJMP_RECOVERED (subset of STACKWALK_LONGJMP_RECOVERED, isolating dump-time symbolication faults from stack-walk faults — different root causes) and METHOD_RESOLVE_UNPROTECTED (resolves that ran unprotected because no ProfiledThread could be allocated; expected to stay at 0).
On that OOM path resolveMethod() resolves unprotected rather than returning nullptr — both call sites in writeStackTraces() dereference the result unconditionally, so nullptr would convert a transient allocation failure into a SIGSEGV on the dump thread.
Tests
resolveMethodFaultInjection_ut — verifies INJECT_CRASH_LIKELY() in fillMethod() is recovered rather than fatal. Uses a BCI_ERROR frame (reaches the protected window, needs no live JVM) and asserts the recovery returns &lookup._unknown_method with the map untouched, and that the jmp ctx is restored on both paths. Has a non-FI #else branch: a translation unit registering zero tests fails to link, since nothing pulls a member out of -lgtest before -lgtest_main is processed.
signalSafety_ut — EXPECT_DEATH for an unmatched compensating call; new NegativeDepthIsNotTrackedSignalContext covering the release-build > 0 reader (verified non-vacuous by reverting the predicate to != 0 and watching it fail).
MethodIdReuseTest — gained a method-pool referential-integrity oracle beside its duplicate-id one: every method id a stack frame references must have been emitted in that chunk. Reuses the existing raw-chunk walker; guarded against false positives from its early bail-out, which matters because T_STACK_TRACE is serialized before T_METHOD. Nothing previously checked this.
Motivation:
Improve stability - avoid dump-time symbol resolution to crash application.
Additional Notes:
How to test the change?:
For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!