Guard Recording::writeElement against SIGSEGV on a corrupted metadata tree - #692
Open
jbachorik wants to merge 7 commits into
Open
Guard Recording::writeElement against SIGSEGV on a corrupted metadata tree#692jbachorik wants to merge 7 commits into
jbachorik wants to merge 7 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
CI Test ResultsRun: #30918776776 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 31 | Failed: 0 | Cancelled: 1 Updated: 2026-08-04 17:28:59 UTC |
jbachorik
changed the title
3 tasks
jbachorik
changed the title
jbachorik
changed the title
Contributor
Benchmark Results (commit 3583a36)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127425053 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
Contributor
Benchmark Results (commit 7b6ab83)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127634397 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10334 ms (21 iters) | ✅ 10322 ms (21 iters) | ≈ -0.1% (±11.6%) | — / — |
| akka-uct | 25 | ✅ 8811 ms (24 iters) | ✅ 8871 ms (24 iters) | ≈ +0.7% (±9.7%) | — / — |
| finagle-chirper | 21 | ✅ 5932 ms (33 iters) | ✅ 6005 ms (33 iters) | ≈ +1.2% (±25.4%) | |
| finagle-chirper | 25 | ✅ 5467 ms (36 iters) | ✅ 5423 ms (36 iters) | ≈ -0.8% (±23.9%) | |
| fj-kmeans | 21 | ✅ 2754 ms (68 iters) | ✅ 2695 ms (70 iters) | ≈ -2.1% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2821 ms (66 iters) | ✅ 2811 ms (66 iters) | ≈ -0.4% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2075 ms (90 iters) | ✅ 2120 ms (87 iters) | ≈ +2.2% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 2090 ms (89 iters) | ✅ 2033 ms (91 iters) | 🟢 -2.7% | — / — |
| naive-bayes | 21 | ✅ 1344 ms (128 iters) | ✅ 1307 ms (131 iters) | ≈ -2.8% (±32.2%) | — / — |
| naive-bayes | 25 | ✅ 1001 ms (171 iters) | ✅ 1012 ms (169 iters) | ≈ +1.1% (±31.8%) | — / — |
| reactors | 21 | ✅ 16074 ms (15 iters) | ✅ 16204 ms (15 iters) | ≈ +0.8% (±8%) | — / — |
| reactors | 25 | ✅ 18350 ms (15 iters) | ✅ 18478 ms (15 iters) | ≈ +0.7% (±4.9%) | — / — |
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 / ✅ | 2099 / 2044 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2259 / 2339 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 5 | 8410 / 8407 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 5 / 2 | 8363 / 8727 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 3 | 1298 / 1277 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 10 | 1265 / 1270 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 2895 / 3034 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 2913 / 2904 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 6 | 3528 / 3558 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 3500 / 3475 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 1638 / 1771 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1893 / 1903 | ✅ / ✅ | ✅ / ✅ |
Collaborator
Author
|
@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: 7b6ab83587
ℹ️ 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".
Contributor
Benchmark Results (commit 00070a8)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127722360 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10283 ms (21 iters) | ✅ 10259 ms (21 iters) | ≈ -0.2% (±11.6%) | — / — |
| akka-uct | 25 | ✅ 8938 ms (24 iters) | ✅ 8849 ms (24 iters) | ≈ -1% (±9.1%) | — / — |
| finagle-chirper | 21 | ✅ 6004 ms (33 iters) | ✅ 5945 ms (33 iters) | ≈ -1% (±25%) | |
| finagle-chirper | 25 | ✅ 5529 ms (36 iters) | ✅ 5474 ms (36 iters) | ≈ -1% (±24.5%) | |
| fj-kmeans | 21 | ✅ 2703 ms (70 iters) | ✅ 2704 ms (69 iters) | ≈ +0% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2818 ms (66 iters) | ✅ 2786 ms (67 iters) | ≈ -1.1% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2145 ms (87 iters) | ✅ 2098 ms (90 iters) | ≈ -2.2% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 1986 ms (94 iters) | ✅ 2068 ms (90 iters) | 🔴 +4.1% | — / — |
| naive-bayes | 21 | ✅ 1269 ms (135 iters) | ✅ 1236 ms (138 iters) | ≈ -2.6% (±32%) | — / — |
| naive-bayes | 25 | ✅ 980 ms (174 iters) | ✅ 1017 ms (169 iters) | ≈ +3.8% (±32.5%) | — / — |
| reactors | 21 | ✅ 16812 ms (15 iters) | ✅ 16680 ms (15 iters) | ≈ -0.8% (±8.4%) | — / — |
| reactors | 25 | ✅ 17862 ms (15 iters) | ✅ 18605 ms (15 iters) | ≈ +4.2% (±4.4%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1939 / 2016 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 2347 / 2190 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 8381 / 8397 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 8334 / 8682 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1276 / 1255 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1262 / 1288 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 2919 / 2913 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 3533 / 3499 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 3484 / 3494 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1633 / 1836 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1778 / 1941 | ✅ / ✅ | ✅ / ✅ |
Contributor
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/127721763 |
jbachorik
force-pushed
the
sphinx/fix-jira-PROF-15075
branch
from
August 4, 2026 10:51
00070a8 to
6c71e41
Compare
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
ddprof-lib/src/main/cpp/flightRecorder.cpp:1167
- The depth-limit ERROR log in writeElement() never fires in normal recursion because recursion is stopped earlier via
truncate_childrenat depth==10 (sodepth > 10is unreachable from the only call site). That means depth truncation is currently counter-only with no log line, which contradicts the stated goal of a logged truncated dump when the metadata tree is unexpectedly deep/cyclic. Consider emitting a one-time ERROR whentruncate_childrenis true (e.g., inside countSerializableChildren), while keeping the per-child counter increments as-is.
size_t Recording::countSerializableChildren(
const std::vector<const Element *> &children, int depth) {
// Children one level deeper than `depth` are what writeElement() would
// truncate on its own depth check, so exclude them here too, before being
// counted, so child_count always matches the number of children actually
// serialized below (an inflated count would make the metadata stream
// itself malformed).
bool truncate_children = depth + 1 > 10;
size_t child_count = 0;
for (size_t i = 0; i < children.size(); i++) {
if (children[i] == nullptr) {
Counters::increment(METADATA_TREE_NULL_CHILD);
fprintf(stderr, "[ddprof] [WARN] writeElement skipping null child at index %zu\n", i);
} else if (truncate_children) {
Counters::increment(METADATA_TREE_DEPTH_EXCEEDED);
} else {
child_count++;
}
docs/sphinx/specs/2026-07-27-sigsegv-in-recording-writeelement.md:18
- This spec describes two fixes as being “shipped together” and includes detailed scope/correctness requirements for a JfrMetadata::reset() cleanup, but this PR does not implement any reset() deletion logic (reset() still just clears vectors). Please either implement the reset() cleanup in this PR, or update the spec to clearly mark the reset() leak work as out-of-scope/follow-up so the doc matches the actual changes.
What follows are two independently-real, independently-justified fixes shipped together. Fix 2 (defensive traversal) is the one that actually addresses the observed crash shape, regardless of trigger. Fix 1 (reset() leak) is a genuine bug worth closing but is not believed to be the trigger for this specific crash cluster, since it requires a restart cycle this deployment's lifecycle doesn't exercise.
1. **[Latent, not confirmed as trigger] Memory leak in JfrMetadata::reset():** The `reset()` function (jfrMetadata.cpp:32–40) calls `_root._children.clear()` but never deletes the heap-allocated Element objects allocated in `initialize()` (lines 183, 145–149, 207). If `Profiler::start()` were ever called a second time in the same process (restart), and the heap allocator reused freed memory addresses, `writeElement()` could dereference stale/corrupted pointers. Real bug, but only reachable via a restart cycle that does not occur in normal deployments — see status note above.
2. **[Addresses the observed crash shape] Unbounded recursion without cycle or null guard:** `writeElement()` recursively traverses the Element tree without a depth limit, cycle detection, or null checks on `e`/`e->_children[i]`. Whatever is producing the corrupted/dangling `Element*` in production, this is what turns it into a SIGSEGV instead of a safely-logged truncation. This is the mitigation that actually matters for PROF-15075 until the true trigger is found.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
docs/sphinx/specs/2026-07-27-sigsegv-in-recording-writeelement.md:48
- The "Primary fixes" section lists the JfrMetadata::reset() cleanup as a primary fix, but that implementation is not part of this PR. Mark it explicitly as follow-up/out-of-scope here as well (or add the missing code change) to avoid readers assuming the leak was fixed by this PR.
1. **jfrMetadata.cpp:32–40 (JfrMetadata::reset)** — Memory leak: Implement recursive deletion of all heap-allocated Elements in `_root._children` before clearing the vector. Add a private helper function `void deleteElementTree(Element* e)` that recursively deletes all children and the element itself.
2. **flightRecorder.cpp:1140–1156 (Recording::writeElement)** — Unbounded recursion and null-pointer dereference: Add a depth parameter (default 0) with a check on entry; if `depth > 10`, log ERROR and return without recursing. Add null checks for `e`, `e->_children[i]` with defensive logging and continue-on-skip behavior.
ddprof-lib/src/main/cpp/flightRecorder.cpp:1168
- countSerializableChildren() currently emits one stderr line per null child and does per-child atomic counter increments. On a heavily corrupted metadata tree this can flood logs during periodic chunk dumps and adds unnecessary atomic ops. Consider batching null/truncated child counts and logging once per element (and using a single counter increment with delta).
for (size_t i = 0; i < children.size(); i++) {
if (children[i] == nullptr) {
Counters::increment(METADATA_TREE_NULL_CHILD);
fprintf(stderr, "[ddprof] [WARN] writeElement skipping null child at index %zu\n", i);
} else if (truncate_children) {
ddprof-lib/src/main/cpp/flightRecorder.cpp:1174
- writeElement() returns immediately on a null element without incrementing METADATA_TREE_NULL_CHILD or logging. If the entrypoint ever sees a null Element* (e.g., corrupted root), this loses the durable telemetry signal the PR description/doc calls out.
if (e == nullptr) {
return;
}
docs/sphinx/specs/2026-07-27-sigsegv-in-recording-writeelement.md:16
- This spec says two fixes are "shipped together" and details a JfrMetadata::reset() leak fix, but this PR’s code changes do not touch jfrMetadata.*. Please clarify in the spec that the reset() leak item is a follow-up (or add the corresponding code change) so the doc matches what’s actually in the PR.
This issue also appears on line 46 of the same file.
What follows are two independently-real, independently-justified fixes shipped together. Fix 2 (defensive traversal) is the one that actually addresses the observed crash shape, regardless of trigger. Fix 1 (reset() leak) is a genuine bug worth closing but is not believed to be the trigger for this specific crash cluster, since it requires a restart cycle this deployment's lifecycle doesn't exercise.
1. **[Latent, not confirmed as trigger] Memory leak in JfrMetadata::reset():** The `reset()` function (jfrMetadata.cpp:32–40) calls `_root._children.clear()` but never deletes the heap-allocated Element objects allocated in `initialize()` (lines 183, 145–149, 207). If `Profiler::start()` were ever called a second time in the same process (restart), and the heap allocator reused freed memory addresses, `writeElement()` could dereference stale/corrupted pointers. Real bug, but only reachable via a restart cycle that does not occur in normal deployments — see status note above.
Drop the JfrMetadata::reset() leak fix and its tests: that leak is only reachable via a Profiler restart (reset() -> initialize() twice in the same process), which doesn't happen in production. It's real but unrelated to this crash and is fixed separately in DataDog/java-profiler#<leak-pr>. The PROF-15075 crash trace (dump -> switchChunk -> writeMetadata -> writeElement, no restart involved) is addressed by the null-check and recursion-depth guard in writeElement alone; true trigger still unconfirmed, see updated spec.
stderr from an embedded native lib is rarely captured/monitored, and this guard exists precisely because the tree-corruption trigger is unconfirmed (PROF-15075) -- an unmonitored log line would let it recur invisibly. Increment metadata_tree_null_child / metadata_tree_depth_exceeded so a recurrence shows up as a datadog.ProfilerCounter JFR event instead of only stderr.
Exclude depth-truncated children from the encoded child_count so the JFR metadata stream stays structurally valid, and add gtest coverage for the null-child and depth-boundary serialization guards.
Each TEST now uses an ElementOwner that frees its Elements via unique_ptr on scope exit, instead of leaking raw new-allocated Elements per test run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jbachorik
force-pushed
the
sphinx/fix-jira-PROF-15075
branch
from
August 4, 2026 11:11
46757e3 to
655da91
Compare
jbachorik
marked this pull request as ready for review
August 4, 2026 11:13
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
ddprof-lib/src/main/cpp/flightRecorder.cpp:1169
- When
truncate_childrenis true (depth boundary), this helper only incrementsMETADATA_TREE_DEPTH_EXCEEDEDbut emits no error log. SincewriteElement()also stops recursing whentruncate_childrenis true, thedepth > 10branch (and its[ERROR] ... truncating outputlog) is effectively unreachable from the normal recursion path, so deep/cyclic metadata trees can be silently truncated except for the counter signal. Consider logging once per truncated node (not once per child) when depth limiting is applied, so the PR behavior matches the "logged, truncated dump" description and improves diagnosability in logs.
if (children[i] == nullptr) {
Counters::increment(METADATA_TREE_NULL_CHILD);
fprintf(stderr, "[ddprof] [WARN] writeElement skipping null child at index %zu\n", i);
} else if (truncate_children) {
Counters::increment(METADATA_TREE_DEPTH_EXCEEDED);
Contributor
Benchmark Results (commit 655da91)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/128813465 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10475 ms (21 iters) | ✅ 10226 ms (21 iters) | ≈ -2.4% (±10.4%) | — / — |
| akka-uct | 25 | ✅ 8831 ms (24 iters) | ✅ 8793 ms (24 iters) | ≈ -0.4% (±9.9%) | — / — |
| finagle-chirper | 21 | ✅ 5965 ms (33 iters) | ✅ 5988 ms (33 iters) | ≈ +0.4% (±25.3%) | |
| finagle-chirper | 25 | ✅ 5440 ms (36 iters) | ✅ 5496 ms (36 iters) | ≈ +1% (±24%) | |
| fj-kmeans | 21 | ✅ 2696 ms (70 iters) | ✅ 2696 ms (69 iters) | ≈ 0% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2848 ms (66 iters) | ✅ 2852 ms (66 iters) | ≈ +0.1% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2076 ms (89 iters) | ✅ 2099 ms (88 iters) | ≈ +1.1% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 2056 ms (90 iters) | ✅ 2124 ms (87 iters) | 🔴 +3.3% | — / — |
| naive-bayes | 21 | ✅ 1305 ms (132 iters) | ✅ 1253 ms (136 iters) | ≈ -4% (±31.6%) | — / — |
| naive-bayes | 25 | ✅ 1012 ms (168 iters) | ✅ 975 ms (175 iters) | ≈ -3.7% (±31.4%) | — / — |
| reactors | 21 | ✅ 16279 ms (15 iters) | ✅ 16648 ms (15 iters) | ≈ +2.3% (±7.3%) | — / — |
| reactors | 25 | ✅ 18667 ms (15 iters) | ✅ 18594 ms (15 iters) | ≈ -0.4% (±4.5%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | 1 / 4 | 2286 / 2162 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 7 / 4 | 8919 / 8506 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 8223 / 8159 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 4 | 1297 / 1255 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1290 / 1270 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 2915 / 2970 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2862 / 2900 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 5 | 3537 / 3509 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 3 | 3467 / 3517 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1591 / 1666 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1839 / 1910 | ✅ / ✅ | ✅ / ✅ |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
ddprof-lib/src/main/cpp/flightRecorder.cpp:1182
depth > 10handling is effectively unreachable with the current recursion gate (truncate_children = depth + 1 > 10andif (!truncate_children)around the loop). As a result, the[ERROR] writeElement depth limit exceeded...log and its counter increment will never trigger from the normal call path, and all depth-exceeded accounting comes fromcountSerializableChildren()instead. Consider either removing the deaddepth > 10block, or reworking the depth logic so a single, reachable place is responsible for both truncation and telemetry (ideally logging once per truncated element rather than per child).
if (depth > 10) {
// stderr from an embedded native lib is rarely captured or monitored, and
// we don't yet know what corrupts the tree, so the counter is the durable
// signal here — an unmonitored log line would let it recur invisibly.
Counters::increment(METADATA_TREE_DEPTH_EXCEEDED);
fprintf(stderr, "[ddprof] [ERROR] writeElement depth limit exceeded, truncating output\n");
return;
ddprof-lib/src/main/cpp/counters.h:139
- The comment claims writeElement() guards against a "dangling" metadata tree, but the added guards only handle null children and excessive recursion depth. A truly dangling (non-null invalid)
Element*would still SIGSEGV on dereference, so this wording is misleading.
/* writeElement() guards against a corrupted/dangling JfrMetadata tree. \
* Root cause is still unconfirmed, so these counters are the durable \
* signal for spotting a recurrence. */ \
Contributor
Benchmark Results (commit 1fa759e)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/128867886 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10251 ms (21 iters) | ✅ 10286 ms (21 iters) | ≈ +0.3% (±11.3%) | — / — |
| akka-uct | 25 | ✅ 8900 ms (24 iters) | ✅ 8812 ms (24 iters) | ≈ -1% (±10.5%) | — / — |
| finagle-chirper | 21 | ✅ 5959 ms (33 iters) | ✅ 5950 ms (33 iters) | ≈ -0.2% (±25.3%) | |
| finagle-chirper | 25 | ✅ 5431 ms (36 iters) | ✅ 5440 ms (36 iters) | ≈ +0.2% (±24.3%) | |
| fj-kmeans | 21 | ✅ 2728 ms (68 iters) | ✅ 2667 ms (69 iters) | ≈ -2.2% (±2.6%) | — / — |
| fj-kmeans | 25 | ✅ 2834 ms (66 iters) | ✅ 2834 ms (66 iters) | ≈ 0% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2075 ms (89 iters) | ✅ 2095 ms (89 iters) | ≈ +1% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2001 ms (93 iters) | ✅ 2074 ms (90 iters) | 🔴 +3.6% | — / — |
| naive-bayes | 21 | ✅ 1273 ms (134 iters) | ✅ 1276 ms (135 iters) | ≈ +0.2% (±32.8%) | — / — |
| naive-bayes | 25 | ✅ 1009 ms (170 iters) | ✅ 1024 ms (167 iters) | ≈ +1.5% (±31.9%) | — / — |
| reactors | 21 | ✅ 16217 ms (15 iters) | ✅ 16346 ms (15 iters) | ≈ +0.8% (±7.4%) | — / — |
| reactors | 25 | ✅ 18641 ms (15 iters) | ✅ 18034 ms (15 iters) | ≈ -3.3% (±5.2%) | — / — |
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 | 1927 / 1959 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2300 / 2231 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 8848 / 8550 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 8281 / 8345 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1240 / 1232 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1268 / 1276 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 4 | 2945 / 3025 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 2958 / 2871 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 5 | 3493 / 3532 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 9 | 3469 / 3497 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 1792 / 1683 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1884 / 1766 | ✅ / ✅ | ✅ / ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
Adds a null check and a recursion-depth guard (10) to
Recording::writeElement(), so a corrupted or danglingElement*in the JFR metadata tree produces a logged, truncated dump instead of a SIGSEGV. Each guard trip also increments a dedicated counter (metadata_tree_null_child/metadata_tree_depth_exceeded), which flows out as adatadog.ProfilerCounterJFR event through the existing counter-reporting path — a durable, queryable signal in case this recurs, rather than relying onstderrfrom an embedded native library (rarely captured/monitored). Also fixes a macOS x86_64 build issue insafeAccess.cpp(current_pcreferenced an undefinedcontext_ripmacro instead of theDU3_PREFIX-baseduc_mcontextaccessor already used on aarch64), found while testing this change locally.Motivation:
PROF-15075: a SIGSEGV crash cluster in
Recording::writeElement, seen on JDK 17/21/25 with dd-trace-java 1.63.0. The production trace (writeElementx5 self-recursive <-writeMetadata<-switchChunk<-Profiler::dump) is a plain periodic chunk dump with no profiler restart involved, so what actually corrupts/nulls theElement*is not yet confirmed. This guard turns the crash into a safe, logged, telemetry-visible failure in the meantime.Additional Notes:
The true trigger remains open. A core dump or heap-sanitizer run is what's needed to confirm it; the new counters and log lines are meant to catch and localize a recurrence in the meantime, not to explain away the underlying issue.
How to test the change?:
./gradlew :ddprof-lib:buildDebug -Pskip-gtestand theflightRecorder_result_utgtest suite pass with this 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!