Skip to content

[fault-injection] Profiler::recordSample() stack walk is unprotected#680

Draft
zhengyu123 wants to merge 29 commits into
mainfrom
zgu/unprotected_stackwalk_recordSample
Draft

[fault-injection] Profiler::recordSample() stack walk is unprotected#680
zhengyu123 wants to merge 29 commits into
mainfrom
zgu/unprotected_stackwalk_recordSample

Conversation

@zhengyu123

@zhengyu123 zhengyu123 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?:

Motivation:

Additional Notes:

How to test the change?:

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-15447

Unsure? Have a question? Request a review!

zhengyu123 and others added 16 commits July 20, 2026 19:39
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 22, 2026 12:47

Copilot AI 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.

Pull request overview

This PR hardens the native profiler’s initialization and stack-walking paths against startup-ordering issues (Agent_OnLoad) and fault/crash scenarios during sampling, and adds a regression test to prevent reintroducing the initialization regression.

Changes:

  • Add setjmp/longjmp crash protection around Profiler::recordSample()’s stack unwind to avoid unrecoverable faults during metadata reads.
  • Ensure HotSpot VMStructs initialization happens early enough for the -agentpath: (Agent_OnLoad) path by calling VM::ready() from VM::initProfilerBridge(), and add debug test logging for the decision points.
  • Add a Java regression test that launches a child JVM with -agentpath: and asserts the DebugNonSafepoints flag lookup path is taken (not the CompiledMethodLoad workaround).

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
ddprof-test/src/test/java/com/datadoghq/profiler/JVMAccessTest.java Adds regression test covering Agent_OnLoad initialization ordering and VM flag detection.
ddprof-lib/src/main/java/com/datadoghq/profiler/LibraryLoader.java Exposes a package-visible library-path resolver for tests to supply a real -agentpath: without loading in-process.
ddprof-lib/src/main/cpp/vmEntry.h Reworks _libjvm handling to a CodeCache* with an accessor using acquire semantics.
ddprof-lib/src/main/cpp/vmEntry.cpp Makes VM::ready() idempotently initialize VMStructs once; calls ready() from initProfilerBridge() for Agent_OnLoad; adds test logs.
ddprof-lib/src/main/cpp/profiler.cpp Wraps native+Java stack walking in recordSample() with crash protection and records an error frame on recovery; switches debug-symbol query to VM::libjvm().
ddprof-lib/src/main/cpp/hotspot/vmStructs.inline.h Adds safe offset-load helpers and uses SafeAccess for VMFlag name reads; introduces fault-injection point for offset access.
ddprof-lib/src/main/cpp/hotspot/vmStructs.h Moves offset accessors to inline definitions and adds a safe-load template; updates VMFlag::name() signature.
ddprof-lib/src/main/cpp/hotspot/vmStructs.cpp Avoids repeated unsafe dereferences by caching VMFlag::name() into a local before strcmp.
ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp Adds a null-check for scope.method() and renames the recovered-longjmp counter to be stackwalk-generic.
ddprof-lib/src/main/cpp/flightRecorder.cpp Switches debug-symbol setting emission to use VM::libjvm().
ddprof-lib/src/main/cpp/counters.h Renames WALKVM_LONGJMP_RECOVERED to STACKWALK_LONGJMP_RECOVERED.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dd-octo-sts

dd-octo-sts Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

CI Test Results

Run: #29944756344 | Commit: 2c6b4bc | Duration: 17m 18s (longest job)

7 of 32 test jobs failed

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

Failed Tests

glibc-aarch64/debug / 11-j9

Job: View logs

No detailed failure information available. Check the job logs.

glibc-aarch64/debug / 17-j9

Job: View logs

No detailed failure information available. Check the job logs.

glibc-aarch64/debug / 8-j9

Job: View logs

No detailed failure information available. Check the job logs.

glibc-amd64/debug / 8-ibm

Job: View logs

No detailed failure information available. Check the job logs.

glibc-amd64/debug / 8-j9

Job: View logs

No detailed failure information available. Check the job logs.

glibc-amd64/debug / 17-j9

Job: View logs

No detailed failure information available. Check the job logs.

glibc-amd64/debug / 11-j9

Job: View logs

No detailed failure information available. Check the job logs.

Summary: Total: 32 | Passed: 25 | Failed: 7


Updated: 2026-07-22 18:20:40 UTC

Copilot AI review requested due to automatic review settings July 22, 2026 13:29

Copilot AI 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.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Comment thread ddprof-lib/src/main/cpp/vmEntry.cpp
Comment thread ddprof-lib/src/main/cpp/vmEntry.cpp
Comment thread ddprof-lib/src/main/cpp/profiler.cpp Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 22, 2026 14:03
@dd-octo-sts

dd-octo-sts Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Benchmark Results (commit 3039d1a)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126243478 Commit: 3039d1aaedaee363326bea17a7c8b724984af1b2

⚠️ Significant outliers

  • 🟢 future-genetic (JDK 25): runtime -3.2% (2127→2058 ms)
Runtime details (per benchmark × JDK)
Benchmark JDK Latest Dev Δ (dev vs latest) Issues L/D
akka-uct 21 ✅ 10098 ms (21 iters) ✅ 10322 ms (21 iters) ≈ +2.2% (±12.1%) — / —
finagle-chirper 25 ✅ 5505 ms (36 iters) ✅ 5467 ms (36 iters) ≈ -0.7% (±24.6%) ⚠️ W:3 / ⚠️ W:3
fj-kmeans 21 ✅ 2709 ms (70 iters) ✅ 2752 ms (68 iters) ≈ +1.6% (±2.8%) — / —
fj-kmeans 25 ✅ 2838 ms (66 iters) ✅ 2775 ms (67 iters) ≈ -2.2% (±2.7%) — / —
future-genetic 21 ✅ 2118 ms (88 iters) ✅ 2103 ms (88 iters) ≈ -0.7% (±2.7%) — / —
future-genetic 25 ✅ 2127 ms (88 iters) ✅ 2058 ms (90 iters) 🟢 -3.2% — / —
naive-bayes 21 ✅ 1275 ms (134 iters) ✅ 1299 ms (132 iters) ≈ +1.9% (±32.9%) — / —
naive-bayes 25 ✅ 1026 ms (167 iters) ✅ 1013 ms (169 iters) ≈ -1.3% (±31.2%) — / —
reactors 21 ✅ 17105 ms (15 iters) ✅ 16884 ms (15 iters) ≈ -1.3% (±6.3%) — / —
Internal counter details (ddprof)

ddprof internal counters, latest / dev (✅ = 0, · = unavailable):

Benchmark JDK Dropped rec Dropped jvmti Dropped trace Skipped WC AGCT fail Unwind fail
akka-uct 21 ✅ / ✅ ✅ / ✅ 3 / ✅ 1909 / 1998 ✅ / ✅ ✅ / ✅
akka-uct 25 ✅ / ✅ ✅ / ✅ ✅ / ✅ 2222 / 2288 ✅ / ✅ ✅ / ✅
finagle-chirper 21 ✅ / ✅ ✅ / ✅ 4 / 2 8808 / 8669 ✅ / ✅ ✅ / ✅
finagle-chirper 25 ✅ / ✅ ✅ / ✅ 1 / ✅ 8595 / 8112 ✅ / ✅ ✅ / ✅
fj-kmeans 21 ✅ / ✅ ✅ / ✅ 5 / 4 1289 / 1296 ✅ / ✅ ✅ / ✅
fj-kmeans 25 ✅ / ✅ ✅ / ✅ 3 / 3 1267 / 1292 ✅ / ✅ ✅ / ✅
future-genetic 21 ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅
future-genetic 25 ✅ / ✅ ✅ / ✅ 3 / ✅ 2940 / 2869 ✅ / ✅ ✅ / ✅
naive-bayes 21 ✅ / ✅ ✅ / ✅ 3 / 3 3549 / 3484 ✅ / ✅ ✅ / ✅
naive-bayes 25 ✅ / ✅ ✅ / ✅ 5 / 4 3503 / 3493 ✅ / ✅ ✅ / ✅
reactors 21 ✅ / ✅ ✅ / ✅ ✅ / 1 1751 / 1823 ✅ / ✅ ✅ / ✅
reactors 25 ✅ / ✅ ✅ / ✅ 1 / ✅ 1836 / 1828 ✅ / ✅ ✅ / ✅

Copilot AI 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.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Comment thread ddprof-lib/src/main/cpp/profiler.cpp Outdated
Comment thread ddprof-lib/src/main/cpp/profiler.cpp Outdated
Comment thread ddprof-lib/src/main/cpp/vmEntry.h
Comment thread ddprof-lib/src/main/cpp/profiler.cpp
Comment thread ddprof-lib/src/main/cpp/flightRecorder.cpp
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 22, 2026 15:06
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@dd-octo-sts

dd-octo-sts Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Reliability & Chaos Results

All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/126327649

…ataDog/java-profiler into zgu/unprotected_stackwalk_recordSample

Copilot AI 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.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 22, 2026 15:12

Copilot AI 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.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comment thread ddprof-lib/src/main/cpp/vmEntry.cpp Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 22, 2026 15:24

Copilot AI 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.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

ddprof-lib/src/main/cpp/vmEntry.cpp:222

  • Typo in comment: "libaray" -> "library".
  // The libaray must have been loaded. Otherwise, we cannot get to here due
  // to JVM initialization

Comment thread ddprof-lib/src/main/cpp/profiler.cpp
Comment thread ddprof-lib/src/main/cpp/profiler.cpp
@datadog-official

This comment has been minimized.

@datadog-official

datadog-official Bot commented Jul 22, 2026

Copy link
Copy Markdown

Bits has a CI fix ready

🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready

Profiler::recordSample left its unwind-recovery branch unclosed, causing following member definitions to be parsed in function scope. Added the missing closing brace in ddprof-lib/src/main/cpp/profiler.cpp.

Commit fix to this PR


View in Datadog | Reviewed commit 4403952 · Any feedback? Reach out in #deveng-pr-agent

Copilot AI review requested due to automatic review settings July 22, 2026 17:43

Copilot AI 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.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

ddprof-lib/src/main/cpp/vmEntry.cpp:223

  • Typo in comment: "libaray" should be "library".
  // The libaray must have been loaded. Otherwise, we cannot get to here due
  // to JVM initialization
  assert(lib != nullptr && "JVM library must be loaded");

Comment thread ddprof-lib/src/main/cpp/profiler.cpp Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 22, 2026 17:56
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

ddprof-lib/src/main/cpp/vmEntry.cpp:222

  • Typo in comment: "libaray" should be "library" (and the sentence can be tightened).
  // The library must have been loaded. Otherwise, we cannot get to here due
  // to JVM initialization

Copilot AI review requested due to automatic review settings July 22, 2026 18:00

Copilot AI 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.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Comment on lines 336 to 338
TEST(CheckFaultGuardTest, NullThreadIsNoop) {
HotspotSupport::checkFault(nullptr); // must not crash
Profiler::checkFault(nullptr); // must not crash
}
@dd-octo-sts

dd-octo-sts Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Benchmark Results (commit a66e913)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126327661 Commit: a66e913022c97e8eb8ff017b544a817828030288

⚠️ Significant outliers

  • 🔴 future-genetic (JDK 25): runtime +4.8% (2040→2138 ms)
Runtime details (per benchmark × JDK)
Benchmark JDK Latest Dev Δ (dev vs latest) Issues L/D
akka-uct 21 ✅ 10146 ms (21 iters) ✅ 10256 ms (21 iters) ≈ +1.1% (±11.4%) — / —
akka-uct 25 ✅ 8891 ms (24 iters) ✅ 8863 ms (24 iters) ≈ -0.3% (±11.1%) — / —
finagle-chirper 21 ✅ 5943 ms (33 iters) ✅ 5983 ms (33 iters) ≈ +0.7% (±25.5%) ⚠️ W:3 / ⚠️ W:3
finagle-chirper 25 ✅ 5507 ms (36 iters) ✅ 5417 ms (36 iters) ≈ -1.6% (±24.3%) ⚠️ W:3 / ⚠️ W:4
fj-kmeans 21 ✅ 2773 ms (68 iters) ✅ 2772 ms (67 iters) ≈ -0% (±2.7%) — / —
fj-kmeans 25 ✅ 2823 ms (66 iters) ✅ 2779 ms (67 iters) ≈ -1.6% (±2.7%) — / —
future-genetic 21 ✅ 2128 ms (87 iters) ✅ 2093 ms (88 iters) ≈ -1.6% (±2.7%) — / —
future-genetic 25 ✅ 2040 ms (90 iters) ✅ 2138 ms (87 iters) 🔴 +4.8% — / —
naive-bayes 21 ✅ 1323 ms (130 iters) ✅ 1267 ms (135 iters) ≈ -4.2% (±31.9%) — / —
naive-bayes 25 ✅ 1017 ms (168 iters) ✅ 1015 ms (168 iters) ≈ -0.2% (±31.6%) — / —
reactors 21 ✅ 16518 ms (15 iters) ✅ 16995 ms (15 iters) ≈ +2.9% (±6.4%) — / —
reactors 25 ✅ 18203 ms (15 iters) ✅ 18574 ms (15 iters) ≈ +2% (±5.3%) — / —
Internal counter details (ddprof)

ddprof internal counters, latest / dev (✅ = 0, · = unavailable):

Benchmark JDK Dropped rec Dropped jvmti Dropped trace Skipped WC AGCT fail Unwind fail
akka-uct 21 ✅ / ✅ ✅ / ✅ 4 / 3 1921 / 1981 ✅ / ✅ ✅ / ✅
akka-uct 25 ✅ / ✅ ✅ / ✅ 2 / 1 2429 / 2360 ✅ / ✅ ✅ / ✅
finagle-chirper 21 ✅ / ✅ ✅ / ✅ 2 / 4 8337 / 8708 ✅ / ✅ ✅ / ✅
finagle-chirper 25 ✅ / ✅ ✅ / ✅ ✅ / 2 8615 / 8139 ✅ / ✅ ✅ / ✅
fj-kmeans 21 ✅ / ✅ ✅ / ✅ 4 / 1 1264 / 1262 ✅ / ✅ ✅ / ✅
fj-kmeans 25 ✅ / ✅ ✅ / ✅ 1 / ✅ 1284 / 1266 ✅ / ✅ ✅ / ✅
future-genetic 21 ✅ / ✅ ✅ / ✅ 2 / ✅ 2972 / 2957 ✅ / ✅ ✅ / ✅
future-genetic 25 ✅ / ✅ ✅ / ✅ 2 / 3 2870 / 2948 ✅ / ✅ ✅ / ✅
naive-bayes 21 ✅ / ✅ ✅ / ✅ 3 / 2 3511 / 3493 ✅ / ✅ ✅ / ✅
naive-bayes 25 ✅ / ✅ ✅ / ✅ 5 / 3 3481 / 3475 ✅ / ✅ ✅ / ✅
reactors 21 ✅ / ✅ ✅ / ✅ 1 / ✅ 1791 / 1687 ✅ / ✅ ✅ / ✅
reactors 25 ✅ / ✅ ✅ / ✅ 2 / 1 1829 / 1904 ✅ / ✅ ✅ / ✅

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.

2 participants