Skip to content

Commit e3224fd

Browse files
committed
Cap dump-time class insertions to MAX_CLASS_MAP_SIZE
lookupDuringDump() had no size check, so the JFR dump path could still grow _class_map past the cap Profiler::lookupClass() enforces at sample time.
1 parent a8733ae commit e3224fd

4 files changed

Lines changed: 76 additions & 31 deletions

File tree

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

Lines changed: 44 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ SharedLineNumberTable::~SharedLineNumberTable() {
6868

6969
void Lookup::fillNativeMethodInfo(MethodInfo *mi, const char *name,
7070
const char *lib_name) {
71-
mi->_class = _classes->lookupDuringDump("", 0);
71+
mi->_class = _classes->lookupDuringDump("", 0, Profiler::maxClassMapSize());
7272
// TODO return the library name once we figured out how to cooperate with the
7373
// backend
7474
// if (lib_name == NULL) {
@@ -117,7 +117,8 @@ void Lookup::fillNativeMethodInfo(MethodInfo *mi, const char *name,
117117

118118
void Lookup::fillRemoteFrameInfo(MethodInfo *mi, const RemoteFrameInfo *rfi) {
119119
// Store build-id in the class name field
120-
mi->_class = _classes->lookupDuringDump(rfi->build_id, strlen(rfi->build_id));
120+
mi->_class = _classes->lookupDuringDump(rfi->build_id, strlen(rfi->build_id),
121+
Profiler::maxClassMapSize());
121122

122123
// Store PC offset in hex format in the signature field
123124
char offset_hex[32];
@@ -269,35 +270,40 @@ void Lookup::fillJavaMethodInfo(MethodInfo *mi, jmethodID method,
269270
"Ljdk/internal/reflect/GeneratedConstructorAccessor")) {
270271
class_name_id = _classes->lookupDuringDump(
271272
"jdk/internal/reflect/GeneratedConstructorAccessor",
272-
strlen("jdk/internal/reflect/GeneratedConstructorAccessor"));
273+
strlen("jdk/internal/reflect/GeneratedConstructorAccessor"),
274+
Profiler::maxClassMapSize());
273275
method_name_id =
274276
_symbols.lookup("Object "
275277
"jdk.internal.reflect.GeneratedConstructorAccessor."
276278
"newInstance(Object[])");
277279
method_sig_id = _symbols.lookup(method_sig);
278280
} else if (has_prefix(class_name,
279281
"Lsun/reflect/GeneratedConstructorAccessor")) {
280-
class_name_id =
281-
_classes->lookupDuringDump("sun/reflect/GeneratedConstructorAccessor",
282-
strlen("sun/reflect/GeneratedConstructorAccessor"));
282+
class_name_id = _classes->lookupDuringDump(
283+
"sun/reflect/GeneratedConstructorAccessor",
284+
strlen("sun/reflect/GeneratedConstructorAccessor"),
285+
Profiler::maxClassMapSize());
283286
method_name_id = _symbols.lookup(
284287
"Object "
285288
"sun.reflect.GeneratedConstructorAccessor.newInstance(Object[])");
286289
method_sig_id = _symbols.lookup(method_sig);
287290
} else if (has_prefix(class_name,
288291
"Ljdk/internal/reflect/GeneratedMethodAccessor")) {
289-
class_name_id =
290-
_classes->lookupDuringDump("jdk/internal/reflect/GeneratedMethodAccessor",
291-
strlen("jdk/internal/reflect/GeneratedMethodAccessor"));
292+
class_name_id = _classes->lookupDuringDump(
293+
"jdk/internal/reflect/GeneratedMethodAccessor",
294+
strlen("jdk/internal/reflect/GeneratedMethodAccessor"),
295+
Profiler::maxClassMapSize());
292296
method_name_id =
293297
_symbols.lookup("Object "
294298
"jdk.internal.reflect.GeneratedMethodAccessor."
295299
"invoke(Object, Object[])");
296300
method_sig_id = _symbols.lookup(method_sig);
297301
} else if (has_prefix(class_name,
298302
"Lsun/reflect/GeneratedMethodAccessor")) {
299-
class_name_id = _classes->lookupDuringDump("sun/reflect/GeneratedMethodAccessor",
300-
strlen("sun/reflect/GeneratedMethodAccessor"));
303+
class_name_id = _classes->lookupDuringDump(
304+
"sun/reflect/GeneratedMethodAccessor",
305+
strlen("sun/reflect/GeneratedMethodAccessor"),
306+
Profiler::maxClassMapSize());
301307
method_name_id = _symbols.lookup(
302308
"Object sun.reflect.GeneratedMethodAccessor.invoke(Object, "
303309
"Object[])");
@@ -308,30 +314,38 @@ void Lookup::fillJavaMethodInfo(MethodInfo *mi, jmethodID method,
308314
// we want to normalise to java/lang/invoke/LambdaForm$MH,
309315
// java/lang/invoke/LambdaForm$DMH, java/lang/invoke/LambdaForm$BMH,
310316
if (has_prefix(class_name + lambdaFormPrefixLength, "MH")) {
311-
class_name_id = _classes->lookupDuringDump("java/lang/invoke/LambdaForm$MH",
312-
strlen("java/lang/invoke/LambdaForm$MH"));
317+
class_name_id = _classes->lookupDuringDump(
318+
"java/lang/invoke/LambdaForm$MH",
319+
strlen("java/lang/invoke/LambdaForm$MH"),
320+
Profiler::maxClassMapSize());
313321
} else if (has_prefix(class_name + lambdaFormPrefixLength, "BMH")) {
314-
class_name_id = _classes->lookupDuringDump("java/lang/invoke/LambdaForm$BMH",
315-
strlen("java/lang/invoke/LambdaForm$BMH"));
322+
class_name_id = _classes->lookupDuringDump(
323+
"java/lang/invoke/LambdaForm$BMH",
324+
strlen("java/lang/invoke/LambdaForm$BMH"),
325+
Profiler::maxClassMapSize());
316326
} else if (has_prefix(class_name + lambdaFormPrefixLength, "DMH")) {
317-
class_name_id = _classes->lookupDuringDump("java/lang/invoke/LambdaForm$DMH",
318-
strlen("java/lang/invoke/LambdaForm$DMH"));
327+
class_name_id = _classes->lookupDuringDump(
328+
"java/lang/invoke/LambdaForm$DMH",
329+
strlen("java/lang/invoke/LambdaForm$DMH"),
330+
Profiler::maxClassMapSize());
319331
} else {
320332
// don't recognise the suffix, so don't normalise
321333
class_name_id = _classes->lookupDuringDump(
322-
normalized_class_name, normalized_class_name_len);
334+
normalized_class_name, normalized_class_name_len,
335+
Profiler::maxClassMapSize());
323336
}
324337
method_name_id = _symbols.lookup(method_name);
325338
method_sig_id = _symbols.lookup(method_sig);
326339
} else {
327-
class_name_id = _classes->lookupDuringDump(normalized_class_name,
328-
normalized_class_name_len);
340+
class_name_id = _classes->lookupDuringDump(
341+
normalized_class_name, normalized_class_name_len,
342+
Profiler::maxClassMapSize());
329343
method_name_id = _symbols.lookup(method_name);
330344
method_sig_id = _symbols.lookup(method_sig);
331345
}
332346
} else {
333347
Counters::increment(JMETHODID_SKIPPED);
334-
class_name_id = _classes->lookupDuringDump("", 0);
348+
class_name_id = _classes->lookupDuringDump("", 0, Profiler::maxClassMapSize());
335349
method_name_id = _symbols.lookup("jvmtiError");
336350
method_sig_id = _symbols.lookup("()L;");
337351
}
@@ -426,7 +440,7 @@ bool Lookup::resolveVTableReceiver(VMSymbol *sym, char *buf, size_t bufsize,
426440
// the stack frame's class_id absent from this chunk's class pool.
427441
// (Plain lookup() remains correct for non-dump callers — e.g. Profiler::
428442
// lookupClass on JVM threads — where the next rotate() will propagate.)
429-
u32 class_id = _classes->lookupDuringDump(buf, len);
443+
u32 class_id = _classes->lookupDuringDump(buf, len, Profiler::maxClassMapSize());
430444
// Apply synthetic-accessor/LambdaForm normalisation so that the many
431445
// distinct names HotSpot generates for these families (..Accessor1234,
432446
// LambdaForm$MH/0x...) collapse to one bucket each in the JFR class pool.
@@ -436,32 +450,32 @@ bool Lookup::resolveVTableReceiver(VMSymbol *sym, char *buf, size_t bufsize,
436450
if (has_prefix_n(buf, len,
437451
"jdk/internal/reflect/GeneratedConstructorAccessor")) {
438452
static const char kName[] = "jdk/internal/reflect/GeneratedConstructorAccessor";
439-
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1);
453+
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize());
440454
} else if (has_prefix_n(buf, len, "sun/reflect/GeneratedConstructorAccessor")) {
441455
static const char kName[] = "sun/reflect/GeneratedConstructorAccessor";
442-
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1);
456+
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize());
443457
} else if (has_prefix_n(buf, len,
444458
"jdk/internal/reflect/GeneratedMethodAccessor")) {
445459
static const char kName[] = "jdk/internal/reflect/GeneratedMethodAccessor";
446-
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1);
460+
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize());
447461
} else if (has_prefix_n(buf, len, "sun/reflect/GeneratedMethodAccessor")) {
448462
static const char kName[] = "sun/reflect/GeneratedMethodAccessor";
449-
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1);
463+
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize());
450464
} else if (has_prefix_n(buf, len, "java/lang/invoke/LambdaForm$")) {
451465
size_t prefix_len = strlen("java/lang/invoke/LambdaForm$");
452466
const char *suffix = buf + prefix_len;
453467
size_t suffix_len = len - prefix_len;
454468
if (suffix_len >= 2 && suffix[0] == 'M' && suffix[1] == 'H') {
455469
static const char kName[] = "java/lang/invoke/LambdaForm$MH";
456-
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1);
470+
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize());
457471
} else if (suffix_len >= 3 && suffix[0] == 'B' && suffix[1] == 'M' &&
458472
suffix[2] == 'H') {
459473
static const char kName[] = "java/lang/invoke/LambdaForm$BMH";
460-
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1);
474+
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize());
461475
} else if (suffix_len >= 3 && suffix[0] == 'D' && suffix[1] == 'M' &&
462476
suffix[2] == 'H') {
463477
static const char kName[] = "java/lang/invoke/LambdaForm$DMH";
464-
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1);
478+
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize());
465479
}
466480
}
467481
*out_class_id = class_id;
@@ -485,7 +499,7 @@ u32 Lookup::resolveVTableReceiverCached(void *sym) {
485499
// marker instead of an empty class name (which is indistinguishable
486500
// from a parser/encoder error downstream).
487501
static const char kName[] = "<unresolved_vtable_receiver>";
488-
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1);
502+
class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize());
489503
}
490504
_vtable_receiver_cache[sym] = class_id;
491505
return class_id;

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,10 @@ class alignas(alignof(SpinLock)) Profiler {
271271
Engine *wallEngine() { return _wall_engine; }
272272

273273
StringDictionary *classMap() { return &_class_map; }
274+
// Same cap lookupClass() enforces for sample-time inserts; dump-time
275+
// insertions (Lookup::_classes->lookupDuringDump in flightRecorder.cpp)
276+
// must use this too so _class_map cannot grow past it via that path.
277+
static int maxClassMapSize() { return MAX_CLASS_MAP_SIZE; }
274278
SharedLockGuard classMapSharedGuard() { return SharedLockGuard(&_class_map_lock); }
275279
StringDictionary *stringLabelMap() { return &_string_label_map; }
276280
StringDictionary *contextValueMap() { return &_context_value_map; }

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include "tripleBuffer.h"
1313
#include "arch.h"
1414
#include <atomic>
15+
#include <climits>
1516
#include <map>
1617
#include <stddef.h>
1718
#include <stdlib.h>
@@ -550,7 +551,11 @@ class StringDictionary {
550551
// Resolve a key during the dump phase. Safe to call from the dump thread
551552
// after rotate(); must NOT be called from signal handlers or concurrently
552553
// with another lookupDuringDump call.
553-
u32 lookupDuringDump(const char* key, size_t len) {
554+
// size_limit bounds new insertions the same way bounded_lookup() does:
555+
// once active->size() reaches size_limit, previously-known keys still
556+
// resolve (dump/active hits above), but a genuinely new key returns 0
557+
// instead of growing the dictionary further.
558+
u32 lookupDuringDump(const char* key, size_t len, int size_limit = INT_MAX) {
554559
StringDictionaryBuffer* dump = _rot.dumpBuffer();
555560

556561
u32 id = dump->lookup(key, len);
@@ -571,6 +576,7 @@ class StringDictionary {
571576
StringDictionaryBuffer* active = _rot.active();
572577
RefCountGuard guard(active);
573578
if (!guard.isActive()) return 0;
579+
if (active->size() >= size_limit) return 0;
574580
u32 new_id = nextId();
575581
new_id = active->insert_with_id(key, len, new_id);
576582
if (new_id != 0) dump->insert_with_id(key, len, new_id);

ddprof-lib/src/test/cpp/stringDictionary_ut.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,3 +289,24 @@ TEST_F(StringDictionaryTest, LookupDuringDumpInsertsNewKeyIntoActiveAndStandby)
289289
// Must be in active (bounded_lookup is a probe of active)
290290
EXPECT_EQ(id, dict.bounded_lookup("brand/New", 9));
291291
}
292+
293+
TEST_F(StringDictionaryTest, LookupDuringDumpWithSizeLimitRejectsNewKeyAtCapacity) {
294+
dict.rotate(); // empty active becomes dump, fresh active
295+
296+
// Fill active up to size_limit distinct keys via the capped overload.
297+
const int size_limit = 5;
298+
for (int i = 0; i < size_limit; i++) {
299+
std::string k = "dump_cap_" + std::to_string(i);
300+
u32 id = dict.lookupDuringDump(k.c_str(), k.size(), size_limit);
301+
EXPECT_GT(id, 0u) << "insert " << i << " should have succeeded below the limit";
302+
}
303+
304+
// A brand-new key must be rejected once active->size() >= size_limit —
305+
// neither active nor the dump snapshot should gain the entry.
306+
EXPECT_EQ(0u, dict.lookupDuringDump("dump_overflow", 13, size_limit));
307+
308+
// An already-present key must still resolve to its existing id even at
309+
// capacity (dump/active hits are checked before the capacity check).
310+
u32 id0 = dict.lookupDuringDump("dump_cap_0", 10, size_limit);
311+
EXPECT_GT(id0, 0u);
312+
}

0 commit comments

Comments
 (0)