Protect Lookup::resolveMethod() with siglongjmp - #752
Conversation
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
CI Test ResultsRun: #33075504486 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-08-27 13:30:30 UTC |
|
🔗 Commit SHA: 386f48b | Docs | View more details | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f87be0a19a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c875dc9915
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdd76c3424
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
A few nits and clarifying questions |
6 similar comments
|
A few nits and clarifying questions |
|
A few nits and clarifying questions |
|
A few nits and clarifying questions |
|
A few nits and clarifying questions |
|
A few nits and clarifying questions |
|
A few nits and clarifying questions |
|
A few nits and clarifying questions |
1 similar comment
|
A few nits and clarifying questions |
|
A few nits and clarifying questions |
2 similar comments
|
A few nits and clarifying questions |
|
A few nits and clarifying questions |
jbachorik
left a comment
jbachorik
left a comment
There was a problem hiding this comment.
AFAICS, the change does what it says and I don't see any red flags.
Are we running it somewhere with fault injection? Just to make sure the recovery is working properly?
@jbachorik Yes, I injected a fault in |
2 participants
What does this PR do?:
Crash protection for method symbolication
Note: This mechanism cannot protect against faults originating from JNI or JVMTI calls, as the faulting PCs are outside the profiler’s address space. Since JNI and JVMTI are public APIs, such faults should generally not occur unless the APIs are misused.
Split
resolveMethod()into a fast, deliberately-unprotected path (already-marked cache hit — provably non-faulting) and a newfillMethod()slow path, armed with its ownsigsetjmp/JmpCtxScopelanding pad before touching any VM metadata.Added
ResolveMethodState, an RAII holder for the intermediate JNI/JVMTI-owned pointers (_demangled,_class_name,_method_name,_method_signature,_line_number_table) acquired mid-resolution.state.release()frees whatever's still pending — on both the normal path and thesiglongjmprecovery path — viajvmti->Deallocate().Added a shared
_unknown_methodrow (Lookup::unknownMethod()) that lives outsideMethodMap, so the disarmed recovery branch can return a valid, already-marked row without allocating (aMethodMap::operator[]insert can throwstd::bad_alloc, which must not happen with no landing pad installed).fillJavaMethodInfo()now returns bool so a row is only marked/keyed when it was actually filled, preventing a permanently-stuck empty row whenPushLocalFramefails or the JVM isn't in JVMTI_PHASE_START/LIVE.Added
INJECT_CRASH_LIKELY()inside the protected window (compiled only under -PenableFaultInjection) to exercise the recovery path under test.Line-number-table handling
SharedLineNumberTableno longer makes a private malloc'd copy of the JVMTI buffer — it now holds the buffer JVMTI itself returned and frees it viajvmti->Deallocate()instead offree(). (The previous copy-then-free() pairing could crash under -XX:NativeMemoryTracking, since HotSpot's Allocate/Deallocate route through NMT-tracked os::malloc/os::free, which add a header free() doesn't know about.)Validation before trusting the buffer is now: a sanity bound on the reported size (MAX_LINE_NUMBER_TABLE_ENTRIES = 65535, the JVM spec's code_length cap) and a
SafeAccessprobe of the pointer —isReadableRange()for a non-zero size,isReadable()for the legitimate zero-size case Hotspot returns (callingisReadableRange(ptr, 0)would trip itsassert(size > 0)).LINE_NUMBER_TABLE_UNREADABLE is now actually incremented when the table is rejected.
Signal-depth bookkeeping
_signal_depthwidened fromuint8_ttoint, andexitSignalScope()/getInSignalDepth()now assert() on an unmatched exit instead of silently clamping at zero — a pairing bug becomes visible in debug/test builds rather than self-healing invisibly.isInTrackedSignalContext()checks> 0rather than!= 0so a (now theoretically possible) negative depth reads as "not in a signal context" rather than permanently pinning dlopen_hook to its synchronous refresh path.Misc
faultInjection.h/.cpp:
crashNow()'s declaration and definition are now consistently guarded behind__FAULT_INJECTION__ || DEBUG; the unused INJECT_CRASH_ALWAYS() macro (previously only defined in the disabled branch) was removed; Profiler's test-only forced-crash path now calls crashNow() instead of a local null-deref.counters.h: added METHOD_RESOLUTION_DROPPED_TLS (dump thread couldn't allocate a ProfiledThread, expected to stay 0); trimmed the METHOD_RESOLVE_FAULT_RECOVERED comment (previously explained it as a non-additive subset of STACKWALK_LONGJMP_RECOVERED, tied to the old raw-Method*-only code path — worth re-checking whether that non-additive relationship still holds and re-documenting it, since the comment was dropped rather than updated).
Motivation:
Lookup::resolveMethod()resolvesjmethodIDs into MethodInfo rows at JFR dump time by calling JVMTI (GetMethodDeclaringClass,GetClassSignature,GetMethodName,GetLineNumberTable). If the declaring class was unloaded between sample capture and dump, these calls can return stale/garbage pointers even withJVMTI_ERROR_NONE— production crash telemetry confirmed this on stock HotSpot, not just OpenJ9. This PR wraps that resolution path in asigsetjmp/siglongjmprecovery window (building on the pattern already used forHotspotSupport::resolve(), #743) and fixes a related correctness/lifetime bug in how the JVMTI line-number-table buffer is validated and owned.Additional Notes:
How to test the change?:
New lookup_resolveMethod_ut.cpp: exercises fillNativeMethodInfo()'s demangle-cut-cache-free lifecycle, and fillJavaMethodInfo()'s PushLocalFrame/PopLocalFrame balance (normal return and PushLocalFrame failure) via faked JNI/JVMTI, plus a line-number-table-survives-re-resolve-in-a-later-chunk case.
lineNumberTableCopy_ut.cpp rewritten to match the current hold-not-copy design: rejects an unmapped source, accepts a valid table and confirms it's held (not copied), accepts a zero-size table without tripping the isReadableRange assert, and rejects negative/oversized reported sizes. Ran directly (gtestDebug_lineNumberTableCopy_ut) — all 6 cases pass.
hotspotMethodId_ut.cpp updated: a rejected method id now resolves to the shared _unknown_method row instead of inserting into MethodMap.
signalSafety_ut.cpp: removed the now-invalid "saturates at zero" test, since double-exit is now an assert failure by design, not a saturating no-op.
For Datadog employees:
If this PR touches code that signs or publishes builds or packages, or handles
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.This PR doesn't touch any of that.
JIRA: PROF-15645
Unsure? Have a question? Request a review!