fix(profiler): guard line-number-table copy against stale jmethodID#647
fix(profiler): guard line-number-table copy against stale jmethodID#647jbachorik wants to merge 2 commits into
Conversation
fillJavaMethodInfo() memcpy'd the JVMTI-returned line-number table without probing it first, unlike the sibling class/method name+sig strings hardened in #537. Crash reported in production as SIGSEGV in Lookup::resolveMethod on stock HotSpot JDK 11.
CI Test ResultsRun: #29115420127 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-10 18:59:40 UTC |
Benchmark Results (commit 36f4468)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124068415 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10326 ms (21 iters) | ✅ 10289 ms (21 iters) | ≈ -0.4% (±11.2%) | — / — |
| akka-uct | 25 | ✅ 8965 ms (24 iters) | ✅ 8824 ms (24 iters) | ≈ -1.6% (±9.7%) | — / — |
| finagle-chirper | 21 | ✅ 6017 ms (33 iters) | ✅ 6003 ms (33 iters) | ≈ -0.2% (±25.4%) | |
| finagle-chirper | 25 | ✅ 5479 ms (36 iters) | ✅ 5525 ms (36 iters) | ≈ +0.8% (±24.9%) | |
| fj-kmeans | 21 | ✅ 2825 ms (66 iters) | ✅ 2711 ms (69 iters) | 🟢 -4% | — / — |
| fj-kmeans | 25 | ✅ 2813 ms (66 iters) | ✅ 2820 ms (66 iters) | ≈ +0.2% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 2126 ms (87 iters) | ✅ 2027 ms (91 iters) | 🟢 -4.7% | — / — |
| naive-bayes | 21 | ✅ 1264 ms (135 iters) | ✅ 1289 ms (133 iters) | ≈ +2% (±33.3%) | — / — |
| naive-bayes | 25 | ✅ 1009 ms (169 iters) | ✅ 1018 ms (168 iters) | ≈ +0.9% (±32.2%) | — / — |
| reactors | 21 | ✅ 16193 ms (15 iters) | ✅ 16743 ms (15 iters) | ≈ +3.4% (±7.3%) | — / — |
| reactors | 25 | ✅ 18502 ms (15 iters) | ✅ 18476 ms (15 iters) | ≈ -0.1% (±4.8%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1952 / 1969 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2077 / 2368 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 4 | 8413 / 8843 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 8590 / 8627 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1261 / 1237 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 4 | 2893 / 2959 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 2868 / 2862 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 3473 / 3538 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 5 | 3507 / 3483 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1661 / 1582 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1975 / 1867 | ✅ / ✅ | ✅ / ✅ |
|
…hodID-churn stress test The EXPECT_DEATH reproducer silently passed on -O3 builds because the optimizer eliminated the dead memcpy/free once owned_table's contents were never observed; a forced volatile read fixes that. Also adds a JUnit stress test that races class-unload churn against profiler dump/resolve and asserts native whitebox counters actually observed a stale jmethodID, not just that the JVM didn't crash. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Benchmark Results (commit e5ef83e)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124087303 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10327 ms (21 iters) | ✅ 10396 ms (21 iters) | ≈ +0.7% (±10.9%) | — / — |
| akka-uct | 25 | ✅ 8806 ms (24 iters) | ✅ 8888 ms (24 iters) | ≈ +0.9% (±11%) | — / — |
| finagle-chirper | 21 | ✅ 5982 ms (33 iters) | ✅ 6066 ms (33 iters) | ≈ +1.4% (±25.2%) | |
| finagle-chirper | 25 | ✅ 5471 ms (36 iters) | ✅ 5502 ms (36 iters) | ≈ +0.6% (±24.4%) | |
| fj-kmeans | 21 | ✅ 2715 ms (68 iters) | ✅ 2762 ms (68 iters) | ≈ +1.7% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2849 ms (66 iters) | ✅ 2840 ms (66 iters) | ≈ -0.3% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2048 ms (91 iters) | ✅ 2129 ms (87 iters) | 🔴 +4% | — / — |
| future-genetic | 25 | ✅ 2085 ms (89 iters) | ✅ 2033 ms (92 iters) | ≈ -2.5% (±2.6%) | — / — |
| naive-bayes | 21 | ✅ 1269 ms (135 iters) | ✅ 1265 ms (135 iters) | ≈ -0.3% (±32.8%) | — / — |
| naive-bayes | 25 | ✅ 1004 ms (170 iters) | ✅ 1017 ms (168 iters) | ≈ +1.3% (±31.7%) | — / — |
| reactors | 21 | ✅ 16602 ms (15 iters) | ✅ 15968 ms (15 iters) | ≈ -3.8% (±7.8%) | — / — |
| reactors | 25 | ✅ 18322 ms (15 iters) | ✅ 18907 ms (15 iters) | ≈ +3.2% (±4.6%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 2184 / 2291 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 8712 / 8658 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 8439 / 8434 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1239 / 1267 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 1288 / 1274 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2973 / 2888 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 2926 / 2837 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 11 / 3 | 3532 / 3526 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 6 / 3 | 3456 / 3462 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1715 / 1428 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1909 / 1913 | ✅ / ✅ | ✅ / ✅ |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5ef83e276
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } else { | ||
| TEST_LOG("Failed to allocate %zu bytes for line number table copy", bytes); | ||
| Counters::increment(LINE_NUMBER_TABLE_UNREADABLE); |
There was a problem hiding this comment.
When GetLineNumberTable returns a pointer that this new guard classifies as unreadable, this branch only increments the counter; line_number_table is still passed to jvmti->Deallocate(...) immediately afterward. This is the same stale/unmapped-pointer scenario the change is trying to survive, and the string path above explicitly nulls bad JVMTI pointers because deallocation can fault too, so the crash can simply move from memcpy to Deallocate. Skip deallocation or clear the pointer on this unreadable path, accepting the small leak as the existing metadata guard does.
Useful? React with 👍 / 👎.
| long unreadableLineTableDelta = after.getOrDefault("line_number_table_unreadable", 0L) | ||
| - before.getOrDefault("line_number_table_unreadable", 0L); | ||
|
|
||
| assertTrue(skippedDelta > 0 || unreadableLineTableDelta > 0, |
There was a problem hiding this comment.
This assertion makes the test fail unless a best-effort 5-second stress window happens to hit the stale-jmethodID race and increments one of these counters. CHURN_ITERATIONS > 0 and a non-empty dump only prove the workload ran, not that class unloading raced with resolveMethod, so a healthy JVM/host combination can leave both deltas at zero and intermittently fail CI. Treat the zero-counter case as an assumption/diagnostic or construct a deterministic unload condition before asserting the guard fired.
Useful? React with 👍 / 👎.
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/124087297 |
1 participant
What does this PR do?:
Guards the JVMTI line-number-table copy in
Lookup::fillJavaMethodInfo(flightRecorder.cpp) withSafeAccess::isReadableRange()beforememcpy-ing it, instead of dereferencing the pointer unconditionally. Adds aLINE_NUMBER_TABLE_UNREADABLEcounter for observability when the guard trips, and a gtest reproducer (lineNumberTableCopy_ut.cpp) that demonstrates the crash pattern and the fix.Motivation:
PROF-15385
Production crash report: SIGSEGV in
__memcpy_evex_unaligned_erms, insideLookup::resolveMethod(inlinedfillJavaMethodInfo), duringFlightRecorder::dump. Confirmed on stock HotSpot JDK 11.fillJavaMethodInfocallsGetLineNumberTable()on ajmethodIDcaptured asynchronously at sample time, which can point at an already-unloaded class by the time the dump thread processes it (the TOCTOU race already documented a few lines above this call, re:GetMethodDeclaringClass). The siblingclass_name/method_name/method_sigstrings returned by the preceding JVMTI calls are already probed withSafeAccess::isReadableRangefor exactly this reason (#537 /ef13aa29f), but the line-number-table pointer was copied unguarded. This PR closes that gap.Additional Notes:
Reproducer isolates the copy step from JVMTI/JNI (impractical to fake a live JVM in a plain gtest) by exercising the identical copy pattern against a real
mmap'd-then-munmap'd page, standing in for "GetLineNumberTable() returned a bad pointer" regardless of the precise reason. Verified independently outside gtest too: the unguarded pattern segfaults (exit 139) and the guarded pattern exits cleanly.How to test the change?:
./gradlew :ddprof-lib:gtestDebug_lineNumberTableCopy_ut— 3 tests: unguarded pattern crashes as expected (EXPECT_DEATH), guarded pattern skips the copy safely when the source is unmapped, guarded pattern still copies correctly for valid data.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!