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

Author: jbachorik

ddprof-lib/src/main/cpp/dictionary.h

@@ -75,6 +75,10 @@ class Dictionary {
   void clear();
 
   bool         check(const char* key);
+  // NOT signal-safe: the inserting lookup overloads call malloc/calloc on miss
+  // (see allocateKey and the calloc in dictionary.cpp). Signal handlers must use
+  // bounded_lookup(key, length, 0) instead, which never inserts and returns
+  // INT_MAX on miss.
   unsigned int lookup(const char *key);
   unsigned int lookup(const char *key, size_t length);
   unsigned int bounded_lookup(const char *key, size_t length, int size_limit);

ddprof-lib/src/main/cpp/flightRecorder.cpp

@@ -1152,9 +1152,10 @@ void Recording::writeCpool(Buffer *buf) {
   // constant pool count - bump each time a new pool is added
   buf->put8(12);
 
-  // Profiler::instance()->classMap() provides access to non-locked _class_map
-  // instance The non-locked access is ok here as this code will never run
-  // concurrently to _class_map.clear()
+  // classMap() is shared across the dump (this thread) and the JVMTI shared-lock
+  // writers (Profiler::lookupClass and friends). writeClasses() holds
+  // classMapSharedGuard() for its full duration; the exclusive classMap()->clear()
+  // in Profiler::dump runs only after this method returns.
   Lookup lookup(this, &_method_map, Profiler::instance()->classMap());
   writeFrameTypes(buf);
   writeThreadStates(buf);
@@ -1368,8 +1369,11 @@ void Recording::writeMethods(Buffer *buf, Lookup *lookup) {
 
 void Recording::writeClasses(Buffer *buf, Lookup *lookup) {
   std::map<u32, const char *> classes;
-  // no need to lock _classes as this code will never run concurrently with
-  // resetting that dictionary
+  // Hold classMapSharedGuard() for the full function. The const char* pointers
+  // stored in classes point into dictionary row storage; clear() frees that
+  // storage under the exclusive lock, so we must not release the shared lock
+  // until we have finished iterating.
+  auto guard = Profiler::instance()->classMapSharedGuard();
   lookup->_classes->collect(classes);
 
   buf->putVar64(T_CLASS);

ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp

@@ -4,6 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+#include <climits>
 #include <cstdlib>
 #include <setjmp.h>
 #include "asyncSampleMutex.h"
@@ -530,8 +531,23 @@ __attribute__((no_sanitize("address"))) int HotspotSupport::walkVM(void* ucontex
                     uintptr_t receiver = frame.jarg0();
                     if (receiver != 0) {
                         VMSymbol* symbol = VMKlass::fromOop(receiver)->name();
-                        u32 class_id = profiler->classMap()->lookup(symbol->body(), symbol->length());
-                        fillFrame(frames[depth++], BCI_ALLOC, class_id);
+                        // walkVM runs in a signal handler. _class_map is mutated
+                        // under _class_map_lock (shared by Profiler::lookupClass
+                        // inserters, exclusive by _class_map.clear() in the dump
+                        // path between unlockAll() and lock()). bounded_lookup
+                        // with size_limit=0 never inserts (no malloc), but it
+                        // still traverses row->next and reads row->keys, which
+                        // clear() concurrently frees. Take the lock shared via
+                        // try-lock; if an exclusive clear() is in progress, drop
+                        // the synthetic frame rather than read freed memory.
+                        auto guard = profiler->classMapTrySharedGuard();
+                        if (guard.ownsLock()) {
+                            u32 class_id = profiler->classMap()->bounded_lookup(
+                                symbol->body(), symbol->length(), 0);
+                            if (class_id != INT_MAX) {
+                                fillFrame(frames[depth++], BCI_ALLOC, class_id);
+                            }
+                        }
                     }
                 }
 

ddprof-lib/src/main/cpp/profiler.h

@@ -204,6 +204,8 @@ class alignas(alignof(SpinLock)) Profiler {
   Engine *wallEngine() { return _wall_engine; }
 
   Dictionary *classMap() { return &_class_map; }
+  SharedLockGuard classMapSharedGuard() { return SharedLockGuard(&_class_map_lock); }
+  BoundedOptionalSharedLockGuard classMapTrySharedGuard() { return BoundedOptionalSharedLockGuard(&_class_map_lock); }
   Dictionary *stringLabelMap() { return &_string_label_map; }
   Dictionary *contextValueMap() { return &_context_value_map; }
   u32 numContextAttributes() { return _num_context_attributes; }

ddprof-lib/src/main/cpp/spinLock.h

@@ -47,11 +47,30 @@ class alignas(DEFAULT_CACHE_LINE_SIZE) SpinLock {
   void unlock() { __sync_fetch_and_sub(&_lock, 1); }
 
   bool tryLockShared() {
+    // Spins while no exclusive lock is held and the CAS to acquire a shared
+    // lock fails (due to concurrent reader contention). Returns false ONLY when
+    // an exclusive lock is observed (_lock > 0); never returns false spuriously.
     int value;
     while ((value = __atomic_load_n(&_lock, __ATOMIC_ACQUIRE)) <= 0) {
       if (__sync_bool_compare_and_swap(&_lock, value, value - 1)) {
         return true;
       }
+      spinPause();
+    }
+    return false;
+  }
+
+  // Signal-safe variant: returns false after at most 5 CAS attempts.
+  // Use only in signal-handler paths where spinning indefinitely is unsafe.
+  bool tryLockSharedBounded() {
+    for (int attempts = 0; attempts < 5; ++attempts) {
+      int value = __atomic_load_n(&_lock, __ATOMIC_ACQUIRE);
+      if (value > 0) {
+        return false;
+      }
+      if (__sync_bool_compare_and_swap(&_lock, value, value - 1)) {
+        return true;
+      }
     }
     return false;
   }
@@ -88,9 +107,9 @@ class SharedLockGuard {
 class OptionalSharedLockGuard {
   SpinLock* _lock;
 public:
-  OptionalSharedLockGuard(SpinLock* lock) : _lock(lock) {
+  explicit OptionalSharedLockGuard(SpinLock* lock) : _lock(lock) {
     if (!_lock->tryLockShared()) {
-      // Locking failed, no need to unlock.
+      // Exclusive lock is held; no unlock needed. Only fails when an exclusive lock is observed.
       _lock = nullptr;
     }
   }
@@ -108,6 +127,33 @@ class OptionalSharedLockGuard {
   OptionalSharedLockGuard& operator=(OptionalSharedLockGuard&&) = delete;
 };
 
+// Signal-safe variant of OptionalSharedLockGuard: uses bounded CAS retries
+// and may fail spuriously when racing other shared lockers. Use ONLY in
+// signal-handler paths where spinning indefinitely is unsafe; do NOT use
+// in hot recording paths where silent acquisition failures would drop events.
+class BoundedOptionalSharedLockGuard {
+  SpinLock* _lock;
+public:
+  explicit BoundedOptionalSharedLockGuard(SpinLock* lock) : _lock(lock) {
+    if (!_lock->tryLockSharedBounded()) {
+      // Locking failed (bounded retries exhausted or exclusive lock held); no unlock needed.
+      _lock = nullptr;
+    }
+  }
+  ~BoundedOptionalSharedLockGuard() {
+    if (_lock != nullptr) {
+      _lock->unlockShared();
+    }
+  }
+  bool ownsLock() { return _lock != nullptr; }
+
+  // Non-copyable and non-movable
+  BoundedOptionalSharedLockGuard(const BoundedOptionalSharedLockGuard&) = delete;
+  BoundedOptionalSharedLockGuard& operator=(const BoundedOptionalSharedLockGuard&) = delete;
+  BoundedOptionalSharedLockGuard(BoundedOptionalSharedLockGuard&&) = delete;
+  BoundedOptionalSharedLockGuard& operator=(BoundedOptionalSharedLockGuard&&) = delete;
+};
+
 class ExclusiveLockGuard {
 private:
   SpinLock* _lock;