Skip to content

Commit 9f44814

Browse files
authored
Merge branch 'master' into alejandro.gonzalez/APPSEC-61693
2 parents 8c215b6 + 311d3bd commit 9f44814

8 files changed

Lines changed: 834 additions & 14 deletions

File tree

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,9 @@ public byte[] transform(
275275
return null;
276276
}
277277
ClassNode classNode = parseClassFile(classFilePath, classfileBuffer);
278-
checkMethodParameters(classNode);
278+
if (!checkMethodParameters(classNode, definitions, fullyQualifiedClassName)) {
279+
return null;
280+
}
279281
if (!checkRecordTypeAnnotation(classNode, definitions, fullyQualifiedClassName)) {
280282
return null;
281283
}
@@ -306,10 +308,11 @@ public byte[] transform(
306308
* instrumented the class, we will retransform for removing the instrumentation and then the
307309
* attribute is stripped. That's why we are preventing it even at load time.
308310
*/
309-
private void checkMethodParameters(ClassNode classNode) {
311+
private boolean checkMethodParameters(
312+
ClassNode classNode, List<ProbeDefinition> definitions, String fullyQualifiedClassName) {
310313
if (JAVA_AT_LEAST_19) {
311314
// bug is fixed since JDK19, no need to perform check
312-
return;
315+
return true;
313316
}
314317
boolean isRecord = ASMHelper.isRecord(classNode);
315318
// capping scanning of methods to 100 to avoid generated class with thousand of methods
@@ -328,13 +331,17 @@ private void checkMethodParameters(ClassNode classNode) {
328331
if (methodNode.parameters != null
329332
&& !methodNode.parameters.isEmpty()
330333
&& SpringHelper.isSpringUsingOnlyMethodParameters(DebuggerAgent.getInstrumentation())) {
331-
throw new RuntimeException(
334+
reportInstrumentationFails(
335+
definitions,
336+
fullyQualifiedClassName,
332337
"Method Parameters attribute detected, instrumentation not supported");
338+
return false;
333339
} else {
334340
// we found at leat a method with one parameter if name is not present we can stop there
335341
break;
336342
}
337343
}
344+
return true;
338345
}
339346

340347
/*

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/sink/DebuggerSink.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -220,13 +220,11 @@ public void addDiagnostics(ProbeId probeId, List<DiagnosticMessage> messages) {
220220
for (DiagnosticMessage msg : messages) {
221221
switch (msg.getKind()) {
222222
case INFO:
223-
LOGGER.info(msg.getMessage());
224-
break;
225223
case WARN:
226-
LOGGER.warn(msg.getMessage());
224+
LOGGER.debug(msg.getMessage());
227225
break;
228226
case ERROR:
229-
LOGGER.error(msg.getMessage());
227+
LOGGER.debug(msg.getMessage());
230228
reportError(probeId, msg);
231229
break;
232230
}

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3059,7 +3059,7 @@ public void methodParametersAttribute() throws Exception {
30593059
verify(probeStatusSink, times(1)).addError(probeIdCaptor.capture(), strCaptor.capture());
30603060
assertEquals(PROBE_ID.getId(), probeIdCaptor.getAllValues().get(0).getId());
30613061
assertEquals(
3062-
"Instrumentation failed for CapturedSnapshot01: java.lang.RuntimeException: Method Parameters attribute detected, instrumentation not supported",
3062+
"Instrumentation failed for CapturedSnapshot01: Method Parameters attribute detected, instrumentation not supported",
30633063
strCaptor.getAllValues().get(0));
30643064
} else {
30653065
Snapshot snapshot = assertOneSnapshot(listener);
@@ -3069,9 +3069,17 @@ public void methodParametersAttribute() throws Exception {
30693069

30703070
@Test
30713071
@EnabledForJreRange(min = JRE.JAVA_17)
3072-
public void methodParametersAttributeRecord() throws IOException, URISyntaxException {
3072+
public void methodParametersAttributeRecord() throws Exception {
30733073
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot29";
30743074
final String RECORD_NAME = "com.datadog.debugger.MyRecord1";
3075+
Config config = mock(Config.class);
3076+
when(config.isDebuggerCodeOriginEnabled()).thenReturn(false);
3077+
when(config.isDebuggerExceptionEnabled()).thenReturn(false);
3078+
when(config.isDynamicInstrumentationEnabled()).thenReturn(false);
3079+
Instrumentation inst = mock(Instrumentation.class);
3080+
Class<?> springClass = Class.forName("org.springframework.core.SpringVersion");
3081+
when(inst.getAllLoadedClasses()).thenReturn(new Class[] {springClass});
3082+
DebuggerAgent.run(config, inst, null);
30753083
TestSnapshotListener listener = installMethodProbeAtExit(RECORD_NAME, "<init>", null);
30763084
Map<String, byte[]> buffers =
30773085
compile(CLASS_NAME, SourceCompiler.DebugInfo.ALL, "17", Arrays.asList("-parameters"));
@@ -3089,7 +3097,7 @@ public void methodParametersAttributeRecord() throws IOException, URISyntaxExcep
30893097
verify(probeStatusSink, times(1)).addError(probeIdCaptor.capture(), strCaptor.capture());
30903098
assertEquals(PROBE_ID.getId(), probeIdCaptor.getAllValues().get(0).getId());
30913099
assertEquals(
3092-
"Instrumentation failed for com.datadog.debugger.MyRecord1: java.lang.RuntimeException: Method Parameters attribute detected, instrumentation not supported",
3100+
"Instrumentation failed for com.datadog.debugger.MyRecord1: Method Parameters attribute detected, instrumentation not supported",
30933101
strCaptor.getAllValues().get(0));
30943102
}
30953103
}
Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
package datadog.trace.util;
2+
3+
import java.util.HashMap;
4+
import java.util.TreeMap;
5+
import java.util.concurrent.ThreadLocalRandom;
6+
import java.util.function.Supplier;
7+
import org.openjdk.jmh.annotations.Benchmark;
8+
import org.openjdk.jmh.annotations.Fork;
9+
import org.openjdk.jmh.annotations.Measurement;
10+
import org.openjdk.jmh.annotations.Threads;
11+
import org.openjdk.jmh.annotations.Warmup;
12+
import org.openjdk.jmh.infra.Blackhole;
13+
14+
/**
15+
*
16+
*
17+
* <ul>
18+
* Benchmark to illustrate the trade-offs around case-insensitive Map look-ups - using either...
19+
* <li>(RECOMMENDED) TreeMap with Comparator of String::compareToIgnoreCase
20+
* <li>HashMap with look-ups using String::to<X>Case
21+
* </ul>
22+
*
23+
* <p>For case-insensitive lookups, TreeMap map creation is consistently faster because it avoids
24+
* String::to<X>Case calls.
25+
*
26+
* <p>Despite calls to String::to<X>Case, HashMap lookups are faster in single threaded
27+
* microbenchmark by 50% but are worse when frequently called in a multi-threaded system.
28+
*
29+
* <p>With many threads, the extra allocation from calling String::to<X>Case leads to frequent GCs
30+
* which has adverse impacts on the whole system. <code>
31+
* MacBook M1 with 1 thread (Java 21)
32+
*
33+
* Benchmark Mode Cnt Score Error Units
34+
* CaseInsensitiveMapBenchmark.create_hashMap thrpt 6 994213.041 ± 15718.903 ops/s
35+
* CaseInsensitiveMapBenchmark.create_treeMap thrpt 6 1522900.015 ± 21646.688 ops/s
36+
*
37+
* CaseInsensitiveMapBenchmark.get_hashMap thrpt 6 69149862.293 ± 9168648.566 ops/s
38+
* CaseInsensitiveMapBenchmark.get_treeMap thrpt 6 42796699.230 ± 9029447.805 ops/s
39+
* </code> <code>
40+
* MacBook M1 with 8 threads (Java 21)
41+
*
42+
* Benchmark Mode Cnt Score Error Units
43+
* CaseInsensitiveMapBenchmark.create_hashMap thrpt 6 6641003.483 ± 543210.409 ops/s
44+
* CaseInsensitiveMapBenchmark.create_treeMap thrpt 6 10030191.764 ± 1308865.113 ops/s
45+
*
46+
* CaseInsensitiveMapBenchmark.get_hashMap thrpt 6 38748031.837 ± 9012072.804 ops/s
47+
* CaseInsensitiveMapBenchmark.get_treeMap thrpt 6 173495470.789 ± 27824904.999 ops/s
48+
* </code>
49+
*/
50+
@Fork(2)
51+
@Warmup(iterations = 2)
52+
@Measurement(iterations = 3)
53+
@Threads(8)
54+
public class CaseInsensitiveMapBenchmark {
55+
static final String[] PREFIXES = {"foo", "bar", "baz", "quux"};
56+
57+
static final int NUM_SUFFIXES = 4;
58+
59+
static <T> T init(Supplier<T> supplier) {
60+
return supplier.get();
61+
}
62+
63+
static final String[] UPPER_PREFIXES =
64+
init(
65+
() -> {
66+
String[] upperPrefixes = new String[PREFIXES.length];
67+
for (int i = 0; i < PREFIXES.length; ++i) {
68+
upperPrefixes[i] = PREFIXES[i].toUpperCase();
69+
}
70+
return upperPrefixes;
71+
});
72+
73+
static final String[] LOOKUP_KEYS =
74+
init(
75+
() -> {
76+
ThreadLocalRandom curRandom = ThreadLocalRandom.current();
77+
78+
String[] keys = new String[32];
79+
for (int i = 0; i < keys.length; ++i) {
80+
int prefixIndex = curRandom.nextInt(PREFIXES.length);
81+
boolean toUpper = curRandom.nextBoolean();
82+
int suffixIndex = curRandom.nextInt(NUM_SUFFIXES + 1);
83+
84+
String key = PREFIXES[prefixIndex] + "-" + suffixIndex;
85+
keys[i] = toUpper ? key.toUpperCase() : key.toLowerCase();
86+
}
87+
return keys;
88+
});
89+
90+
static int sharedLookupIndex = 0;
91+
92+
static String nextLookupKey() {
93+
int localIndex = ++sharedLookupIndex;
94+
if (localIndex >= LOOKUP_KEYS.length) {
95+
sharedLookupIndex = localIndex = 0;
96+
}
97+
return LOOKUP_KEYS[localIndex];
98+
}
99+
100+
@Benchmark
101+
public void create_baseline(Blackhole blackhole) {
102+
for (int suffix = 0; suffix < NUM_SUFFIXES; ++suffix) {
103+
for (String prefix : PREFIXES) {
104+
blackhole.consume(prefix + "-" + suffix);
105+
blackhole.consume(Integer.valueOf(suffix));
106+
}
107+
}
108+
for (int suffix = 0; suffix < NUM_SUFFIXES; suffix += 2) {
109+
for (String prefix : UPPER_PREFIXES) {
110+
blackhole.consume(prefix + "-" + suffix);
111+
blackhole.consume(Integer.valueOf(suffix + 1));
112+
}
113+
}
114+
}
115+
116+
@Benchmark
117+
public void lookup_baseline(Blackhole blackhole) {
118+
blackhole.consume(nextLookupKey());
119+
}
120+
121+
@Benchmark
122+
public HashMap<String, Integer> create_hashMap() {
123+
return _create_hashMap();
124+
}
125+
126+
static HashMap<String, Integer> _create_hashMap() {
127+
HashMap<String, Integer> map = new HashMap<>();
128+
for (int suffix = 0; suffix < NUM_SUFFIXES; ++suffix) {
129+
for (String prefix : PREFIXES) {
130+
map.put(
131+
(prefix + "-" + suffix).toLowerCase(),
132+
suffix); // arguable, but real caller probably doesn't know the case ahead-of-time
133+
}
134+
}
135+
for (int suffix = 0; suffix < NUM_SUFFIXES; suffix += 2) {
136+
for (String prefix : UPPER_PREFIXES) {
137+
map.put((prefix + "-" + suffix).toLowerCase(), suffix + 1);
138+
}
139+
}
140+
return map;
141+
}
142+
143+
static final HashMap<String, Integer> HASH_MAP = _create_hashMap();
144+
145+
@Benchmark
146+
public Integer lookup_hashMap() {
147+
// This benchmark is still "correct" in multi-threaded context,
148+
// Map is populated under the class initialization lock and not changed thereafter
149+
return HASH_MAP.get(nextLookupKey().toLowerCase());
150+
}
151+
152+
@Benchmark
153+
public TreeMap<String, Integer> create_treeMap() {
154+
return _create_treeMap();
155+
}
156+
157+
static TreeMap<String, Integer> _create_treeMap() {
158+
TreeMap<String, Integer> map = new TreeMap<>(String::compareToIgnoreCase);
159+
for (int suffix = 0; suffix < NUM_SUFFIXES; ++suffix) {
160+
for (String prefix : PREFIXES) {
161+
map.put(prefix + "-" + suffix, suffix);
162+
}
163+
}
164+
for (int suffix = 0; suffix < NUM_SUFFIXES; suffix += 2) {
165+
for (String prefix : UPPER_PREFIXES) {
166+
map.put(prefix + "-" + suffix, suffix + 1);
167+
}
168+
}
169+
return map;
170+
}
171+
172+
static final TreeMap<String, Integer> TREE_MAP = _create_treeMap();
173+
174+
@Benchmark
175+
public Integer lookup_treeMap() {
176+
// This benchmark is still "correct" in multi-threaded context,
177+
// Map is populated under the initial class initialization lock and not changed thereafter
178+
return TREE_MAP.get(nextLookupKey());
179+
}
180+
181+
// TODO: Add ConcurrentSkipListMap & synchronized HashMap & TreeMap
182+
}

internal-api/src/jmh/java/datadog/trace/util/HashingBenchmark.java

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,18 @@
99
import org.openjdk.jmh.annotations.Warmup;
1010

1111
/**
12-
* In contrast to java.util.Objects.hash, datadog.util.HashingUtils.hash has overrides for different
13-
* parameter counts that allow most callers to avoid calling the var-arg version. This avoids the
14-
* common situation where the JIT's escape analysis is unable to elide the var-arg array allocation.
12+
*
13+
*
14+
* <ul>
15+
* Benchmark comparing HashingUtils.hash to Objects.hash
16+
* <li>(RECOMMENDED) HashingUtils.hash - avoids var-arg creation
17+
* <li>Object.hash - high allocation overhead from var-ags
18+
* </ul>
19+
*
20+
* <p>In contrast to java.util.Objects.hash, datadog.util.HashingUtils.hash has overrides for
21+
* different parameter counts that allow most callers to avoid calling the var-arg version. This
22+
* avoids the common situation where the JIT's escape analysis is unable to elide the var-arg array
23+
* allocation.
1524
*
1625
* <p>This results in 3-4x throughput, but more importantly no allocation as compared to GiBs / sec
1726
* with var-args. <code>

0 commit comments

Comments
 (0)