Skip to content

Commit c5b8a1e

Browse files
jbachorikjbachorik
authored
fix(objectSampler): drop Deallocate on GetClassSignature error path (#535)
1 parent 93ab936 commit c5b8a1e

3 files changed

Lines changed: 197 additions & 3 deletions

File tree

‎ddprof-lib/src/main/cpp/objectSampler.cpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,10 @@ void ObjectSampler::recordAllocation(jvmtiEnv *jvmti, JNIEnv *jni,
7777
class_name == NULL) {
7878
// Drop the sample: recording it under the default class id 0
7979
// would corrupt allocation attribution.
80-
if (class_name != NULL) {
81-
jvmti->Deallocate((unsigned char *)class_name);
82-
}
80+
// NOTE: Do NOT call Deallocate here. The JVMTI spec does not guarantee
81+
// output buffers are populated on a non-JVMTI_ERROR_NONE return; the
82+
// pointer value is unspecified, so passing it to Deallocate is unsafe
83+
// in practice and observed to crash with SIGSEGV.
8384
return;
8485
}
8586
const char *name_slice = NULL;

‎ddprof-lib/src/main/cpp/objectSampler.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ typedef int (*get_sampling_interval)();
3030

3131
class ObjectSampler : public Engine {
3232
friend Recording;
33+
friend class ObjectSamplerTestAccessor;
3334

3435
private:
3536
static ObjectSampler *const _instance;

‎ddprof-lib/src/test/cpp/objectSampler_ut.cpp‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,126 @@
66
#include <cstddef>
77
#include <cstring>
88
#include "../../main/cpp/objectSampler.h"
9+
#include "vmEntry.h"
10+
11+
// ---------------------------------------------------------------------------
12+
// ObjectSamplerTestAccessor — friend of ObjectSampler, exposes internals
13+
// needed by the regression tests.
14+
// ---------------------------------------------------------------------------
15+
class ObjectSamplerTestAccessor {
16+
public:
17+
static void setActive(ObjectSampler *s, bool v) {
18+
__atomic_store_n(&s->_active, v, __ATOMIC_RELEASE);
19+
}
20+
21+
static void callRecordAllocation(ObjectSampler *s, jvmtiEnv *jvmti,
22+
JNIEnv *jni, jthread thread,
23+
int event_type, jobject object,
24+
jclass klass, jlong size) {
25+
s->recordAllocation(jvmti, jni, thread, event_type, object, klass, size);
26+
}
27+
};
28+
29+
// ---------------------------------------------------------------------------
30+
// Mock-JVMTI infrastructure for Deallocate regression tests
31+
// ---------------------------------------------------------------------------
32+
33+
// Read-only buffer the success mocks hand back as a class signature; never
34+
// modified, so file-scope storage is safe.
35+
static char g_mock_class_name[] = "Ljava/lang/String;";
36+
37+
// Test fixture owning the per-test JVMTI function table and Deallocate
38+
// counter. Each TEST_F gets a fresh instance, which (a) prevents shared
39+
// static state from leaking between tests, (b) lets the fixture restore
40+
// the process-global ObjectSampler _active flag in TearDown, and (c)
41+
// removes the use-after-return hazard of returning a struct whose
42+
// _jvmtiEnv::functions points into a per-call static.
43+
class ObjectSamplerDeallocateTest : public ::testing::Test {
44+
protected:
45+
jvmtiInterface_1_ tbl{};
46+
_jvmtiEnv mock_env{};
47+
int deallocate_calls = 0;
48+
49+
// Active fixture pointer used by the C-style mock callbacks to reach
50+
// per-instance state. Reset in SetUp/TearDown so the mocks never see a
51+
// stale fixture even if a previous test crashed mid-run.
52+
static thread_local ObjectSamplerDeallocateTest *active_fixture;
53+
54+
void SetUp() override {
55+
deallocate_calls = 0;
56+
tbl = jvmtiInterface_1_{};
57+
tbl.Deallocate = &mock_Deallocate;
58+
mock_env.functions = &tbl;
59+
active_fixture = this;
60+
}
61+
62+
void TearDown() override {
63+
// Restore the process-global singleton flag so subsequent tests start
64+
// from a known state.
65+
ObjectSamplerTestAccessor::setActive(ObjectSampler::instance(), false);
66+
active_fixture = nullptr;
67+
}
68+
69+
void setMockGetClassSignature(
70+
jvmtiError(JNICALL *fn)(jvmtiEnv *, jclass, char **, char **)) {
71+
tbl.GetClassSignature = fn;
72+
}
73+
74+
// Mock Deallocate increments the per-fixture counter; it never frees the
75+
// pointer because the mock signature buffer is statically allocated.
76+
static jvmtiError JNICALL mock_Deallocate(jvmtiEnv * /*env*/,
77+
unsigned char * /*mem*/) {
78+
if (active_fixture) {
79+
++active_fixture->deallocate_calls;
80+
}
81+
return JVMTI_ERROR_NONE;
82+
}
83+
};
84+
85+
thread_local ObjectSamplerDeallocateTest *
86+
ObjectSamplerDeallocateTest::active_fixture = nullptr;
87+
88+
// GetClassSignature mock: returns JVMTI_ERROR_NONE and writes
89+
// g_mock_class_name into *signature_ptr.
90+
static jvmtiError JNICALL mock_GetClassSignature_success(
91+
jvmtiEnv * /*env*/, jclass /*klass*/,
92+
char **signature_ptr, char ** /*generic_ptr*/) {
93+
if (signature_ptr) {
94+
*signature_ptr = g_mock_class_name;
95+
}
96+
return JVMTI_ERROR_NONE;
97+
}
98+
99+
// GetClassSignature mock: returns an error AND writes a non-NULL sentinel
100+
// into *signature_ptr (the UAF scenario we are guarding against).
101+
static jvmtiError JNICALL mock_GetClassSignature_error_with_sentinel(
102+
jvmtiEnv * /*env*/, jclass /*klass*/,
103+
char **signature_ptr, char ** /*generic_ptr*/) {
104+
if (signature_ptr) {
105+
*signature_ptr = g_mock_class_name; // sentinel: non-NULL despite error
106+
}
107+
return JVMTI_ERROR_INVALID_CLASS;
108+
}
109+
110+
// GetClassSignature mock: returns an error and leaves *signature_ptr at NULL.
111+
static jvmtiError JNICALL mock_GetClassSignature_error_null(
112+
jvmtiEnv * /*env*/, jclass /*klass*/,
113+
char **signature_ptr, char ** /*generic_ptr*/) {
114+
// Leave *signature_ptr unchanged (NULL as initialised by recordAllocation).
115+
(void)signature_ptr;
116+
return JVMTI_ERROR_INVALID_CLASS;
117+
}
118+
119+
// GetClassSignature mock: returns JVMTI_ERROR_NONE but writes NULL into
120+
// *signature_ptr — a misbehaving JVMTI impl.
121+
static jvmtiError JNICALL mock_GetClassSignature_success_null_name(
122+
jvmtiEnv * /*env*/, jclass /*klass*/,
123+
char **signature_ptr, char ** /*generic_ptr*/) {
124+
if (signature_ptr) {
125+
*signature_ptr = NULL;
126+
}
127+
return JVMTI_ERROR_NONE;
128+
}
9129

10130
// Regression tests for ObjectSampler::normalizeClassSignature, the
11131
// guard that recordAllocation uses against null, empty, or malformed
@@ -87,3 +207,75 @@ TEST(ObjectSamplerTest, NormalizePassesThroughObjectArray) {
87207
EXPECT_EQ(out_name, signature);
88208
EXPECT_EQ(out_len, strlen("[Ljava/lang/String;"));
89209
}
210+
211+
// ---------------------------------------------------------------------------
212+
// T-01: GetClassSignature returns error with non-NULL sentinel in *signature_ptr.
213+
// Deallocate MUST NOT be called.
214+
// ---------------------------------------------------------------------------
215+
TEST_F(ObjectSamplerDeallocateTest, DeallocateNotCalledOnErrorWithNonNullSentinel) {
216+
setMockGetClassSignature(mock_GetClassSignature_error_with_sentinel);
217+
ObjectSampler *s = ObjectSampler::instance();
218+
ObjectSamplerTestAccessor::setActive(s, true);
219+
ObjectSamplerTestAccessor::callRecordAllocation(
220+
s, &mock_env, nullptr, nullptr, BCI_ALLOC,
221+
nullptr, nullptr, 1024);
222+
EXPECT_EQ(deallocate_calls, 0);
223+
}
224+
225+
// ---------------------------------------------------------------------------
226+
// T-02: GetClassSignature succeeds with a valid class name.
227+
// Deallocate IS called exactly once (on the success path).
228+
// Note: lookupClass returns -1 because the class map is empty, so the
229+
// method returns without recording — that is the expected behaviour.
230+
// ---------------------------------------------------------------------------
231+
TEST_F(ObjectSamplerDeallocateTest, DeallocateCalledOnceOnGetClassSignatureSuccess) {
232+
setMockGetClassSignature(mock_GetClassSignature_success);
233+
ObjectSampler *s = ObjectSampler::instance();
234+
ObjectSamplerTestAccessor::setActive(s, true);
235+
ObjectSamplerTestAccessor::callRecordAllocation(
236+
s, &mock_env, nullptr, nullptr, BCI_ALLOC,
237+
nullptr, nullptr, 1024);
238+
EXPECT_EQ(deallocate_calls, 1);
239+
}
240+
241+
// ---------------------------------------------------------------------------
242+
// T-03: GetClassSignature fails and leaves class_name at NULL.
243+
// Deallocate MUST NOT be called.
244+
// ---------------------------------------------------------------------------
245+
TEST_F(ObjectSamplerDeallocateTest, DeallocateNotCalledOnErrorWithNullName) {
246+
setMockGetClassSignature(mock_GetClassSignature_error_null);
247+
ObjectSampler *s = ObjectSampler::instance();
248+
ObjectSamplerTestAccessor::setActive(s, true);
249+
ObjectSamplerTestAccessor::callRecordAllocation(
250+
s, &mock_env, nullptr, nullptr, BCI_ALLOC,
251+
nullptr, nullptr, 1024);
252+
EXPECT_EQ(deallocate_calls, 0);
253+
}
254+
255+
// ---------------------------------------------------------------------------
256+
// T-04: GetClassSignature succeeds but writes NULL into *signature_ptr.
257+
// Deallocate MUST NOT be called (the NULL guard in the condition fires).
258+
// ---------------------------------------------------------------------------
259+
TEST_F(ObjectSamplerDeallocateTest, DeallocateNotCalledWhenSuccessButNullName) {
260+
setMockGetClassSignature(mock_GetClassSignature_success_null_name);
261+
ObjectSampler *s = ObjectSampler::instance();
262+
ObjectSamplerTestAccessor::setActive(s, true);
263+
ObjectSamplerTestAccessor::callRecordAllocation(
264+
s, &mock_env, nullptr, nullptr, BCI_ALLOC,
265+
nullptr, nullptr, 1024);
266+
EXPECT_EQ(deallocate_calls, 0);
267+
}
268+
269+
// ---------------------------------------------------------------------------
270+
// T-05: _active is false — recordAllocation returns immediately.
271+
// Deallocate MUST NOT be called.
272+
// ---------------------------------------------------------------------------
273+
TEST_F(ObjectSamplerDeallocateTest, DeallocateNotCalledWhenNotActive) {
274+
setMockGetClassSignature(mock_GetClassSignature_success);
275+
ObjectSampler *s = ObjectSampler::instance();
276+
ObjectSamplerTestAccessor::setActive(s, false);
277+
ObjectSamplerTestAccessor::callRecordAllocation(
278+
s, &mock_env, nullptr, nullptr, BCI_ALLOC,
279+
nullptr, nullptr, 1024);
280+
EXPECT_EQ(deallocate_calls, 0);
281+
}

0 commit comments

Comments
 (0)