Skip to content

Restore ucontext on faulted paths - #781

Open
zhengyu123 wants to merge 25 commits into
mainfrom
zgu/corrupted_rsp
Open

zhengyu123 wants to merge 25 commits into
mainfrom
zgu/corrupted_rsp

Conversation

@zhengyu123

@zhengyu123 zhengyu123 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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 signal ucontext's pc/sp/fp in place while it feeds them to HotSpot's own AsyncGetCallTrace (via frame.restore()/unwindStub()/unwindCompiled(), and the PROBE_SP retry loop), and also patches the live JavaThread's VMJavaFrameAnchor (setLastJavaPC()) in its ticks_unknown_not_Java/ticks_not_walkable_not_Java probes. A SIGSEGV that strikes mid-mutation is caught by Profiler::checkFault() and siglongjmp'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.
  • Adds 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 a JmpCtxScope on the current ProfiledThread, and restores the snapshot in the recovery branch if checkFault() recovers a fault. walkJavaStack() now dispatches through it instead of hand-rolling the same protocol inline.
  • Extracts the snapshot/restore boilerplate into StackFrame::RegisterSnapshot (pc/sp/fp) and HotspotStackFrame::RegisterSnapshot (adds saveJavaAnchor()/anchor restore on top), shared by both getJavaTraceAsync() and the new recovery wrapper. Both snapshot types now carry a destructor that unconditionally calls restore() on scope exit, so the restore happens exactly once per call to withUcontextFaultRecovery() regardless of which return path is taken — getJavaTraceAsync() itself no longer needs (and no longer has) explicit ctx_snapshot.restore() calls at each of its own return points, since the destructor on the caller-owned snapshot already covers every one of them.
  • New: SafeAccess::store() / store32() / storePtr() — a write-side counterpart to the existing load()/load32()/loadPtr(), added specifically so the JavaThread anchor's lastJavaPC can be put back on the recovery path without risking a second fault. The anchor restore in RegisterSnapshot::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 new SafeAccess::storePtr() instead of a raw pointer store. This adds safestore32_impl/safestore64_impl assembly stubs (x86_64 + aarch64, Linux + Apple) mirroring the existing safefetch32_impl/safefetch64_impl, extends SafeAccess::handle_safefetch() to redirect faults from them, and adds SAFESTORE_FAILED/SAFESTORE_WHILE_PROTECTED counters alongside the existing SAFEFETCH_*/SAFECOPY_* ones. VMJavaFrameAnchor::setLastJavaPC() is now templated on SafeStore (defaulting to the safe path); the initial anchor patch in getJavaTraceAsync()'s probe uses the plain/unguarded store (already covered by the outer sigsetjmp there), while the recovery-path restore uses the safe store.
  • Threads the already-valid partial frame count through a partial_result out-parameter so withUcontextFaultRecovery()'s shared recovery branch can still report it as truncated-but-valid — preserving walkJavaStack()'s pre-existing behavior for a fault recovered after getJavaTraceAsync() has already returned frames (e.g. inside fillFrameTypes() or the virtual-thread continuation check), not a new fix.
  • Rewrites hotspot_crash_protection_ut.cpp's WalkJavaStackUcontextRestoreTest suite to call the real HotspotSupport::withUcontextFaultRecovery() directly (driven through the actual Profiler::checkFault(), not a hand-simulated siglongjmp), and adds an INJECT_FAULT_ADDRESS_UNLIKELY site on the unguarded anchor-derived sp dereference in getJavaTraceAsync() 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 signal ucontext or its JavaThread's anchor. If the resulting SIGSEGV is recovered by checkFault() 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 a truncated flag and a partial_result out-parameter, mirroring how walkJavaStack() 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 to java_frames.
  • The crash-protection gate in Profiler::checkFault() only recovers faults whose PC falls inside this library's own address range; a fault inside libjvm.so (e.g. AsyncGetCallTrace itself dereferencing a poisoned sp/pc/fp) is deliberately not recovered here, and the unit tests exercise both sides of that gate via the UNIT_TEST-only Profiler::setAddressRangeForTest(). On that un-recovered path, RegisterSnapshot's destructor still restores the ucontext once withUcontextFaultRecovery() returns normally — that destructor-driven restore is exactly what FaultOutsideProfilerRangeIsNotRecoveredButUcontextIsStillRestored pins.
  • SafeAccess::store()/store32() are currently only exercised by the new unit tests; storePtr() is the one production caller, from VMJavaFrameAnchor::setLastJavaPC<true>().

How to test the change?:
Covered by hotspot_crash_protection_ut.cpp's WalkJavaStackUcontextRestoreTest suite:

  • 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 count work() had already committed, not a hardcoded 0.
  • FaultInsideProfilerRangeRecoversAndRestoresJavaThreadAnchor — a recovered fault also restores the live JavaThread's anchor lastJavaPC to its pre-mutation value.
  • FaultOutsideProfilerRangeIsNotRecoveredButUcontextIsStillRestored — a fault outside the range (standing in for a fault inside libjvm.so) is not recovered by checkFault(), but withUcontextFaultRecovery() still restores the ucontext on its normal-completion return, via RegisterSnapshot'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's SafeFetchTest suite covering the new write-side primitives: valid/invalid-pointer round-trips for store32/storePtr/store, plus real SIGSEGV recovery (via mprotect(PROT_READ), to isolate a write fault specifically) for readOnlyMemoryStore32/readOnlyMemoryStorePtr.

These drive the real production withUcontextFaultRecovery() and SafeAccess code through the real Profiler::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:

  • 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-review
    skill, or file a request via the PSEC review form).
    bewaire also runs automatically on every PR.
  • This PR doesn't touch any of that.
  • JIRA: PROF-15903

Unsure? Have a question? Request a review!

@datadog-datadog-prod-us1

This comment has been minimized.

@dd-octo-sts

dd-octo-sts Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Scan-Build Report

User:runner@runnervmlun5p
Working Directory:/home/runner/work/java-profiler/java-profiler/ddprof-lib/src/test/make
Command Line:make -j4 all
Clang Version:Ubuntu clang version 18.1.3 (1ubuntu1)
Date:Thu Sep 17 14:02:22 2026

Bug Summary

Bug TypeQuantityDisplay?
All Bugs1
Logic error
Dereference of null pointer1

Reports

Bug Group Bug Type ▾ File Function/Method Line Path Length
Logic errorDereference of null pointerfaultInjection.cppcrashNow242

@dd-octo-sts

dd-octo-sts Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

CI Test Results

Run: #35347860911 | Commit: f661b7a | Duration: 15m 38s (longest job)

All 32 test jobs passed

Status Overview

JDK glibc-aarch64/debug glibc-amd64/debug musl-aarch64/debug musl-amd64/debug
8 - - -
8-ibm - - -
8-j9 - -
8-librca - -
8-orcl - - -
11 - - -
11-j9 - -
11-librca - -
17 - -
17-graal - -
17-j9 - -
17-librca - -
21 - -
21-graal - -
21-librca - -
25 - -
25-graal - -
25-librca - -

Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled

Summary: Total: 32 | Passed: 32 | Failed: 0


Updated: 2026-09-18 13:20:49 UTC

@dd-octo-sts

dd-octo-sts Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

All 40 integration tests passed

📊 Dashboard · 👷 Pipeline · 📦 b9e42f57

@zhengyu123
zhengyu123 marked this pull request as ready for review September 4, 2026 20:38
@zhengyu123
zhengyu123 requested a review from a team as a code owner September 4, 2026 20:38

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Datadog Autotest: PASS

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.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Datadog Autotest · Commit 46443bf · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest

@rkennke rkennke added the sphinx:critical Sphinx: critical — human review required label Sep 9, 2026

@rkennke rkennke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗿 🤖 🔴

Sphinx Review found 1 critical/high severity finding(s) that must be addressed.

Comment thread ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp Outdated
Comment thread ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h Outdated
Comment thread ddprof-lib/src/main/cpp/stackFrame.h
Comment thread ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h Outdated
Comment thread ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp Outdated
Comment thread ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp Outdated
Comment thread ddprof-lib/src/main/cpp/hotspot/hotspotSupport.h
Comment thread ddprof-lib/src/main/cpp/stackFrame.h Outdated
Comment thread ddprof-lib/src/main/cpp/stackFrame.h
Comment thread ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp
@zhengyu123

zhengyu123 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Key additions to what was there before:

  • New SafeAccess::store()/store32()/storePtr() section explaining why they exist: the JavaThread anchor's lastJavaPC restore in RegisterSnapshot::restore() runs on the tail end of a fault-recovery path where a second fault has nowhere left to be caught, so that one write now goes through a fault-tolerant store (new safestore32_impl/safestore64_impl asm stubs, handle_safefetch extended to catch their faults, new SAFESTORE_FAILED/SAFESTORE_WHILE_PROTECTED counters) instead of a raw pointer write.
  • setLastJavaPC() now templated on SafeStore, and RegisterSnapshot's destructor now unconditionally restores on scope exit — so the explicit ctx_snapshot.restore() calls scattered through getJavaTraceAsync() were removed as redundant.
  • setLastJavaPC() uses SafeStore on recovery paths to avoid faulting.
  • Updated How to test with the actual current test names (...RestoresJavaThreadAnchor, ...PreservesPartialResult, the renamed ...ButUcontextIsStillRestored) and the new safefetch_ut.cpp store-path coverage.

@zhengyu123
zhengyu123 requested a review from rkennke September 10, 2026 17:59
Comment thread ddprof-lib/src/main/cpp/stackFrame.h Outdated
Comment thread ddprof-lib/src/main/cpp/safeAccess.h
Comment thread ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp Outdated
@jbachorik

Copy link
Copy Markdown
Collaborator

A few explanatory comments, but otherwise looking good

@jbachorik jbachorik left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mergequeue-status: waiting sphinx:critical Sphinx: critical — human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants