Harden CodeCacheArray and profiler against signal-handler races (#413)

* Harden CodeCacheArray and profiler against signal-handler races

- Guard CodeCacheArray readers against NULL during concurrent add()
- Use CAS loop in add() to prevent exceeding MAX_NATIVE_LIBS
- Make at() non-blocking (return NULL instead of spinning)
- Cache anchor() in getJavaTraceAsync() to eliminate TOCTOU
- Add NULL guards on all anchor() dereferences in signal context
- Stop at first NULL in writeNativeLibraries() to avoid skipping entries
- Skip patching sanitizer runtime libs to prevent heap corruption
- Add NULL guard for lib->name() in patch_library_unlocked()

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Block profiling signals around RoutineInfo new/delete

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Address PR feedback: fix at() indentation, simplify writeNativeLibraries

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Harden J9+ASAN tests: forkEvery=1 and CI retry

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* ASAN test timeout, aarch64 retry parity, gdb watchdog

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Fix arm64 lockups: remove RT signal blocking, cap FP-chain and anchor recovery

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Address PR feedback: ACQUIRE ordering, assert in add(), comment fix

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Add arm64 ordering, concurrent iteration, and comment precision rules

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Pointer-first CodeCacheArray::add() eliminates NULL window

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Author: jbachorik

.github/scripts/prepare_reports.sh

@@ -12,6 +12,7 @@ cp ddprof-test/javacore*.txt test-reports/ || true
 cp ddprof-test/build/hs_err* test-reports/ || true
 cp -r ddprof-lib/build/tmp test-reports/native_build || true
 cp -r ddprof-test/build/reports/tests test-reports/tests || true
+cp build/logs/gdb-watchdog.log test-reports/ || true
 cp -r /tmp/recordings test-reports/recordings || true
 find ddprof-lib/build -name 'libjavaProfiler.*' -exec cp {} test-reports/ \; || true
 

.github/workflows/test_workflow.yml

@@ -104,12 +104,25 @@ jobs:
           export LIBC=glibc
           export SANITIZER=${{ matrix.config }}
 
-          mkdir -p build/logs
-          ./gradlew -PCI -PkeepJFRs :ddprof-test:test${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs 2>&1 \
-            | tee -a build/test-raw.log \
-            | python3 -u .github/scripts/filter_gradle_log.py
-          EXIT_CODE=${PIPESTATUS[0]}
-      
+          MAX_ATTEMPTS=1
+          if [[ "${{ matrix.config }}" == "asan" && "${{ matrix.java_version }}" =~ (j9|ibm) ]]; then
+            MAX_ATTEMPTS=2
+          fi
+
+          for attempt in $(seq 1 $MAX_ATTEMPTS); do
+            mkdir -p build/logs
+            ./gradlew -PCI -PkeepJFRs :ddprof-test:test${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs 2>&1 \
+              | tee -a build/test-raw.log \
+              | python3 -u .github/scripts/filter_gradle_log.py
+            EXIT_CODE=${PIPESTATUS[0]}
+
+            if [ $EXIT_CODE -eq 0 ]; then break; fi
+            if [ $attempt -lt $MAX_ATTEMPTS ]; then
+              echo "::warning::Attempt $attempt failed (exit $EXIT_CODE), retrying..."
+              ./gradlew --stop 2>/dev/null || true
+            fi
+          done
+
           if [ $EXIT_CODE -ne 0 ]; then
             echo "glibc-${{ matrix.java_version }}-${{ matrix.config }}-amd64" >> failures_glibc-${{ matrix.java_version }}-${{ matrix.config }}-amd64.txt
             exit 1
@@ -327,7 +340,7 @@ jobs:
           sudo apt update -y
           sudo apt remove -y g++
           sudo apt autoremove -y
-          sudo apt install -y curl zip unzip clang make build-essential binutils
+          sudo apt install -y curl zip unzip clang make build-essential binutils gdb
           # Install debug symbols for system libraries
           sudo apt install -y libc6-dbg
           if [[ ${{ matrix.java_version }} =~ "-zing" ]]; then
@@ -350,11 +363,44 @@ jobs:
           export LIBC=glibc
           export SANITIZER=${{ matrix.config }}
 
-          mkdir -p build/logs
-          ./gradlew -PCI -PkeepJFRs :ddprof-test:test${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs 2>&1 \
-            | tee -a build/test-raw.log \
-            | python3 -u .github/scripts/filter_gradle_log.py
-          EXIT_CODE=${PIPESTATUS[0]}
+          # For ASAN: launch a gdb watchdog that dumps all native threads before the
+          # 30-minute Gradle timeout kills the JVM, so we can diagnose hangs in native code.
+          if [[ "${{ matrix.config }}" == "asan" ]]; then
+            mkdir -p build/logs
+            (
+              sleep 1500  # 25 minutes — fires before the 30-min Gradle timeout
+              echo "::warning::GDB watchdog triggered after 25 minutes"
+              for pid in $(pgrep -f 'java.*ddprof-test'); do
+                echo "=== Thread dump for PID $pid ===" >> build/logs/gdb-watchdog.log
+                gdb -batch -ex 'thread apply all bt full' -p "$pid" >> build/logs/gdb-watchdog.log 2>&1 || true
+              done
+            ) &
+            GDB_WATCHDOG_PID=$!
+          fi
+
+          MAX_ATTEMPTS=1
+          if [[ "${{ matrix.config }}" == "asan" && "${{ matrix.java_version }}" =~ (j9|ibm) ]]; then
+            MAX_ATTEMPTS=2
+          fi
+
+          for attempt in $(seq 1 $MAX_ATTEMPTS); do
+            mkdir -p build/logs
+            ./gradlew -PCI -PkeepJFRs :ddprof-test:test${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs 2>&1 \
+              | tee -a build/test-raw.log \
+              | python3 -u .github/scripts/filter_gradle_log.py
+            EXIT_CODE=${PIPESTATUS[0]}
+
+            if [ $EXIT_CODE -eq 0 ]; then break; fi
+            if [ $attempt -lt $MAX_ATTEMPTS ]; then
+              echo "::warning::Attempt $attempt failed (exit $EXIT_CODE), retrying..."
+              ./gradlew --stop 2>/dev/null || true
+            fi
+          done
+
+          # Kill the watchdog if tests finished before it fired
+          if [[ -n "${GDB_WATCHDOG_PID:-}" ]]; then
+            kill "$GDB_WATCHDOG_PID" 2>/dev/null || true
+          fi
 
           if [ $EXIT_CODE -ne 0 ]; then
             echo "glibc-${{ matrix.java_version }}-${{ matrix.config }}-aarch64" >> failures_glibc-${{ matrix.java_version }}-${{ matrix.config }}-aarch64.txt

AGENTS.md

@@ -357,6 +357,17 @@ The profiler uses a sophisticated double-buffered storage system for call traces
 - **Atomic Operations**: Instance ID management and counter updates use atomics
 - **Memory Allocation**: Minimize malloc() in hot paths, use pre-allocated containers
 
+### Atomic Memory Ordering (Critical for arm64)
+arm64 has a weakly-ordered memory model (unlike x86 TSO). Incorrect ordering causes real lockups on arm64 that never reproduce on x86.
+- **Cross-thread reads**: Always use `__ATOMIC_ACQUIRE` for loads that must see stores from another thread. Never use `__ATOMIC_RELAXED` for cross-thread visibility unless you can prove no ordering dependency exists.
+- **Cross-thread writes**: Use `__ATOMIC_RELEASE` for stores that must be visible to other threads. Pair with `__ATOMIC_ACQUIRE` loads.
+- **Count + pointer patterns**: When a data structure publishes a count and a separate pointer (two-phase add), both the count load and pointer load need acquire semantics so the reader sees the pointer store that preceded the count increment.
+- **Default stance**: When in doubt, use acquire/release. The performance cost is negligible; the correctness cost of relaxed ordering bugs is enormous (silent arm64-only lockups).
+
+### Concurrent Data Structure Iteration
+- **NULL gaps**: When iterating a concurrent array (e.g., `CodeCacheArray`), always NULL-check each slot — a slot may be count-allocated but pointer-not-yet-stored.
+- **Cursor advancement**: Never permanently advance an iteration cursor past NULL gaps. Stop at the first NULL or track the last contiguous non-NULL entry, so missing entries are retried on the next pass.
+
 ### 64-bit Trace ID System
 - **Collision Avoidance**: Instance-based IDs prevent collisions across storage swaps
 - **JFR Compatibility**: 64-bit IDs work with JFR constant pool indices
@@ -705,3 +716,9 @@ The CI caches JDKs via `.github/workflows/cache_java.yml`. When adding a new JDK
 - Always provide tests for bug fixes - test fails before the fix, passes after the fix
 - All code needs to strive to be lean in terms of resources consumption and easy to follow -
     do not shy away from factoring out self containing code to shorter functions with explicit name
+
+### C/C++ Code Style
+- **Indentation**: Match the exact indentation style of the surrounding code in each file. Do not introduce inconsistent indentation — reviewers will flag it.
+- **Minimal complexity**: Do not split inline logic into separate helper functions unless the helpers are reused or the original is genuinely hard to follow. Unnecessary splits add indirection without value.
+- **Comment precision**: Comments explaining "why" must reference concrete mechanisms (e.g., "ASAN's allocator lock is reentrant" not "internal bookkeeping"). Vague comments get challenged in review. Every claim in a comment must be verifiable from the code or documented behavior of the referenced system (ASAN, glibc NPTL, HotSpot, etc.).
+- **No speculative comments**: Do not claim a system (HotSpot, glibc, ASAN) uses a specific mechanism unless you are certain. If unsure, describe the observable symptom instead of guessing the cause.

build-logic/conventions/src/main/kotlin/com/datadoghq/native/util/PlatformUtils.kt

@@ -304,4 +304,22 @@ object PlatformUtils {
             "or specify one with -Pnative.forceCompiler=/path/to/compiler"
         )
     }
+
+    /**
+     * Returns true if the test JVM (from JAVA_TEST_HOME or JAVA_HOME) is an OpenJ9/J9 JVM.
+     * Probes `java -version` stderr output for "J9" or "OpenJ9".
+     */
+    fun isTestJvmJ9(): Boolean {
+        val javaHome = testJavaHome()
+        return try {
+            val process = ProcessBuilder("$javaHome/bin/java", "-version")
+                .redirectErrorStream(true)
+                .start()
+            val output = process.inputStream.bufferedReader().readText()
+            process.waitFor(10, TimeUnit.SECONDS)
+            output.contains("J9") || output.contains("OpenJ9")
+        } catch (_: Exception) {
+            false
+        }
+    }
 }

build-logic/conventions/src/main/kotlin/com/datadoghq/profiler/ProfilerTestPlugin.kt

@@ -15,6 +15,7 @@ import org.gradle.api.provider.Property
 import org.gradle.api.tasks.Exec
 import org.gradle.api.tasks.SourceSetContainer
 import org.gradle.api.tasks.testing.Test
+import java.time.Duration
 import javax.inject.Inject
 
 /**
@@ -254,6 +255,17 @@ class ProfilerTestPlugin : Plugin<Project> {
                 "asan" -> testTask.onlyIf { PlatformUtils.locateLibasan() != null }
                 "tsan" -> testTask.onlyIf { PlatformUtils.locateLibtsan() != null }
             }
+
+            // J9+ASAN: isolate each test class in its own JVM to limit crash blast radius
+            if (testConfig.configName == "asan") {
+                testTask.doFirst {
+                    if (PlatformUtils.isTestJvmJ9()) {
+                        testTask.setForkEvery(1)
+                    }
+                }
+                // Kill hung ASAN test tasks after 30 minutes
+                testTask.timeout.set(Duration.ofMinutes(30))
+            }
         }
     }
 

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

@@ -254,34 +254,47 @@ class CodeCache {
 class CodeCacheArray {
 private:
   CodeCache *_libs[MAX_NATIVE_LIBS];
-  volatile int _count;
+  volatile int _reserved;       // next slot to reserve (CAS by writers)
+  volatile int _count;          // published count (all indices < _count have non-NULL pointers)
   volatile size_t _used_memory;
 
 public:
-  CodeCacheArray() : _count(0), _used_memory(0) {
+  CodeCacheArray() : _reserved(0), _count(0), _used_memory(0) {
     memset(_libs, 0, MAX_NATIVE_LIBS * sizeof(CodeCache *));
   }
 
   CodeCache *operator[](int index) const { return __atomic_load_n(&_libs[index], __ATOMIC_ACQUIRE); }
 
-  int count() const { return __atomic_load_n(&_count, __ATOMIC_RELAXED); }
+  // All indices < count() are guaranteed to have a non-NULL pointer.
+  int count() const { return __atomic_load_n(&_count, __ATOMIC_ACQUIRE); }
 
+  // Pointer-first add: reserve a slot via CAS on _reserved, store the
+  // pointer with RELEASE, then advance _count. Readers see count() grow
+  // only after the pointer is visible, so indices < count() never yield NULL.
   void add(CodeCache *lib) {
-    int index = __atomic_fetch_add(&_count, 1, __ATOMIC_RELAXED);
-    if (index < MAX_NATIVE_LIBS) {
-      __atomic_fetch_add(&_used_memory, lib->memoryUsage(), __ATOMIC_RELAXED);
-       __atomic_store_n(&_libs[index], lib, __ATOMIC_RELEASE);
+    int slot = __atomic_load_n(&_reserved, __ATOMIC_RELAXED);
+    do {
+      if (slot >= MAX_NATIVE_LIBS) return;
+    } while (!__atomic_compare_exchange_n(&_reserved, &slot, slot + 1,
+                                          true, __ATOMIC_RELAXED, __ATOMIC_RELAXED));
+    assert(__atomic_load_n(&_libs[slot], __ATOMIC_RELAXED) == nullptr);
+    __atomic_fetch_add(&_used_memory, lib->memoryUsage(), __ATOMIC_RELAXED);
+    // Store pointer before publishing count. The RELEASE here pairs with
+    // the ACQUIRE load in operator[]/at() and count().
+    __atomic_store_n(&_libs[slot], lib, __ATOMIC_RELEASE);
+    // Advance _count to publish the new slot. Spin until our slot is next
+    // in line, preserving contiguous ordering when multiple adds race.
+    while (__atomic_load_n(&_count, __ATOMIC_RELAXED) != slot) {
+      // wait for preceding slots to publish
     }
+    __atomic_store_n(&_count, slot + 1, __ATOMIC_RELEASE);
   }
 
   CodeCache* at(int index) const {
     if (index >= MAX_NATIVE_LIBS) {
-        return nullptr;
+      return nullptr;
     }
-    CodeCache* lib = nullptr;
-    while ((lib = __atomic_load_n(&_libs[index], __ATOMIC_ACQUIRE)) == nullptr);
-
-    return lib;
+    return __atomic_load_n(&_libs[index], __ATOMIC_ACQUIRE);
   }
 
   size_t memoryUsage() const {

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

@@ -1110,10 +1110,12 @@ void Recording::writeNativeLibraries(Buffer *buf) {
   const CodeCacheArray &native_libs = libraries->native_libs();
   int native_lib_count = native_libs.count();
 
+  // Emit jdk.NativeLibrary events for newly loaded libraries.
+  // CodeCacheArray::add() stores the pointer before advancing count(),
+  // so all indices < native_lib_count are guaranteed non-NULL.
   for (int i = _recorded_lib_count; i < native_lib_count; i++) {
     CodeCache* lib = native_libs[i];
 
-    // Emit jdk.NativeLibrary event with extended fields (buildId and loadBias)
     flushIfNeeded(buf, RECORDING_BUFFER_LIMIT - MAX_STRING_LENGTH);
     int start = buf->skip(5);
     buf->putVar64(T_NATIVE_LIBRARY);

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

@@ -121,16 +121,12 @@ class SignalBlocker {
     sigset_t prof_signals;
     sigemptyset(&prof_signals);
 
-    // Add all profiling signals that could interrupt us
+    // Block only the profiling signals that the profiler actually registers.
+    // No profiler engine uses RT signals, so blocking them is unnecessary
+    // and risks interfering with glibc NPTL internals (SIGRTMIN, SIGRTMIN+1)
+    // or other JVM-internal signal usage.
     sigaddset(&prof_signals, SIGPROF);     // Used by ITimer and CTimer
     sigaddset(&prof_signals, SIGVTALRM);   // Used by WallClock
-#ifdef __linux__
-    // Block RT signals (Linux-only)
-    // This prevents any RT signal handler from interrupting TLS initialization
-    for (int sig = SIGRTMIN; sig <= SIGRTMIN + 5 && sig <= SIGRTMAX; sig++) {
-      sigaddset(&prof_signals, sig);
-    }
-#endif
 
     if (pthread_sigmask(SIG_BLOCK, &prof_signals, &_old_mask) == 0) {
       _active = true;

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

@@ -54,16 +54,22 @@ const void *Libraries::resolveSymbol(const char *name) {
   int native_lib_count = _native_libs.count();
   if (len > 0 && name[len - 1] == '*') {
     for (int i = 0; i < native_lib_count; i++) {
-      const void *address = _native_libs[i]->findSymbolByPrefix(name, len - 1);
-      if (address != NULL) {
-        return address;
+      CodeCache *lib = _native_libs[i];
+      if (lib != NULL) {
+        const void *address = lib->findSymbolByPrefix(name, len - 1);
+        if (address != NULL) {
+          return address;
+        }
       }
     }
   } else {
     for (int i = 0; i < native_lib_count; i++) {
-      const void *address = _native_libs[i]->findSymbol(name);
-      if (address != NULL) {
-        return address;
+      CodeCache *lib = _native_libs[i];
+      if (lib != NULL) {
+        const void *address = lib->findSymbol(name);
+        if (address != NULL) {
+          return address;
+        }
       }
     }
   }
@@ -79,11 +85,14 @@ CodeCache *Libraries::findLibraryByName(const char *lib_name) {
   const size_t lib_name_len = strlen(lib_name);
   const int native_lib_count = _native_libs.count();
   for (int i = 0; i < native_lib_count; i++) {
-    const char *s = _native_libs[i]->name();
-    if (s != NULL) {
-      const char *p = strrchr(s, '/');
-      if (p != NULL && strncmp(p + 1, lib_name, lib_name_len) == 0) {
-        return _native_libs[i];
+    CodeCache *lib = _native_libs[i];
+    if (lib != NULL) {
+      const char *s = lib->name();
+      if (s != NULL) {
+        const char *p = strrchr(s, '/');
+        if (p != NULL && strncmp(p + 1, lib_name, lib_name_len) == 0) {
+          return lib;
+        }
       }
     }
   }
@@ -93,8 +102,9 @@ CodeCache *Libraries::findLibraryByName(const char *lib_name) {
 CodeCache *Libraries::findLibraryByAddress(const void *address) {
   const int native_lib_count = _native_libs.count();
   for (int i = 0; i < native_lib_count; i++) {
-    if (_native_libs[i]->contains(address)) {
-      return _native_libs[i];
+    CodeCache *lib = _native_libs[i];
+    if (lib != NULL && lib->contains(address)) {
+      return lib;
     }
   }
   return NULL;

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

@@ -24,7 +24,7 @@ class Libraries {
   // Note: Parameter is uint32_t to match lib_index packing (17 bits = max 131K libraries)
   CodeCache *getLibraryByIndex(uint32_t index) const {
     if (index < _native_libs.count()) {
-      return _native_libs[index];
+      return _native_libs[index];  // may be NULL during concurrent add()
     }
     return nullptr;
   }