Protect HotspotSupport::resolve() with longjmp - #743
Conversation
CI Test ResultsRun: #32369962976 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-08-20 12:56:55 UTC |
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54e771719e
ℹ️ 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".
|
Can readMethodNames() be rewritten as a dedicated SafeAccess-only traversal, rather than installing a broad siglongjmp landing pad? |
|
This effectively protects for crashes, but are we not worried about downstream effects of passing invalid data ? Using data that was freed would not crash but still produce invalid data. |
|
I like the deterministic tests, though have we considered fuzzing for this type of problem ?
|
@r1viollet The goal is the opposite - Once completing |
@r1viollet Yes, this is my main concern - this approach does mask off some bad bugs inside the protected blocks. I am think about setting up a special build, that disables |
@r1viollet Yes, we may need more fuzzing tests. I think fault-injection and fuzzing overlap in certain ways, and fault-injection has certain advantages, please refer to my reply to Thomas' comment here Fault-injection allows fault to propagate through call graph, instead of selected points. |
jbachorik
left a comment
There was a problem hiding this comment.
Nice cleanup! Looks sane to me, there is one minor issue Sphinx found, if you can address it before merge, it would be great, but it is not a high priority.
What does this PR do?:
Hardens
HotspotSupport::resolve()— the dump-time fallback that resolves a rawMethod*to ajmethodIDwhencstack=vm,fjmethodid=false— against crashes from stale JVM metadata, and fixes three separate bugs that caused<clinit>frames captured under that configuration to always serialize as"unknown"instead of resolving correctly:JVMSupport::initExecution()short-circuited on a staleFully_loadedstate left over from a previous session, so a laterfjmethodid=falsestart never re-evaluated whether jmethodID preloading should actually be disabled.<clinit>JVMTI fallback calledGetClassMethodsdirectly instead of going throughloadMethodIDsIfNeededImpl(), skipping the JDK-8062116 classloader-data patch that every other preload path applies.HotspotSupport::fillJavaFrame()readVM::arguments()._force_jmethodID— a globalArgumentsobject that's only updated by the command-line agent-attach path, never by theJavaProfiler.execute()JNI API. Every session started via the JNI API (which is how the test suite, and most embedding use, starts the profiler) therefore saw a permanently stale_force_jmethodID, so un-preloaded methods were always treated as unwalkable and serialized as"unknown"instead of taking the intended raw-Method*fallback path. Fixed by givingProfilerits own_force_jmethodIDmember (mirroring the existing_cstackfield) set from the actual startupArguments.Also adds a malloc-backed fallback for method/class/signature names that don't fit the fixed inline buffers used during crash-protected resolution (previously such names silently resolved as
"unknown"with no fallback), and closes out the correctness gaps that fallback introduced: a double-free on the crash-recovery path (release()is now idempotent), a setjmp/longjmp indeterminate-value issue (the malloc'd pointers are nowvolatile, since they're mutated betweensigsetjmp()and a possiblesiglongjmp()and then read back by the recovery path), and a dead-code fallback (the malloc path was unreachable for class-name/signature since their short-buffer size equaled the hard rejection ceiling — lowered to 1024 so the fallback actually engages).Motivation:
ClinitResolutionTestwas failing: a class's<clinit>frame, sampled while spinning for ~2s undercpu=1ms,cstack=vm,fjmethodid=false, never resolved to its real class/method name in the resulting JFR — it always showed as the shared"unknown"frame. Root-causing this surfaced the three bugs above, none of which were specific to<clinit>— they affect any raw-Method*resolution underfjmethodid=false,<clinit>just happened to be the scenario the test exercised.Additional Notes:
paul.fournillon/jmethod_clinit_fix, adapted to this branch's crash-protection refactor.METHOD_RESOLVE_FAULT_RECOVEREDoverlapsSTACKWALK_LONGJMP_RECOVEREDfor the same fault; nothing enforces the "subtract, never sum" comment, so a naive counter-summing dashboard would double-count.<clinit>fallback'spatchClassLoaderData()call (JDK 8 only) is now reachable from the dump thread as well as fromClassPrepare; a narrow race could invoke it twice for the same class.resolve()'s top-level doc comment ("only resolves system-classloader methods") is slightly stale given the<clinit>fallback'sload_all=true; the constraint still holds in practice via the earlierFindClasscall, so this is cosmetic.How to test the change?:
./gradlew :ddprof-lib:gtestDebug_hotspotSupport_ut :ddprof-lib:gtestDebug_jvmSupport_ut— new/updated unit tests forJVMSupport::initExecution()'s stale-state bug and the<clinit>fallback'sloadMethodIDsIfNeededImpl()routing../gradlew testdebug --tests com.datadoghq.profiler.cpu.ClinitResolutionTest— the regression test for this bug, passes reliably on the first attempt (previously failed even after all 5 retries)../gradlew testdebug --tests "com.datadoghq.profiler.cpu.*"— fullcputest package, includingVtableReceiverFrameTest, all passing.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).
Unsure? Have a question? Request a review!