Skip to content

Restore ucontext on faulted paths - #781

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

Restore ucontext on faulted paths#781
zhengyu123 wants to merge 6 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 and adds regression coverage for it:

  • 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). It restores them itself on every normal exit path, but a SIGSEGV that strikes mid-mutation was caught by Profiler::checkFault() and siglongjmp'd straight past those restores — leaving the actual ucontext the kernel uses to resume the sampled thread corrupted.
  • Adds HotspotSupport::withUcontextFaultRecovery(), a reusable sigsetjmp/siglongjmp crash-protection wrapper: it snapshots pc/sp/fp 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 itself into StackFrame::RegisterSnapshot, shared by both getJavaTraceAsync()'s own restore and the new recovery wrapper.
  • A fault recovered after getJavaTraceAsync() has already returned a valid frame count and filled the frame buffer (e.g. inside fillFrameTypes() or the virtual-thread continuation check) now reports that partial trace as truncated instead of discarding it as zero frames.
  • 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. If the resulting SIGSEGV is recovered by checkFault() without putting pc/sp/fp 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).

Additional Notes:

  • withUcontextFaultRecovery()'s recovery branch takes a truncated flag and an optional partial_result out-parameter, mirroring how walkJavaStack() already reported partial/truncated traces before this refactor — no behavior change there beyond fixing the "recovered fault discards an already-valid partial trace" bug described above.
  • 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 new unit tests exercise both sides of that gate via the UNIT_TEST-only Profiler::setAddressRangeForTest().

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.
  • FaultOutsideProfilerRangeIsNotRecoveredAndLeavesUcontextCorrupted — a fault outside the range (standing in for a fault inside libjvm.so) is not recovered, and the mutated ucontext is left as-is.
  • NullUcontextSkipsRestoreWithoutCrashing — a null ucontext (e.g. malloc/socket hooks sampled outside any signal context) doesn't crash the recovery branch.

These drive the real production withUcontextFaultRecovery() through the real Profiler::checkFault(), so a regression to the actual recovery branch (e.g. dropping the ucontext restore) 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

datadog-datadog-prod-us1 Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Pipelines

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 46443bf | Docs | View more details | Give us feedback!

@dd-octo-sts

dd-octo-sts Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Scan-Build Report

User:runner@runnervmejwal
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:Fri Sep 4 18:31:25 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: #33906137112 | Commit: 900dff7 | Duration: 43m 13s (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-04 20:13:50 UTC

@dd-octo-sts

dd-octo-sts Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

All 40 integration tests passed

📊 Dashboard · 👷 Pipeline · 📦 46443bfc

@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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant