fix(profiler): fix SIGSEGV in Dictionary::clear under concurrent lookup by jbachorik · Pull Request #522 · DataDog/java-profiler

jbachorik · 2026-05-11T15:58:58Z

Summary

hotspotSupport.cpp:533 called profiler->classMap()->lookup() directly in walkVM's signal-handler path without holding _class_map_lock. Concurrent Dictionary::clear() (called from Profiler::dump and chunk rotation under the exclusive lock) freed row->keys[] and row->next chains mid-traversal, causing a SIGSEGV. The fix routes all class lookups through a new lookupClassSignalSafe() method that acquires _class_map_lock shared before calling a malloc-free read-only lookup, eliminating the race.

Changes

dictionary.cpp/h: Add lookup_readonly(key, len) — for_insert=false, sentinel=0; never calls malloc; returns 0 on miss (async-signal-safe).
profiler.cpp/h: Add lookupClassSignalSafe(key, len) — tryLockShared + lookup_readonly, returns -1 when lock unavailable. Keep lookupClass() with inserting semantics for JVMTI/JNI callers.
hotspot/hotspotSupport.cpp:533: Replace direct classMap()->lookup() with lookupClassSignalSafe(); guard fillFrame on class_id > 0 for graceful frame skip.
stress_dictionary.cpp: Race lookup_readonly against clear() under lock for 500 ms / 1000 ms, 8 threads — must not SIGSEGV.
hotspotSupport_vtable_lookup_ut.cpp: Static text checks (forbidden pattern absent, required call present) + functional tests for lookup_readonly semantics.
vtable_graceful_skip_ut.cpp: Verifies class_id > 0 guard — frames emitted only for valid IDs, skipped for 0 and -1.

Acceptance Criteria

hotspotSupport.cpp:533 uses Profiler::lookupClassSignalSafe() instead of profiler->classMap()->lookup() directly (signal-safe variant with lock protection)
When lookupClassSignalSafe() returns -1 (clear in progress), no vtable-target frame is added (depth not incremented) — graceful skip via class_id > 0 guard
New GTest stress_dictionary.cpp stress-tests concurrent lookup threads vs. clear-under-lock thread for at least 500 ms without SIGSEGV
The test would crash (UAF) on the unfixed code and passes cleanly on the fixed code

What does this PR do?:
Fixes SIGSEGV in Dictionary::clear() — a use-after-free caused by hotspotSupport.cpp:walkVM calling classMap()->lookup() in a signal handler without holding _class_map_lock. Adds a signal-safe lookup path that holds the shared lock for the duration of the read.

Motivation:
5 crash events in 2 days with top frame Dictionary::clear(). Signal handler was bypassing the existing lock protocol.

Additional Notes:

lookup_readonly is async-signal-safe: no malloc, 0 on miss (base_index=1 so 0 is unambiguous)
lookupClass() inserting semantics preserved for JVMTI/JNI callers (objectSampler.cpp, etc.)
class_id > 0 guard handles both miss (0) and lock-unavailable (-1) as graceful frame skips

How to test the change?:
Three new C++ GTest files:

stress_dictionary.cpp: concurrent clear vs. lookup_readonly for 500 ms/1000 ms across 8 threads
hotspotSupport_vtable_lookup_ut.cpp: static call-site checks + lookup_readonly semantics
vtable_graceful_skip_ut.cpp: class_id > 0 guard correctness

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-14582

Unsure? Have a question? Request a review!

🤖 Generated with Claude Code via muse implement

The vtable-stub branch in HotspotSupport::walkVM called classMap()->lookup() directly in an async-signal-handler context without holding _class_map_lock. A concurrent Profiler::clearCallTraces() call that acquired the exclusive lock and freed the dictionary's key storage caused a UAF SIGSEGV. Fix: route the signal-handler path through a new Profiler::lookupClassSignalSafe() which acquires a shared lock via tryLockShared() and uses Dictionary::lookup_readonly() (for_insert=false, sentinel=0) — never calling malloc, fully async-signal-safe. The existing Profiler::lookupClass() retains the inserting overload for JVMTI/JNI callers (objectSampler, livenessTracker, javaApi) that rely on class auto-registration and guard only id == -1 as the error sentinel. Tests added: - vtable_graceful_skip_ut.cpp: verifies the class_id > 0 guard in the vtable-stub branch - stress_dictionary.cpp: concurrent locked-lookup vs clear, extended duration, and lookup_readonly non-insertion invariant - hotspotSupport_vtable_lookup_ut.cpp: static check + functional tests for Dictionary::lookup_readonly semantics Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dd-octo-sts · 2026-05-11T19:06:05Z

CI Test Results

Run:#25734411251 | Commit:5eaeb53 | Duration: 3h 0m 39s (longest job)

❌ 6 of 32 test jobs failed

Status Overview

JDK	glibc-aarch64/debug	glibc-amd64/debug	musl-aarch64/debug	musl-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