Restore ucontext on faulted paths - #781
Open
zhengyu123 wants to merge 25 commits into
Open
zhengyu123 wants to merge 25 commits into
zhengyu123 wants to merge 25 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
Contributor
CI Test ResultsRun: #35347860911 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-18 13:20:49 UTC |
Contributor
zhengyu123
marked this pull request as ready for review
September 4, 2026 20:38
Contributor
There was a problem hiding this comment.
More details
The recovery path restores pc, sp, and fp after a recoverable fault. It also keeps a valid partial trace and marks it as truncated.
🤖 Datadog Autotest · Commit 46443bf · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
rkennke
requested changes
Sep 9, 2026
rkennke
left a comment
Contributor
There was a problem hiding this comment.
🗿 🤖 🔴
Sphinx Review found 1 critical/high severity finding(s) that must be addressed.
Co-authored-by: Roman Kennke <roman.kennke@datadoghq.com>
Co-authored-by: Roman Kennke <roman.kennke@datadoghq.com>
…into zgu/corrupted_rsp
Co-authored-by: Roman Kennke <roman.kennke@datadoghq.com>
…into zgu/corrupted_rsp
Contributor
Author
|
Key additions to what was there before:
|
jbachorik
reviewed
Sep 17, 2026
Collaborator
|
A few explanatory comments, but otherwise looking good |
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?:
Fixes a corrupted-ucontext bug on the async Java stack-walk path, extends the same fault-tolerance to the JavaThread anchor state it also mutates, and adds regression coverage for both:
HotspotSupport::getJavaTraceAsync()mutates the real signalucontext's pc/sp/fp in place while it feeds them to HotSpot's ownAsyncGetCallTrace(viaframe.restore()/unwindStub()/unwindCompiled(), and thePROBE_SPretry loop), and also patches the liveJavaThread'sVMJavaFrameAnchor(setLastJavaPC()) in itsticks_unknown_not_Java/ticks_not_walkable_not_Javaprobes. ASIGSEGVthat strikes mid-mutation is caught byProfiler::checkFault()andsiglongjmp's straight past any in-function restore — leaving the actual ucontext the kernel uses to resume the sampled thread (and/or the live JVM's anchor state) corrupted.HotspotSupport::withUcontextFaultRecovery(), a reusable sigsetjmp/siglongjmp crash-protection wrapper: it snapshots pc/sp/fp (and, if the wrapped work mutates it, the JavaThread anchor) before running the protected work, chains aJmpCtxScopeon the currentProfiledThread, and restores the snapshot in the recovery branch ifcheckFault()recovers a fault.walkJavaStack()now dispatches through it instead of hand-rolling the same protocol inline.StackFrame::RegisterSnapshot(pc/sp/fp) andHotspotStackFrame::RegisterSnapshot(addssaveJavaAnchor()/anchor restore on top), shared by bothgetJavaTraceAsync()and the new recovery wrapper. Both snapshot types now carry a destructor that unconditionally callsrestore()on scope exit, so the restore happens exactly once per call towithUcontextFaultRecovery()regardless of which return path is taken —getJavaTraceAsync()itself no longer needs (and no longer has) explicitctx_snapshot.restore()calls at each of its own return points, since the destructor on the caller-owned snapshot already covers every one of them.SafeAccess::store()/store32()/storePtr()— a write-side counterpart to the existingload()/load32()/loadPtr(), added specifically so the JavaThread anchor'slastJavaPCcan be put back on the recovery path without risking a second fault. The anchor restore inRegisterSnapshot::restore()runs inside (or past) the signal-recovery landing pad itself, where there's no more outer protection layer left to catch a further fault — so that one write goes through the newSafeAccess::storePtr()instead of a raw pointer store. This addssafestore32_impl/safestore64_implassembly stubs (x86_64 + aarch64, Linux + Apple) mirroring the existingsafefetch32_impl/safefetch64_impl, extendsSafeAccess::handle_safefetch()to redirect faults from them, and addsSAFESTORE_FAILED/SAFESTORE_WHILE_PROTECTEDcounters alongside the existingSAFEFETCH_*/SAFECOPY_*ones.VMJavaFrameAnchor::setLastJavaPC()is now templated onSafeStore(defaulting to the safe path); the initial anchor patch ingetJavaTraceAsync()'s probe uses the plain/unguarded store (already covered by the outer sigsetjmp there), while the recovery-path restore uses the safe store.partial_resultout-parameter sowithUcontextFaultRecovery()'s shared recovery branch can still report it as truncated-but-valid — preservingwalkJavaStack()'s pre-existing behavior for a fault recovered aftergetJavaTraceAsync()has already returned frames (e.g. insidefillFrameTypes()or the virtual-thread continuation check), not a new fix.hotspot_crash_protection_ut.cpp'sWalkJavaStackUcontextRestoreTestsuite to call the realHotspotSupport::withUcontextFaultRecovery()directly (driven through the actualProfiler::checkFault(), not a hand-simulatedsiglongjmp), and adds anINJECT_FAULT_ADDRESS_UNLIKELYsite on the unguarded anchor-derivedspdereference ingetJavaTraceAsync()so the recovery path is exercised for real rather than only in the unit test's simulated walk.Motivation:
A profiling signal can interrupt a sampled thread at any point, including mid-way through
getJavaTraceAsync()'s in-place mutation of that thread's own signalucontextor itsJavaThread's anchor. If the resultingSIGSEGVis recovered bycheckFault()without putting pc/sp/fp (and the anchor) back the way they were, the signal handler returns and the kernel resumes the sampled thread with a corrupted register set instead of its real one — a crash indistinguishable from stack corruption in the profiled process itself (see branch name). The anchor restore needed its own fault-tolerant store, rather than a raw write, because that particular restore can itself run on the tail end of a fault-recovery path where nothing is left to catch a second fault.Additional Notes:
withUcontextFaultRecovery()'s recovery branch takes atruncatedflag and apartial_resultout-parameter, mirroring howwalkJavaStack()already reported partial/truncated traces before this refactor — no behavior change there; the out-parameter only exists because the shared recovery branch no longer has direct lexical access tojava_frames.Profiler::checkFault()only recovers faults whose PC falls inside this library's own address range; a fault insidelibjvm.so(e.g.AsyncGetCallTraceitself dereferencing a poisoned sp/pc/fp) is deliberately not recovered here, and the unit tests exercise both sides of that gate via theUNIT_TEST-onlyProfiler::setAddressRangeForTest(). On that un-recovered path,RegisterSnapshot's destructor still restores the ucontext oncewithUcontextFaultRecovery()returns normally — that destructor-driven restore is exactly whatFaultOutsideProfilerRangeIsNotRecoveredButUcontextIsStillRestoredpins.SafeAccess::store()/store32()are currently only exercised by the new unit tests;storePtr()is the one production caller, fromVMJavaFrameAnchor::setLastJavaPC<true>().How to test the change?:
Covered by
hotspot_crash_protection_ut.cpp'sWalkJavaStackUcontextRestoreTestsuite:FaultInsideProfilerRangeRecoversAndRestoresUcontext— a fault inside this library's range is recovered and the ucontext's pc/sp/fp are restored to their pre-walk values.FaultInsideProfilerRangeRecoversAndPreservesPartialResult— the same recovered fault returns whatever partial frame countwork()had already committed, not a hardcoded 0.FaultInsideProfilerRangeRecoversAndRestoresJavaThreadAnchor— a recovered fault also restores the liveJavaThread's anchorlastJavaPCto its pre-mutation value.FaultOutsideProfilerRangeIsNotRecoveredButUcontextIsStillRestored— a fault outside the range (standing in for a fault insidelibjvm.so) is not recovered bycheckFault(), butwithUcontextFaultRecovery()still restores the ucontext on its normal-completion return, viaRegisterSnapshot's destructor.NullUcontextSkipsRestoreWithoutCrashing— a null ucontext (e.g. malloc/socket hooks sampled outside any signal context) doesn't crash the recovery branch.And by new cases in
safefetch_ut.cpp'sSafeFetchTestsuite covering the new write-side primitives: valid/invalid-pointer round-trips forstore32/storePtr/store, plus real SIGSEGV recovery (viamprotect(PROT_READ), to isolate a write fault specifically) forreadOnlyMemoryStore32/readOnlyMemoryStorePtr.These drive the real production
withUcontextFaultRecovery()andSafeAccesscode through the realProfiler::checkFault()/signal handlers, so a regression to the actual recovery branch (e.g. dropping a restore call, or a store silently failing) fails these tests too, not just a hand-rolled replica of the same logic.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!