fix(profiler): guard JVMTI sample path against null calltrace buffer (#536)

jbachorik · web-flow · commit 980bc8d87dd8 · 2026-05-22T17:01:10.000+02:00
diff --git a/ddprof-lib/build.gradle.kts b/ddprof-lib/build.gradle.kts
@@ -50,6 +50,8 @@ gtest {
     "$javaHome/include/$platformInclude",
     project(":malloc-shim").file("src/main/public"),
   )
+
+  failFast.set(true)
 }
 
 // Java configuration - using sourceCompatibility (not --release 8)
diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp
@@ -1926,6 +1926,7 @@ void FlightRecorder::recordEventDelegated(int lock_index, int tid,
           // Delegation is only wired for CPU/wall samples in v1.
           break;
       }
+      rec->flushIfNeeded(buf);
       rec->addThread(lock_index, tid);
     }
   }
diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp
@@ -458,11 +458,20 @@ u64 Profiler::recordJVMTISample(u64 counter, int tid, jthread thread, jint event
   }
   u64 call_trace_id = 0;
   if (!_omit_stacktraces) {
+    // Defensive: the buffer slot can be observed as nullptr in pathological
+    // start sequences (e.g. a calloc failure path in Profiler::start before
+    // engines are enabled). Drop the sample rather than dereferencing.
+    CallTraceBuffer *buf = _calltrace_buffer[lock_index];
+    if (buf == nullptr) {
+      atomicIncRelaxed(_failures[-ticks_skipped]);
+      _locks[lock_index].unlock();
+      return 0;
+    }
 #ifdef COUNTERS
     u64 startTime = TSC::ticks();
 #endif // COUNTERS
-    ASGCT_CallFrame *frames = _calltrace_buffer[lock_index]->_asgct_frames;
-    jvmtiFrameInfo *jvmti_frames = _calltrace_buffer[lock_index]->_jvmti_frames;
+    ASGCT_CallFrame *frames = buf->_asgct_frames;
+    jvmtiFrameInfo *jvmti_frames = buf->_jvmti_frames;
 
     int num_frames = 0;
 
@@ -676,24 +685,28 @@ void Profiler::recordExternalSample(u64 weight, int tid, int num_frames,
   CriticalSection cs;
   atomicIncRelaxed(_total_samples);
 
-  // Convert ASGCT_CallFrame to ASGCT_CallFrame
-  // External samplers (like ObjectSampler) provide standard frames only
   u32 lock_index = getLockIndex(tid);
-  ASGCT_CallFrame *extended_frames = _calltrace_buffer[lock_index]->_asgct_frames;
-  for (int i = 0; i < num_frames; i++) {
-    extended_frames[i] = frames[i];
-  }
-
-  u64 call_trace_id =
-      _call_trace_storage.put(num_frames, extended_frames, truncated, weight);
   if (!_locks[lock_index].tryLock() &&
       !_locks[lock_index = (lock_index + 1) % CONCURRENCY_LEVEL].tryLock() &&
       !_locks[lock_index = (lock_index + 2) % CONCURRENCY_LEVEL].tryLock()) {
-    // Too many concurrent signals already
     atomicIncRelaxed(_failures[-ticks_skipped]);
     return;
   }
 
+  CallTraceBuffer *buf = _calltrace_buffer[lock_index];
+  if (buf == nullptr) {
+    atomicIncRelaxed(_failures[-ticks_skipped]);
+    _locks[lock_index].unlock();
+    return;
+  }
+  // External samplers (like ObjectSampler) provide standard frames only
+  ASGCT_CallFrame *extended_frames = buf->_asgct_frames;
+  for (int i = 0; i < num_frames; i++) {
+    extended_frames[i] = frames[i];
+  }
+
+  u64 call_trace_id =
+      _call_trace_storage.put(num_frames, extended_frames, truncated, weight);
   _jfr.recordEvent(lock_index, tid, call_trace_id, event_type, event);
 
   _locks[lock_index].unlock();
@@ -1166,13 +1179,23 @@ Error Profiler::start(Arguments &args, bool reset) {
     size_t nelem = _max_stack_depth + RESERVED_FRAMES;
 
     for (int i = 0; i < CONCURRENCY_LEVEL; i++) {
-      free(_calltrace_buffer[i]);
-      _calltrace_buffer[i] = (CallTraceBuffer*)calloc(nelem, sizeof(CallTraceBuffer));
-      if (_calltrace_buffer[i] == NULL) {
+      // Allocate the replacement before touching the slot so a calloc failure
+      // does not leave the slot pointing at freed memory.
+      CallTraceBuffer *fresh =
+          (CallTraceBuffer*)calloc(nelem, sizeof(CallTraceBuffer));
+      if (fresh == NULL) {
         _max_stack_depth = 0;
         return Error("Not enough memory to allocate stack trace buffers (try "
                      "smaller jstackdepth)");
       }
+      // Swap under the per-shard lock: all readers (recordJVMTISample,
+      // recordExternalSample) acquire this lock via tryLock before reading
+      // _calltrace_buffer, so no reader can observe a freed pointer mid-replacement.
+      _locks[i].lock();
+      CallTraceBuffer *prev = _calltrace_buffer[i];
+      _calltrace_buffer[i] = fresh;
+      _locks[i].unlock();
+      free(prev);
     }
   }
 
diff --git a/ddprof-lib/src/main/cpp/profiler.h b/ddprof-lib/src/main/cpp/profiler.h
@@ -97,8 +97,11 @@ class alignas(alignof(SpinLock)) Profiler {
   void *_timer_id;
 
   volatile u64 _total_samples;
+  // On a separate cache line: incremented from every signal handler via
+  // recordSampleDelegated; must not share a line with _failures (written by
+  // ASGCT paths) or _total_samples (written by every recording path).
   alignas(DEFAULT_CACHE_LINE_SIZE) volatile u64 _sample_seq;
-  u64 _failures[ASGCT_FAILURE_TYPES];
+  alignas(DEFAULT_CACHE_LINE_SIZE) u64 _failures[ASGCT_FAILURE_TYPES];
 
   SpinLock _class_map_lock;
   SpinLock _locks[CONCURRENCY_LEVEL];
diff --git a/ddprof-lib/src/main/cpp/vmEntry.h b/ddprof-lib/src/main/cpp/vmEntry.h
@@ -129,7 +129,10 @@ class VM {
   static bool _is_adaptive_gc_boundary_flag_set;
 
   // HotSpot JFR async stack-trace extension (optional, JDK 27+).
-  // Null until InitializeRequestStackTrace succeeds; published with RELEASE.
+  // _request_stack_trace is atomic (RELEASE/ACQUIRE) because canRequestStackTrace()
+  // is called from signal handlers; _init_request_stack_trace is plain because it
+  // is only ever read by initializeRequestStackTrace(), called once from the same
+  // init thread before any signal handlers are installed.
   static jvmtiExtensionFunction _request_stack_trace;
   static jvmtiExtensionFunction _init_request_stack_trace;
 
diff --git a/ddprof-lib/src/main/cpp/wallClock.cpp b/ddprof-lib/src/main/cpp/wallClock.cpp
@@ -27,7 +27,7 @@
 
 std::atomic<bool> BaseWallClock::_enabled{false};
 
-bool WallClockASGCT::inSyscall(void *ucontext) {
+bool BaseWallClock::inSyscall(void *ucontext) {
   StackFrame frame(ucontext);
   uintptr_t pc = frame.pc();
 
@@ -226,27 +226,6 @@ void WallClockASGCT::timerLoop() {
 // instead of invoking ASGCT. Used only when VM::canRequestStackTrace() is true
 // and the profiler has opted into jvmtistacks.
 
-bool WallClockJvmti::inSyscall(void *ucontext) {
-  StackFrame frame(ucontext);
-  uintptr_t pc = frame.pc();
-
-  if (StackFrame::isSyscall((instruction_t *)pc)) {
-    return true;
-  }
-
-  uintptr_t prev_pc = pc - SYSCALL_SIZE;
-  if ((pc & 0xfff) >= SYSCALL_SIZE ||
-      Libraries::instance()->findLibraryByAddress((instruction_t *)prev_pc) !=
-          NULL) {
-    if (StackFrame::isSyscall((instruction_t *)prev_pc) &&
-        frame.checkInterruptedSyscall()) {
-      return true;
-    }
-  }
-
-  return false;
-}
-
 void WallClockJvmti::sharedSignalHandler(int signo, siginfo_t *siginfo,
                                          void *ucontext) {
   WallClockJvmti *engine =
@@ -327,6 +306,8 @@ void WallClockJvmti::timerLoop() {
           threads_already_exited++;
         } else if (errno == EPERM) {
           permission_denied++;
+        } else if (errno == EAGAIN) {
+          // Signal queue limit (RLIMIT_SIGPENDING) reached — count as missed.
         } else {
           Log::debug("unexpected error %s", strerror(errno));
         }
diff --git a/ddprof-lib/src/main/cpp/wallClock.h b/ddprof-lib/src/main/cpp/wallClock.h
@@ -40,6 +40,7 @@ class BaseWallClock : public Engine {
     }
 
     bool isEnabled() const;
+    static bool inSyscall(void* ucontext);
 
     template <typename ThreadType, typename CollectThreadsFunc, typename SampleThreadsFunc, typename CleanThreadFunc>
     void timerLoopCommon(CollectThreadsFunc collectThreads, SampleThreadsFunc sampleThreads, CleanThreadFunc cleanThreads, int reservoirSize, u64 interval) {
@@ -142,8 +143,6 @@ class WallClockASGCT : public BaseWallClock {
   private:
     bool _collapsing;
 
-    static bool inSyscall(void* ucontext);
-
     static void sharedSignalHandler(int signo, siginfo_t* siginfo, void* ucontext);
     void signalHandler(int signo, siginfo_t* siginfo, void* ucontext, u64 last_sample);
 
@@ -163,8 +162,6 @@ class WallClockASGCT : public BaseWallClock {
 // VM::canRequestStackTrace().
 class WallClockJvmti : public BaseWallClock {
   private:
-    static bool inSyscall(void* ucontext);
-
     static void sharedSignalHandler(int signo, siginfo_t* siginfo, void* ucontext);
     void signalHandler(int signo, siginfo_t* siginfo, void* ucontext, u64 last_sample);
 
diff --git a/ddprof-lib/src/test/cpp/objectSampler_ut.cpp b/ddprof-lib/src/test/cpp/objectSampler_ut.cpp
@@ -6,8 +6,17 @@
 #include <cstddef>
 #include <cstring>
 #include "../../main/cpp/objectSampler.h"
+#include "../../main/cpp/gtest_crash_handler.h"
 #include "vmEntry.h"
 
+static constexpr char OBJECT_SAMPLER_TEST_NAME[] = "ObjectSamplerTest";
+class ObjectSamplerGlobalSetup {
+public:
+  ObjectSamplerGlobalSetup()  { installGtestCrashHandler<OBJECT_SAMPLER_TEST_NAME>(); }
+  ~ObjectSamplerGlobalSetup() { restoreDefaultSignalHandlers(); }
+};
+static ObjectSamplerGlobalSetup object_sampler_global_setup;
+
 // ---------------------------------------------------------------------------
 // ObjectSamplerTestAccessor — friend of ObjectSampler, exposes internals
 // needed by the regression tests.
@@ -46,10 +55,7 @@ class ObjectSamplerDeallocateTest : public ::testing::Test {
   _jvmtiEnv mock_env{};
   int deallocate_calls = 0;
 
-  // Active fixture pointer used by the C-style mock callbacks to reach
-  // per-instance state. Reset in SetUp/TearDown so the mocks never see a
-  // stale fixture even if a previous test crashed mid-run.
-  static thread_local ObjectSamplerDeallocateTest *active_fixture;
+  static ObjectSamplerDeallocateTest *active_fixture;
 
   void SetUp() override {
     deallocate_calls = 0;
@@ -71,22 +77,27 @@ class ObjectSamplerDeallocateTest : public ::testing::Test {
     tbl.GetClassSignature = fn;
   }
 
-  // Mock Deallocate increments the per-fixture counter; it never frees the
-  // pointer because the mock signature buffer is statically allocated.
+  void runAndExpect(
+      jvmtiError(JNICALL *mock)(jvmtiEnv *, jclass, char **, char **),
+      bool active, int expected_calls) {
+    setMockGetClassSignature(mock);
+    ObjectSampler *s = ObjectSampler::instance();
+    ObjectSamplerTestAccessor::setActive(s, active);
+    ObjectSamplerTestAccessor::callRecordAllocation(
+        s, &mock_env, nullptr, nullptr, BCI_ALLOC, nullptr, nullptr, 1024);
+    EXPECT_EQ(deallocate_calls, expected_calls);
+  }
+
   static jvmtiError JNICALL mock_Deallocate(jvmtiEnv * /*env*/,
                                             unsigned char * /*mem*/) {
-    if (active_fixture) {
-      ++active_fixture->deallocate_calls;
-    }
+    ++active_fixture->deallocate_calls;
     return JVMTI_ERROR_NONE;
   }
 };
 
-thread_local ObjectSamplerDeallocateTest *
+ObjectSamplerDeallocateTest *
     ObjectSamplerDeallocateTest::active_fixture = nullptr;
 
-// GetClassSignature mock: returns JVMTI_ERROR_NONE and writes
-// g_mock_class_name into *signature_ptr.
 static jvmtiError JNICALL mock_GetClassSignature_success(
     jvmtiEnv * /*env*/, jclass /*klass*/,
     char **signature_ptr, char ** /*generic_ptr*/) {
@@ -96,28 +107,24 @@ static jvmtiError JNICALL mock_GetClassSignature_success(
   return JVMTI_ERROR_NONE;
 }
 
-// GetClassSignature mock: returns an error AND writes a non-NULL sentinel
-// into *signature_ptr (the UAF scenario we are guarding against).
+// Returns error but writes a non-NULL sentinel into *signature_ptr — the UAF
+// scenario the fix guards against (Deallocate must not be called on error).
 static jvmtiError JNICALL mock_GetClassSignature_error_with_sentinel(
     jvmtiEnv * /*env*/, jclass /*klass*/,
     char **signature_ptr, char ** /*generic_ptr*/) {
   if (signature_ptr) {
-    *signature_ptr = g_mock_class_name; // sentinel: non-NULL despite error
+    *signature_ptr = g_mock_class_name;
   }
   return JVMTI_ERROR_INVALID_CLASS;
 }
 
-// GetClassSignature mock: returns an error and leaves *signature_ptr at NULL.
 static jvmtiError JNICALL mock_GetClassSignature_error_null(
     jvmtiEnv * /*env*/, jclass /*klass*/,
     char **signature_ptr, char ** /*generic_ptr*/) {
-  // Leave *signature_ptr unchanged (NULL as initialised by recordAllocation).
   (void)signature_ptr;
   return JVMTI_ERROR_INVALID_CLASS;
 }
 
-// GetClassSignature mock: returns JVMTI_ERROR_NONE but writes NULL into
-// *signature_ptr — a misbehaving JVMTI impl.
 static jvmtiError JNICALL mock_GetClassSignature_success_null_name(
     jvmtiEnv * /*env*/, jclass /*klass*/,
     char **signature_ptr, char ** /*generic_ptr*/) {
@@ -208,74 +215,34 @@ TEST(ObjectSamplerTest, NormalizePassesThroughObjectArray) {
     EXPECT_EQ(out_len, strlen("[Ljava/lang/String;"));
 }
 
-// ---------------------------------------------------------------------------
 // T-01: GetClassSignature returns error with non-NULL sentinel in *signature_ptr.
 // Deallocate MUST NOT be called.
-// ---------------------------------------------------------------------------
 TEST_F(ObjectSamplerDeallocateTest, DeallocateNotCalledOnErrorWithNonNullSentinel) {
-    setMockGetClassSignature(mock_GetClassSignature_error_with_sentinel);
-    ObjectSampler *s = ObjectSampler::instance();
-    ObjectSamplerTestAccessor::setActive(s, true);
-    ObjectSamplerTestAccessor::callRecordAllocation(
-        s, &mock_env, nullptr, nullptr, BCI_ALLOC,
-        nullptr, nullptr, 1024);
-    EXPECT_EQ(deallocate_calls, 0);
+    runAndExpect(mock_GetClassSignature_error_with_sentinel, true, 0);
 }
 
-// ---------------------------------------------------------------------------
 // T-02: GetClassSignature succeeds with a valid class name.
 // Deallocate IS called exactly once (on the success path).
 // Note: lookupClass returns -1 because the class map is empty, so the
 // method returns without recording — that is the expected behaviour.
-// ---------------------------------------------------------------------------
 TEST_F(ObjectSamplerDeallocateTest, DeallocateCalledOnceOnGetClassSignatureSuccess) {
-    setMockGetClassSignature(mock_GetClassSignature_success);
-    ObjectSampler *s = ObjectSampler::instance();
-    ObjectSamplerTestAccessor::setActive(s, true);
-    ObjectSamplerTestAccessor::callRecordAllocation(
-        s, &mock_env, nullptr, nullptr, BCI_ALLOC,
-        nullptr, nullptr, 1024);
-    EXPECT_EQ(deallocate_calls, 1);
+    runAndExpect(mock_GetClassSignature_success, true, 1);
 }
 
-// ---------------------------------------------------------------------------
 // T-03: GetClassSignature fails and leaves class_name at NULL.
 // Deallocate MUST NOT be called.
-// ---------------------------------------------------------------------------
 TEST_F(ObjectSamplerDeallocateTest, DeallocateNotCalledOnErrorWithNullName) {
-    setMockGetClassSignature(mock_GetClassSignature_error_null);
-    ObjectSampler *s = ObjectSampler::instance();
-    ObjectSamplerTestAccessor::setActive(s, true);
-    ObjectSamplerTestAccessor::callRecordAllocation(
-        s, &mock_env, nullptr, nullptr, BCI_ALLOC,
-        nullptr, nullptr, 1024);
-    EXPECT_EQ(deallocate_calls, 0);
+    runAndExpect(mock_GetClassSignature_error_null, true, 0);
 }
 
-// ---------------------------------------------------------------------------
 // T-04: GetClassSignature succeeds but writes NULL into *signature_ptr.
 // Deallocate MUST NOT be called (the NULL guard in the condition fires).
-// ---------------------------------------------------------------------------
 TEST_F(ObjectSamplerDeallocateTest, DeallocateNotCalledWhenSuccessButNullName) {
-    setMockGetClassSignature(mock_GetClassSignature_success_null_name);
-    ObjectSampler *s = ObjectSampler::instance();
-    ObjectSamplerTestAccessor::setActive(s, true);
-    ObjectSamplerTestAccessor::callRecordAllocation(
-        s, &mock_env, nullptr, nullptr, BCI_ALLOC,
-        nullptr, nullptr, 1024);
-    EXPECT_EQ(deallocate_calls, 0);
+    runAndExpect(mock_GetClassSignature_success_null_name, true, 0);
 }
 
-// ---------------------------------------------------------------------------
 // T-05: _active is false — recordAllocation returns immediately.
 // Deallocate MUST NOT be called.
-// ---------------------------------------------------------------------------
 TEST_F(ObjectSamplerDeallocateTest, DeallocateNotCalledWhenNotActive) {
-    setMockGetClassSignature(mock_GetClassSignature_success);
-    ObjectSampler *s = ObjectSampler::instance();
-    ObjectSamplerTestAccessor::setActive(s, false);
-    ObjectSamplerTestAccessor::callRecordAllocation(
-        s, &mock_env, nullptr, nullptr, BCI_ALLOC,
-        nullptr, nullptr, 1024);
-    EXPECT_EQ(deallocate_calls, 0);
+    runAndExpect(mock_GetClassSignature_success, false, 0);
 }
diff --git a/ddprof-lib/src/test/cpp/profiler_null_calltrace_buffer_ut.cpp b/ddprof-lib/src/test/cpp/profiler_null_calltrace_buffer_ut.cpp
diff --git a/ddprof-lib/src/test/cpp/thread_teardown_safety_ut.cpp b/ddprof-lib/src/test/cpp/thread_teardown_safety_ut.cpp

Original file line number	Diff line number	Diff line change
`@@ -50,6 +50,8 @@ gtest {`
`50`	`50`	`"$javaHome/include/$platformInclude",`
`51`	`51`	`project(":malloc-shim").file("src/main/public"),`
`52`	`52`	`)`
	`53`	`+`
	`54`	`+ failFast.set(true)`
`53`	`55`	`}`
`54`	`56`
`55`	`57`	`// Java configuration - using sourceCompatibility (not --release 8)`
Original file line number	Diff line number	Diff line change
`@@ -1926,6 +1926,7 @@ void FlightRecorder::recordEventDelegated(int lock_index, int tid,`
`1926`	`1926`	`// Delegation is only wired for CPU/wall samples in v1.`
`1927`	`1927`	`break;`
`1928`	`1928`	`}`
	`1929`	`+ rec->flushIfNeeded(buf);`
`1929`	`1930`	`rec->addThread(lock_index, tid);`
`1930`	`1931`	`}`
`1931`	`1932`	`}`