fix(profiler): make _class_map access signal-safe in walkVM by jbachorik · Pull Request #512 · DataDog/java-profiler

jbachorik · 2026-05-07T10:09:35Z

What does this PR do?:

Fixes a SIGSEGV in Dictionary::clear reached via Profiler::dump by closing two correctness holes around Profiler::_class_map. Also covers the crash signature from PROF-14550 (same root cause, dump-path variant).

walkVM at hotspotSupport.cpp:388 previously called the inserting 2-arg Dictionary::lookup from a signal handler, which calls malloc/calloc (dictionary.cpp:25,114). That is async-signal-unsafe and could tear the malloc arena. Switched to the existing read-only bounded_lookup(..., 0) guarded by OptionalSharedLockGuard (tryLockShared); on miss or contention the synthetic vtable-target frame is skipped — the JVMTI / Java-API path will populate the entry from a non-signal context.
Recording::writeClasses previously called _classes->collect(map) with no lock while JVMTI shared-lock writers (Profiler::lookupClass, ObjectSampler, LivenessTracker) could mutate the dictionary, and the dump path's exclusive _class_map.clear() ran shortly after. Now wrapped in SharedLockGuard guard(Profiler::instance()->classMapLock()). Multiple shared-lock holders coexist with CAS-based inserters; the exclusive clear() waits until writeClasses returns.
Added a public Profiler::classMapLock() accessor next to the existing classMap() accessor.
Documented the signal-safety hazard on the inserting Dictionary::lookup overloads.
Corrected the misleading comments in writeCpool and writeClasses that previously claimed concurrent access was already safe.

Motivation:

PROF-14549 reports a SIGSEGV in Dictionary::collect reached via Recording::writeClasses during Profiler::dump. PROF-14550 reports the same SIGSEGV in Dictionary::clear reached via Profiler::dump directly. Both trace to the same root cause:

walkVM calling inserting lookup from a signal handler — malloc-arena tearing under signal preemption.
writeClasses reading the Dictionary chain unsynchronised against the dump-path clear().

The vtable_target feature is on by default, so the buggy site is reachable on every CPU/wall/native-alloc sample whose top frame is a vtable stub.

Additional Notes:

The BCI_ALLOC reuse at hotspotSupport.cpp:388 is semantic overloading (frame marker, not allocation event). It was downported from upstream async-profiler and is preserved as-is here — only the signal-safety of the lookup is changed.
The same hazard exists in upstream src/stackWalker.cpp:399-405. Filing the equivalent fix upstream is a follow-up.
Trade-off: first-occurrence vtable-target annotations for cold classes are temporarily lost until the JVMTI / Java-API path inserts the class via Profiler::lookupClass. Eliminating the SIGSEGV outweighs the cosmetic loss.

How to test the change?:

New unit/stress tests at ddprof-lib/src/test/cpp/dictionary_concurrent_ut.cpp cover:
1. bounded_lookup(..., 0) on a miss returns INT_MAX and does not insert.
2. bounded_lookup(..., 0) on a hit returns the same id that the inserting lookup produced.
3. ~500ms concurrent run with 4 shared-lock inserter threads, one shared-lock collector thread, and one exclusive-lock clear thread — regression gate for the lock discipline (PROF-14549).
4. ~500ms concurrent run with 4 OptionalSharedLockGuard + bounded_lookup(0) "signal-handler" threads vs one exclusive-lock clear thread — regression gate for the PROF-14550 dump-path crash signature.
Existing CPU / wall / native-alloc / JVMTI alloc / liveness profile tests continue to pass.

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-14549, PROF-14550

🤖 Generated with Claude Code

walkVM ran the inserting Dictionary::lookup from a signal handler, calling malloc/calloc — async-signal-unsafe. Recording::writeClasses also walked _class_map without holding _class_map_lock, racing the exclusive clear() that follows in Profiler::dump. Switch the vtable-target site to bounded_lookup(..., 0) and skip the synthetic frame on miss; hold _class_map_lock shared while writeClasses recurses into Dictionary::collect; document the signal-safety hazard on the inserting overloads; add a concurrent test for the lock discipline. Fixes PROF-14549. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

jbachorik · 2026-05-07T10:29:01Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:7880e7ce7f

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

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Codex flagged a remaining race after the initial fix: walkVM's read-only bounded_lookup still traverses row->next and reads row->keys[j] without holding _class_map_lock, while Profiler::dump's exclusive _class_map.clear() runs in the unlockAll -> lock window and free()s those same nodes/strings. Take _class_map_lock shared via OptionalSharedLockGuard (try-lock so the signal handler never blocks). When an exclusive clear() is in progress, drop the synthetic vtable-target frame instead of dereferencing freed memory. Also add the missing <cstring> include in dictionary_concurrent_ut.cpp; strlen() previously worked only via a transitive include through gtest_crash_handler.h. Addresses review comments on PR #512. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

dd-octo-sts · 2026-05-07T11:35:07Z

CI Test Results

Run:#28552205361 | Commit:c5adab3 | Duration: 0s (longest job)

✅ All 0 test jobs passed

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

Updated: 2026-07-01 22:39:57 UTC

flightRecorder.cpp: refer to classMap()/classMapLock() rather than the underlying _class_map/_class_map_lock fields. dictionary_concurrent_ut.cpp: drop PROF-14549 ticket reference. Co-Authored-By: muse <muse@noreply>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

… vs dump clear Adds SignalHandlerBoundedLookupVsDumpClear: concurrent signal-handler threads (OptionalSharedLockGuard + bounded_lookup(0)) vs a dump thread (exclusive lock + dict.clear()), reproducing the PROF-14550 crash signature without the fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Codex flagged a remaining race after the initial fix: walkVM's read-only bounded_lookup still traverses row->next and reads row->keys[j] without holding _class_map_lock, while Profiler::dump's exclusive _class_map.clear() runs in the unlockAll -> lock window and free()s those same nodes/strings. Take _class_map_lock shared via OptionalSharedLockGuard (try-lock so the signal handler never blocks). When an exclusive clear() is in progress, drop the synthetic vtable-target frame instead of dereferencing freed memory. Also add the missing <cstring> include in dictionary_concurrent_ut.cpp; strlen() previously worked only via a transitive include through gtest_crash_handler.h. Addresses review comments on PR #512. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

rkennke

Looks good. Thank you!

… handler and dump path (#516)

…4618) Restores vtable_target frame capture in CPU-only recordings. After PR #512 made walkVM signal-safe (bounded_lookup with size_limit=0, no malloc), _class_map was only populated by allocation/liveness paths via lookupClass. In a CPU-only recording these paths never fire, so signal-safe lookups always missed and vtable-target frames were silently dropped. This change pre-populates _class_map from safe JVM-thread contexts: - Profiler::preregisterLoadedClasses(jvmtiEnv*) calls GetLoadedClasses + GetClassSignature + ObjectSampler::normalizeClassSignature, inserting every reference class into _class_map. Invoked at Profiler::start() (after clearAll()) and Profiler::dump() (after clearStandby()). TripleBufferedDictionary::lookup is thread-safe via RefCountGuard so no external locking is needed. - VM::ClassPrepare is moved out-of-line to vmEntry.cpp (vmEntry.h is included by profiler.h, hence the prior inline definition could not reference Profiler::instance()). The handler still calls loadMethodIDs first, then conditionally inserts the new class when _features.vtable_target is set. The hot signal-handler path in hotspotSupport.cpp is unchanged — pre-registration runs only on JVM threads, never under a signal. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

jbachorik added the AI label May 7, 2026

jbachorik requested a review from Copilot May 7, 2026 10:12

jbachorik changed the title ~~fix(profiler): make _class_map access signal-safe in walkVM (PROF-14549)~~fix(profiler): make _class_map access signal-safe in walkVMMay 7, 2026

Copilotstarted reviewing on behalf of jbachorik May 7, 2026 10:13 View session

This comment was marked as outdated.
Sign in to view

jbachorik force-pushed the muse/sigsegv-in-recording-writeclasses branch from f3e2ccb to 7880e7cCompare May 7, 2026 10:25

jbachorik requested a review from Copilot May 7, 2026 10:28

Copilotstarted reviewing on behalf of jbachorik May 7, 2026 10:29 View session

chatgpt-codex-connectorBot reviewed May 7, 2026
View reviewed changes

Comment threadddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp Outdated

CopilotAI reviewed May 7, 2026
View reviewed changes

Comment threadddprof-lib/src/test/cpp/dictionary_concurrent_ut.cpp

jbachorik commented May 7, 2026
View reviewed changes

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

docs: align comments with accessor names; drop JIRA reference
bc05222
flightRecorder.cpp: refer to classMap()/classMapLock() rather than the underlying _class_map/_class_map_lock fields. dictionary_concurrent_ut.cpp: drop PROF-14549 ticket reference. Co-Authored-By: muse <muse@noreply>

jbachorik force-pushed the muse/sigsegv-in-recording-writeclasses branch from 8f59e1a to bc05222Compare May 7, 2026 20:22

jbachorik requested a review from Copilot May 7, 2026 20:23

Copilotstarted reviewing on behalf of jbachorik May 7, 2026 20:24 View session

CopilotAI reviewed May 7, 2026
View reviewed changes

Comment threadddprof-lib/src/test/cpp/dictionary_concurrent_ut.cpp Outdated

Address comments
9707596
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

jbachorik marked this pull request as ready for review May 7, 2026 20:32

jbachorik requested a review from a team as a code owner May 7, 2026 20:32

Merge branch 'main' into muse/sigsegv-in-recording-writeclasses
b80d1d6

jbachorik added the AI: Author Signed-off label May 7, 2026

rkennke approved these changes May 11, 2026
View reviewed changes

fix(profiler): eliminate SIGSEGV race on _class_map.clear() in signal…
cb6c809
… handler and dump path (#516)

jbachorik merged commit c5adab3 into mainMay 12, 2026
98 checks passed

jbachorik deleted the muse/sigsegv-in-recording-writeclasses branch May 12, 2026 11:02

github-actionsBot added this to the 1.43.0 milestone May 12, 2026

This was referenced May 12, 2026
test(jfr): regression test for dump+restart race (SIGSEGV in _Rb_tree_increment) #523
Closed
refactor(vtable_target): defer receiver resolution to dump time via SafeAccess #527
Merged

jbachorik added the identified-by:crashtracking Issue identified via crash tracking label Jul 1, 2026

Navigation Menu

Uh oh!

fix(profiler): make _class_map access signal-safe in walkVM - #512

fix(profiler): make _class_map access signal-safe in walkVM#512
jbachorik merged 7 commits into
mainfrom
muse/sigsegv-in-recording-writeclasses

jbachorik commented May 7, 2026•
edited by atlassian Bot
Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

jbachorik commented May 7, 2026

Uh oh!

chatgpt-codex-connectorBot left a comment

Uh oh!

Uh oh!

CopilotAI left a comment

Uh oh!

Uh oh!

dd-octo-stsBot commented May 7, 2026•
edited
Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Uh oh!

Uh oh!

rkennke left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jbachorik commented May 7, 2026• edited by atlassian BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

jbachorik commented May 7, 2026

Uh oh!

chatgpt-codex-connectorBot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

dd-octo-stsBot commented May 7, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Test Results

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

rkennke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jbachorik commented May 7, 2026•
edited by atlassian Bot
Loading

dd-octo-stsBot commented May 7, 2026•
edited
Loading