Uh oh!
There was an error while loading. Please reload this page.
Guard Recording::writeElement against SIGSEGV on a corrupted metadata tree - #692
Conversation
This comment has been minimized.
This comment has been minimized.
CI Test ResultsRun:#31099759182 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-08-06 12:23:03 UTC |
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):
|
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 | ✅ / ✅ | ✅ / ✅ |
jbachorik
commented
Jul 29, 2026
@codex review |
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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 | ✅ / ✅ | ✅ / ✅ |
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/127721763 |
00070a8 to
6c71e41CompareScan-Build Report
Bug Summary
Reports
| ||||||||||||||||||||||||||||||||||||
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.
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 | ✅ / ✅ | ✅ / ✅ |
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. */ \
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 | ✅ / ✅ | ✅ / ✅ |
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>
1fa759e to
ee6f9bdComparejbachorik
commented
Aug 6, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:ee6f9bda5a
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
rkennke
left a comment
There was a problem hiding this comment.
Depth-exceeded truncation is telemetrized but never logged to stderr — unlike the null-child guard.
In Recording::writeElement() (flightRecorder.cpp), the if (depth > 10) { ...; fprintf(stderr, "...depth limit exceeded...") } block at the top of the function is dead code. writeElement() is only reachable at depth 0..10: the sole recursive call site only fires when !truncate_children (i.e. depth + 1 <= 10), so depth can never exceed 10 when writeElement() is entered. That means this fprintf — and the Counters::increment(METADATA_TREE_DEPTH_EXCEEDED) alongside it — never execute.
The actual telemetry for depth-truncated children does fire, but from a different place: countSerializableChildren()'s else if (truncate_children) branch, which runs on every writeElement() call and correctly increments METADATA_TREE_DEPTH_EXCEEDED once per excluded child. It just has no accompanying fprintf, unlike the null-child branch right above it, which does both Counters::increment(METADATA_TREE_NULL_CHILD) and a fprintf(stderr, ...).
Net effect: a null-child truncation is both counted and logged; a depth-exceeded truncation is only counted, silently. That's inconsistent with the PR's own stated rationale — that stderr logging is needed as a human-visible backstop alongside the counters, precisely because the tree-corruption trigger is unconfirmed.
Suggested fix: move the fprintf into countSerializableChildren()'s truncate_children branch (next to its existing counter increment), and delete the now-unreachable if (depth > 10) block in writeElement() entirely, rather than trying to make that block reachable. Making it reachable instead (e.g. by loosening the caller's truncate_children guard so recursion continues to depth 11) would double-count the same truncation event (once in countSerializableChildren(), again in writeElement()) and would reintroduce the exact child_count/actual-serialized-children mismatch this PR's own regression tests guard against, since countSerializableChildren() would have already excluded that child from the encoded count before writeElement() serialized it anyway.
The reset()-regression suite belongs to main (merged separately in #694) and was never part of this PR's scope; a rebase conflict resolution deleted it by mistake.
writeElement()'s depth>10 guard was dead code: the sole recursive call site never invokes it above depth 10. Depth-exceeded truncation was therefore only ever counted, never logged, unlike the null-child case right above it. Move the fprintf into countSerializableChildren()'s truncate_children branch and drop the unreachable block.
jbachorik
commented
Aug 6, 2026
Good catch on the dead code — fixed in 8a8d4ce. Moved the |
Uh oh!
There was an error while loading. Please reload this page.
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!