From e7ff78cc9cd95db54f9cec474819246c77040c83 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Mon, 18 May 2026 21:29:24 +0200 Subject: [PATCH 1/2] fix(objectSampler): drop Deallocate on GetClassSignature error path The error branch in ObjectSampler::recordAllocation called jvmti->Deallocate(class_name) regardless of whether GetClassSignature actually returned success. The JVMTI spec does not guarantee output buffers are populated on a non-JVMTI_ERROR_NONE return; the pointer value is unspecified, so passing it to Deallocate is unsafe and was observed to crash with SIGSEGV (PROF-14626 / dd-crashsig-1b6db147ca4415c7). Add a TEST_F-based regression suite that exercises recordAllocation with a mock jvmtiEnv covering: error+non-NULL sentinel (the UAF), error+NULL, success+NULL name, success+valid name, and the !_active short-circuit. Co-Authored-By: Claude Opus 4.7 (1M context) --- ddprof-lib/src/main/cpp/objectSampler.cpp | 7 +- ddprof-lib/src/main/cpp/objectSampler.h | 1 + ddprof-lib/src/test/cpp/objectSampler_ut.cpp | 192 +++++++++++++++++++ 3 files changed, 197 insertions(+), 3 deletions(-) diff --git a/ddprof-lib/src/main/cpp/objectSampler.cpp b/ddprof-lib/src/main/cpp/objectSampler.cpp index f669dab05..ebcb3371d 100644 --- a/ddprof-lib/src/main/cpp/objectSampler.cpp +++ b/ddprof-lib/src/main/cpp/objectSampler.cpp @@ -77,9 +77,10 @@ void ObjectSampler::recordAllocation(jvmtiEnv *jvmti, JNIEnv *jni, class_name == NULL) { // Drop the sample: recording it under the default class id 0 // would corrupt allocation attribution. - if (class_name != NULL) { - jvmti->Deallocate((unsigned char *)class_name); - } + // NOTE: Do NOT call Deallocate here. The JVMTI spec does not guarantee + // output buffers are populated on a non-JVMTI_ERROR_NONE return; the + // pointer value is unspecified, so passing it to Deallocate is unsafe + // in practice and observed to crash with SIGSEGV. return; } const char *name_slice = NULL; diff --git a/ddprof-lib/src/main/cpp/objectSampler.h b/ddprof-lib/src/main/cpp/objectSampler.h index 54e860ed8..88718087c 100644 --- a/ddprof-lib/src/main/cpp/objectSampler.h +++ b/ddprof-lib/src/main/cpp/objectSampler.h @@ -30,6 +30,7 @@ typedef int (*get_sampling_interval)(); class ObjectSampler : public Engine { friend Recording; + friend class ObjectSamplerTestAccessor; private: static ObjectSampler *const _instance; diff --git a/ddprof-lib/src/test/cpp/objectSampler_ut.cpp b/ddprof-lib/src/test/cpp/objectSampler_ut.cpp index e4d2c3e33..17d4a9f3d 100644 --- a/ddprof-lib/src/test/cpp/objectSampler_ut.cpp +++ b/ddprof-lib/src/test/cpp/objectSampler_ut.cpp @@ -6,6 +6,126 @@ #include #include #include "../../main/cpp/objectSampler.h" +#include "vmEntry.h" + +// --------------------------------------------------------------------------- +// ObjectSamplerTestAccessor — friend of ObjectSampler, exposes internals +// needed by the regression tests. +// --------------------------------------------------------------------------- +class ObjectSamplerTestAccessor { +public: + static void setActive(ObjectSampler *s, bool v) { + __atomic_store_n(&s->_active, v, __ATOMIC_RELEASE); + } + + static void callRecordAllocation(ObjectSampler *s, jvmtiEnv *jvmti, + JNIEnv *jni, jthread thread, + int event_type, jobject object, + jclass klass, jlong size) { + s->recordAllocation(jvmti, jni, thread, event_type, object, klass, size); + } +}; + +// --------------------------------------------------------------------------- +// Mock-JVMTI infrastructure for Deallocate regression tests +// --------------------------------------------------------------------------- + +// Read-only buffer the success mocks hand back as a class signature; never +// modified, so file-scope storage is safe. +static char g_mock_class_name[] = "Ljava/lang/String;"; + +// Test fixture owning the per-test JVMTI function table and Deallocate +// counter. Each TEST_F gets a fresh instance, which (a) prevents shared +// static state from leaking between tests, (b) lets the fixture restore +// the process-global ObjectSampler _active flag in TearDown, and (c) +// removes the use-after-return hazard of returning a struct whose +// _jvmtiEnv::functions points into a per-call static. +class ObjectSamplerDeallocateTest : public ::testing::Test { +protected: + jvmtiInterface_1_ tbl{}; + _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; + + void SetUp() override { + deallocate_calls = 0; + tbl = jvmtiInterface_1_{}; + tbl.Deallocate = &mock_Deallocate; + mock_env.functions = &tbl; + active_fixture = this; + } + + void TearDown() override { + // Restore the process-global singleton flag so subsequent tests start + // from a known state. + ObjectSamplerTestAccessor::setActive(ObjectSampler::instance(), false); + active_fixture = nullptr; + } + + void setMockGetClassSignature( + jvmtiError(JNICALL *fn)(jvmtiEnv *, jclass, char **, char **)) { + tbl.GetClassSignature = fn; + } + + // Mock Deallocate increments the per-fixture counter; it never frees the + // pointer because the mock signature buffer is statically allocated. + static jvmtiError JNICALL mock_Deallocate(jvmtiEnv * /*env*/, + unsigned char * /*mem*/) { + if (active_fixture) { + ++active_fixture->deallocate_calls; + } + return JVMTI_ERROR_NONE; + } +}; + +thread_local 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*/) { + if (signature_ptr) { + *signature_ptr = g_mock_class_name; + } + 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). +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 + } + 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*/) { + if (signature_ptr) { + *signature_ptr = NULL; + } + return JVMTI_ERROR_NONE; +} // Regression tests for ObjectSampler::normalizeClassSignature, the // guard that recordAllocation uses against null, empty, or malformed @@ -87,3 +207,75 @@ TEST(ObjectSamplerTest, NormalizePassesThroughObjectArray) { EXPECT_EQ(out_name, signature); 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); +} + +// --------------------------------------------------------------------------- +// T-02: GetClassSignature succeeds with a valid class name. +// Deallocate IS called exactly once (on the success path). +// Note: after Deallocate, lookupClass returns -1 (empty class map) and 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); +} + +// --------------------------------------------------------------------------- +// 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); +} + +// --------------------------------------------------------------------------- +// 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); +} + +// --------------------------------------------------------------------------- +// 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); +} From b1ef1f981ba77c751967b9b697a3a3001787efb9 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Tue, 19 May 2026 20:13:54 +0200 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- ddprof-lib/src/test/cpp/objectSampler_ut.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddprof-lib/src/test/cpp/objectSampler_ut.cpp b/ddprof-lib/src/test/cpp/objectSampler_ut.cpp index 17d4a9f3d..e146bdae8 100644 --- a/ddprof-lib/src/test/cpp/objectSampler_ut.cpp +++ b/ddprof-lib/src/test/cpp/objectSampler_ut.cpp @@ -225,7 +225,7 @@ TEST_F(ObjectSamplerDeallocateTest, DeallocateNotCalledOnErrorWithNonNullSentine // --------------------------------------------------------------------------- // T-02: GetClassSignature succeeds with a valid class name. // Deallocate IS called exactly once (on the success path). -// Note: after Deallocate, lookupClass returns -1 (empty class map) and the +// 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) {