Skip to content

fix: own line number table memory to avoid UAF on class unload (PROF-14547) - #511

Closed
jbachorik wants to merge 6 commits into
muse/sigsegv-in-recordingfrom
muse/fix-sigsegv-recording
Closed

fix: own line number table memory to avoid UAF on class unload (PROF-14547)#511
jbachorik wants to merge 6 commits into
muse/sigsegv-in-recordingfrom
muse/fix-sigsegv-recording

Conversation

@jbachorik

@jbachorikjbachorik commented May 6, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?:

Fixes SIGSEGV in Profiler::processCallTraces (production stack: processCallTracesRecording::writeStackTracesRecording::writeCpoolRecording::finishChunkFlightRecorder::dumpProfiler::dump). Detaches MethodInfo::_line_number_table->_ptr lifetime from JVMTI by copying the line number table into a malloc'd buffer at capture time inside Lookup::fillJavaMethodInfo and immediately calling jvmti->Deallocate on the JVMTI memory. ~SharedLineNumberTable now calls free() on the owned buffer.

Motivation:

Production SIGSEGV in processCallTraces+0xaa during JFR dump. Root cause: MethodInfo::_line_number_table->_ptr held a raw pointer to JVMTI-allocated memory returned by GetLineNumberTable. When the underlying class was unloaded, the JVM freed that memory; the next dump's writeStackTraces lambda called MethodInfo::getLineNumber(bci) on a dangling _ptr and crashed.

This is a peer manifestation of (PR #510), which crashes at the symmetric deallocation site (~SharedLineNumberTableDeallocate). Both stem from the same dangling pointer; this fix addresses the lifetime invariant once and subsumes both crashes. PR #510 stays correct (becomes belt-and-suspenders) but is no longer required for the SIGSEGV fix.

Additional Notes:

  • Hot-path impact is once per first-time-encountered method per dump cycle (gated by !mi->_mark and mi->_key == 0). The new work is one malloc + memcpy of the line number table (typically 16–80 bytes) plus an immediate Deallocate — replaces the implicit JVMTI-side allocation that already happened. Not on the signal handler path (fillJavaMethodInfo runs on the JFR-writer thread under _state_lock + lockAll()).
  • Memory footprint is unchanged: same logical bytes per method, same lifetime, gated by the existing AGE_THRESHOLD=3 eviction.
  • malloc failure is handled silently on the read side (getLineNumber returns 0 when _line_number_table is null) and logged via TEST_LOG.
  • jvmti->Deallocate failure now logged via TEST_LOG (preserves the diagnostic that the destructor previously emitted).
  • Reviewed by 4 internal reviewer voices (concurrency, perf, error-paths, tests). Concerns addressed inline; deferred items: JMH benchmark for chunk-write latency on method-churn workloads (follow-up).

How to test the change?:

Added regression test WriteStackTracesAfterClassUnloadTest:

  1. Generate a class with line-numbered methods via ASM.
  2. Hammer its methods for ~300ms while CPU profiling at 1ms so traces land in call_trace_storage and a MethodInfo with _line_number_table is created.
  3. Drop strong refs and System.gc() until the WeakReference clears (class actually unloaded); assumeTrue-skip if the JVM declined to unload within the deadline.
  4. Dump 4 times in tight succession. Without the fix, getLineNumber dereferences the freed JVMTI memory → SIGSEGV. With the fix, _ptr is a malloc'd copy → safe.

Test set deliberately uses mcleanup=false to isolate the failure to the PROF-14547 read path; the PROF-14545 cleanup path is covered separately by CleanupAfterClassUnloadTest (PR #510). Best-effort under ASan/UBSan, which amplify the signal even when the freed region happens to read sensible bytes — limitations documented in the test javadoc.

Build verified locally: :ddprof-lib:assembleRelease and :ddprof-test:compileTestJava both pass on macOS arm64.

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 review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.
  • JIRA: PROF-14547

@jbachorikjbachorik added the AI label May 6, 2026
@jbachorik
jbachorik requested a review from CopilotMay 6, 2026 23:28
@jbachorik

Copy link
Copy Markdown
CollaboratorAuthor

@codex review

This comment was marked as outdated.

@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:05a6f4cb00

ℹ️ 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/main/cpp/mallocTracer.cpp
@dd-octo-sts

dd-octo-stsBot commented May 6, 2026

Copy link
Copy Markdown
Contributor

CI Test Results

Run:#25755969382 | Commit:25f4cb2 | Duration: 31m 6s (longest job)

1 of 32 test jobs failed

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

Failed Tests

musl-aarch64/debug / 11-librca

Job:View logs

No detailed failure information available. Check the job logs.

Summary: Total: 32 | Passed: 31 | Failed: 1


Updated: 2026-05-12 19:58:39 UTC

@jbachorik

Copy link
Copy Markdown
CollaboratorAuthor

@codex review

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 1 comment.

@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:8b42102179

ℹ️ 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/main/cpp/flightRecorder.cpp Outdated
@jbachorik
jbachorik marked this pull request as ready for review May 7, 2026 20:25
@jbachorik
jbachorik requested a review from a team as a code ownerMay 7, 2026 20:25
@jbachorik
jbachorik requested a review from CopilotMay 7, 2026 20:26

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.

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

Looks good overall, I only have a few minor comments.

Comment threadddprof-lib/src/main/cpp/flightRecorder.cpp Outdated
Comment threadddprof-lib/src/main/cpp/flightRecorder.cpp

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

Thank you! Looks good, now!

@jbachorik
jbachorikforce-pushed the muse/fix-sigsegv-recording branch from 8eeddf9 to bb53d21CompareMay 11, 2026 22:14
@jbachorik
jbachorik changed the base branch from main to muse/sigsegv-in-recordingMay 11, 2026 22:14
@jbachorik
jbachorikforce-pushed the muse/fix-sigsegv-recording branch from bb53d21 to b485d32CompareMay 12, 2026 00:33
@jbachorik
jbachorikforce-pushed the muse/sigsegv-in-recording branch from 1ad4c18 to d73f80cCompareMay 12, 2026 05:56
@jbachorik
jbachorikforce-pushed the muse/fix-sigsegv-recording branch 2 times, most recently from 5df7be9 to c75a4a1CompareMay 12, 2026 07:15
jbachorikand others added 2 commits May 12, 2026 21:02
…14547)
Copy the JVMTI line number table into a malloc'd buffer at capture time
and Deallocate the JVMTI memory immediately, detaching _ptr lifetime
from class unload. Eliminates the SIGSEGV in MethodInfo::getLineNumber
during writeStackTraces. Adds a regression test.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
jbachorikand others added 4 commits May 12, 2026 21:02
- Remove PROF-14545/PROF-14547 ticket references from comments in
flightRecorder.cpp and flightRecorder.h
- Fix samplingInterval metadata: use <=1 threshold to match
shouldSample's interval<=1 condition (nativemem=1 also records every
allocation)
Co-Authored-By: muse <muse@noreply>
- Remove dead `line_number_table = nullptr` (local var, no effect after Deallocate)
- Extract AbstractDynamicClassTest base class; deduplicate generateClassBytecode,
tempFile (now uses java.io.tmpdir), and IsolatedClassLoader from
GetLineNumberTableLeakTest and WriteStackTracesAfterClassUnloadTest
Co-Authored-By: muse <muse@noreply>
@jbachorik
jbachorikforce-pushed the muse/fix-sigsegv-recording branch from c75a4a1 to afdfea2CompareMay 12, 2026 19:02
@jbachorik

Copy link
Copy Markdown
CollaboratorAuthor

Subsumed into #510 — all commits merged into muse/sigsegv-in-recording.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@jbachorik@rkennke