[fault-injection] Profiler::recordSample() stack walk is unprotected#680
[fault-injection] Profiler::recordSample() stack walk is unprotected#680zhengyu123 wants to merge 29 commits into
Conversation
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>
There was a problem hiding this comment.
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
VMStructsinitialization happens early enough for the-agentpath:(Agent_OnLoad) path by callingVM::ready()fromVM::initProfilerBridge(), and add debug test logging for the decision points. - Add a Java regression test that launches a child JVM with
-agentpath:and asserts theDebugNonSafepointsflag 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.
CI Test ResultsRun: #29944756344 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Testsglibc-aarch64/debug / 11-j9Job: View logs No detailed failure information available. Check the job logs. glibc-aarch64/debug / 17-j9Job: View logs No detailed failure information available. Check the job logs. glibc-aarch64/debug / 8-j9Job: View logs No detailed failure information available. Check the job logs. glibc-amd64/debug / 8-ibmJob: View logs No detailed failure information available. Check the job logs. glibc-amd64/debug / 8-j9Job: View logs No detailed failure information available. Check the job logs. glibc-amd64/debug / 17-j9Job: View logs No detailed failure information available. Check the job logs. glibc-amd64/debug / 11-j9Job: 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 |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Benchmark Results (commit 3039d1a)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126243478 Commit:
|
| 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%) | |
| 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 | ✅ / ✅ | ✅ / ✅ |
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>
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
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
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
This comment has been minimized.
This comment has been minimized.
Bits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready
View in Datadog | Reviewed commit 4403952 · Any feedback? Reach out in #deveng-pr-agent |
There was a problem hiding this comment.
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");
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>
There was a problem hiding this comment.
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
| TEST(CheckFaultGuardTest, NullThreadIsNoop) { | ||
| HotspotSupport::checkFault(nullptr); // must not crash | ||
| Profiler::checkFault(nullptr); // must not crash | ||
| } |
Benchmark Results (commit a66e913)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126327661 Commit:
|
| 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%) | |
| finagle-chirper | 25 | ✅ 5507 ms (36 iters) | ✅ 5417 ms (36 iters) | ≈ -1.6% (±24.3%) | |
| 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 | ✅ / ✅ | ✅ / ✅ |
What does this PR do?:
Motivation:
Additional Notes:
How to test the change?:
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!