Skip to content

Fix memory leak in JfrMetadata::reset() - #694

Merged
jbachorik merged 4 commits into
mainfrom
fix/jfrmetadata-reset-leak
Aug 6, 2026
Merged

Fix memory leak in JfrMetadata::reset()#694
jbachorik merged 4 commits into
mainfrom
fix/jfrmetadata-reset-leak

Conversation

@jbachorik

@jbachorikjbachorik commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?:
JfrMetadata::reset() cleared _root._children/_root._attributes without deleting the heap-allocated Element/NoField objects initialize() had allocated, leaking them and leaving the tree vulnerable to use-after-free if the allocator later reused a freed address. Adds recursive deletion of the Element tree and the tracked _nofields list before clearing.

Motivation:
Only reachable via a profiler restart (reset() followed by initialize() a second time in the same process). Found while investigating PROF-15075 (SIGSEGV in Recording::writeElement) but not its cause — that crash trace shows a plain chunk dump with no restart involved. Closing this separately since it's a real leak/UAF risk on its own.

How to test the change?:

  • ddprof-lib/src/test/cpp/jfrMetadata_ut.cpp exercises initialize()/reset() cycles, including with context attributes, and asserts no leaked/dangling Elements.

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: [JIRA-XXXX]

Unsure? Have a question? Request a review!

reset() cleared _root._children/_attributes without deleting the
heap-allocated Element/NoField objects initialize() had allocated.
Only reachable via a Profiler restart (reset() -> initialize() a
second time in the same process); unrelated to PROF-15075, which
crashes on a plain chunk dump with no restart involved (split out
of #692 per https://datadoghq.atlassian.net/browse/PROF-15075).
CopilotAI review requested due to automatic review settings July 28, 2026 11:41

CopilotAI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@jbachorikjbachorik changed the title Fix memory leak in JfrMetadata::reset() (split from PROF-15075)Fix memory leak in JfrMetadata::reset()Jul 28, 2026
@dd-octo-sts

dd-octo-stsBot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

CI Test Results

Run:#31097106586 | Commit:8b9d329 | Duration: 14m 42s (longest job)

All 32 test jobs passed

Status Overview

JDKglibc-aarch64/debugglibc-amd64/debugmusl-aarch64/debugmusl-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

Summary: Total: 32 | Passed: 32 | Failed: 0


Updated: 2026-08-06 11:41:22 UTC

@dd-octo-sts

Copy link
Copy Markdown
Contributor

Benchmark Results (commit 008d688)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127401287 Commit: 008d6889e4f9acc4be699bb2ac845f3396e20528

⚠️ Significant outliers

  • 🟢 fj-kmeans (JDK 21): runtime -3.9% (2745→2638 ms)
  • 🟢 future-genetic (JDK 21): runtime -3% (2122→2058 ms)
Runtime details (per benchmark × JDK)
BenchmarkJDKLatestDevΔ (dev vs latest)Issues L/D
akka-uct21✅ 10193 ms (21 iters)✅ 10416 ms (21 iters)≈ +2.2% (±11.2%)— / —
akka-uct25✅ 8880 ms (24 iters)✅ 8743 ms (24 iters)≈ -1.5% (±10%)— / —
finagle-chirper21✅ 6002 ms (33 iters)✅ 5929 ms (33 iters)≈ -1.2% (±24.7%)⚠️ W:3 / ⚠️ W:3
finagle-chirper25✅ 5437 ms (36 iters)✅ 5445 ms (36 iters)≈ +0.1% (±24.3%)⚠️ W:3 / ⚠️ W:3
fj-kmeans21✅ 2745 ms (68 iters)✅ 2638 ms (72 iters)🟢 -3.9%— / —
fj-kmeans25✅ 2832 ms (66 iters)✅ 2832 ms (66 iters)≈ 0% (±2.6%)— / —
future-genetic21✅ 2122 ms (87 iters)✅ 2058 ms (90 iters)🟢 -3%— / —
future-genetic25✅ 2059 ms (90 iters)✅ 2049 ms (90 iters)≈ -0.5% (±2.6%)— / —
naive-bayes21✅ 1257 ms (136 iters)✅ 1304 ms (132 iters)≈ +3.7% (±33.1%)— / —
naive-bayes25✅ 1028 ms (167 iters)✅ 1014 ms (169 iters)≈ -1.4% (±31.8%)— / —
reactors21✅ 16668 ms (15 iters)✅ 15846 ms (15 iters)≈ -4.9% (±7.4%)— / —
reactors25✅ 18281 ms (15 iters)✅ 18391 ms (15 iters)≈ +0.6% (±5%)— / —
Internal counter details (ddprof)

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

BenchmarkJDKDropped recDropped jvmtiDropped traceSkipped WCAGCT failUnwind fail
akka-uct21✅ / ✅✅ / ✅4 / 21961 / 1939✅ / ✅✅ / ✅
akka-uct25✅ / ✅✅ / ✅6 / 12369 / 2031✅ / ✅✅ / ✅
finagle-chirper21✅ / ✅✅ / ✅4 / 18227 / 8628✅ / ✅✅ / ✅
finagle-chirper25✅ / ✅✅ / ✅✅ / ✅8367 / 8248✅ / ✅✅ / ✅
fj-kmeans21✅ / ✅✅ / ✅2 / 11267 / 1276✅ / ✅✅ / ✅
fj-kmeans25✅ / ✅✅ / ✅✅ / ✅1287 / 1259✅ / ✅✅ / ✅
future-genetic21✅ / ✅✅ / ✅2 / 12932 / 3002✅ / ✅✅ / ✅
future-genetic25✅ / ✅✅ / ✅2 / 22843 / 2845✅ / ✅✅ / ✅
naive-bayes21✅ / ✅✅ / ✅2 / 43484 / 3550✅ / ✅✅ / ✅
naive-bayes25✅ / ✅✅ / ✅2 / 43492 / 3488✅ / ✅✅ / ✅
reactors21✅ / ✅✅ / ✅1 / 21638 / 1621✅ / ✅✅ / ✅
reactors25✅ / ✅✅ / ✅1 / 11843 / 1824✅ / ✅✅ / ✅

CopilotAI review requested due to automatic review settings August 4, 2026 10:25
@jbachorik

Copy link
Copy Markdown
CollaboratorAuthor

@codex review

@dd-octo-sts

dd-octo-stsBot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Scan-Build Report

User:runner@runnervmd93pd
Working Directory:/home/runner/work/java-profiler/java-profiler/ddprof-lib/src/test/make
Command Line:make -j4 all
Clang Version:Ubuntu clang version 18.1.3 (1ubuntu1)
Date:Thu Aug 6 11:26:20 2026

Bug Summary

Bug TypeQuantityDisplay?
All Bugs1
Logic error
Dereference of null pointer1

Reports

Bug GroupBug Type ▾FileFunction/MethodLinePath Length
Logic errorDereference of null pointerprofiler.hfindLibraryByAddress51713

CopilotAI 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 3 out of 3 changed files in this pull request and generated no new comments.

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:64c7b4423d

ℹ️ 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".

Comment threadddprof-lib/src/test/cpp/jfrMetadata_ut.cpp
Comment threadddprof-lib/src/test/cpp/jfrMetadata_ut.cpp
@dd-octo-sts

Copy link
Copy Markdown
Contributor

Benchmark Results (commit 64c7b44)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/128804523 Commit: 64c7b4423d39dedb72601809089f923c0f484e62

⚠️ Significant outliers

  • 🔴 future-genetic (JDK 21): runtime +2.6% (2037→2090 ms)
Runtime details (per benchmark × JDK)
BenchmarkJDKLatestDevΔ (dev vs latest)Issues L/D
akka-uct21✅ 10336 ms (21 iters)✅ 10225 ms (21 iters)≈ -1.1% (±11.2%)— / —
akka-uct25✅ 8844 ms (24 iters)✅ 8823 ms (24 iters)≈ -0.2% (±9.5%)— / —
finagle-chirper21✅ 5987 ms (33 iters)✅ 5912 ms (33 iters)≈ -1.3% (±25.2%)⚠️ W:3 / ⚠️ W:3
finagle-chirper25✅ 5454 ms (36 iters)✅ 5458 ms (36 iters)≈ +0.1% (±24.1%)⚠️ W:3 / ⚠️ W:3
fj-kmeans21✅ 2712 ms (68 iters)✅ 2695 ms (70 iters)≈ -0.6% (±2.8%)— / —
fj-kmeans25✅ 2828 ms (66 iters)✅ 2822 ms (66 iters)≈ -0.2% (±2.6%)— / —
future-genetic21✅ 2037 ms (91 iters)✅ 2090 ms (89 iters)🔴 +2.6%— / —
future-genetic25✅ 2038 ms (91 iters)✅ 2027 ms (92 iters)≈ -0.5% (±2.6%)— / —
naive-bayes21✅ 1264 ms (135 iters)✅ 1273 ms (134 iters)≈ +0.7% (±32.6%)— / —
naive-bayes25✅ 975 ms (175 iters)✅ 984 ms (173 iters)≈ +0.9% (±31.8%)— / —
reactors21✅ 16879 ms (15 iters)✅ 16257 ms (15 iters)≈ -3.7% (±6.4%)— / —
reactors25✅ 18594 ms (15 iters)✅ 18489 ms (15 iters)≈ -0.6% (±5.2%)— / —
Internal counter details (ddprof)

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

BenchmarkJDKDropped recDropped jvmtiDropped traceSkipped WCAGCT failUnwind fail
akka-uct21✅ / ✅✅ / ✅5 / ✅1958 / 2013✅ / ✅✅ / ✅
akka-uct25✅ / ✅✅ / ✅1 / 22362 / 2213✅ / ✅✅ / ✅
finagle-chirper21✅ / ✅✅ / ✅1 / 48457 / 8027✅ / ✅✅ / ✅
finagle-chirper25✅ / ✅✅ / ✅2 / ✅8212 / 8454✅ / ✅✅ / ✅
fj-kmeans21✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅
fj-kmeans25✅ / ✅✅ / ✅3 / 21255 / 1238✅ / ✅✅ / ✅
future-genetic21✅ / ✅✅ / ✅2 / ✅3008 / 2999✅ / ✅✅ / ✅
future-genetic25✅ / ✅✅ / ✅6 / 32856 / 2871✅ / ✅✅ / ✅
naive-bayes21✅ / ✅✅ / ✅8 / 73501 / 3494✅ / ✅✅ / ✅
naive-bayes25✅ / ✅✅ / ✅3 / 63478 / 3462✅ / ✅✅ / ✅
reactors21✅ / ✅✅ / ✅✅ / ✅1843 / 1642✅ / ✅✅ / ✅
reactors25✅ / ✅✅ / ✅1 / ✅1897 / 1838✅ / ✅✅ / ✅

@jbachorik
jbachorik marked this pull request as ready for review August 4, 2026 11:15
@jbachorik
jbachorik requested a review from a team as a code ownerAugust 4, 2026 11:15

@kaahoskaahos 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.

Thanks for the fix! It looks good to me overall, I've just left a suggestion to avoid a potential UB.

Comment threadddprof-lib/src/main/cpp/jfrMetadata.h
CopilotAI review requested due to automatic review settings August 6, 2026 11:07
CopilotAI reviewed Aug 6, 2026

CopilotAI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

CopilotAI review requested due to automatic review settings August 6, 2026 11:24
CopilotAI reviewed Aug 6, 2026

CopilotAI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@jbachorik
jbachorik merged commit b845049 into mainAug 6, 2026
89 of 99 checks passed
@jbachorik
jbachorik deleted the fix/jfrmetadata-reset-leak branch August 6, 2026 11:32
@github-actionsgithub-actionsBot added this to the 1.49.0 milestone Aug 6, 2026
jbachorik added a commit that referenced this pull request Aug 6, 2026
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.
zhengyu123 pushed a commit that referenced this pull request Aug 11, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@jbachorik@kaahos