Skip to content

Commit 4925a50

Browse files
manuel-alvarez-alvarezmanuel-alvarez-alvarez
authored
Fix issue with call sites in super calls to constructor (#7991)
Fix issue with call sites in super calls to constructor
1 parent c8030bd commit 4925a50

File tree

13 files changed

+293
-22
lines changed

13 files changed

+293
-22
lines changed

‎buildSrc/call-site-instrumentation-plugin/src/main/java/datadog/trace/plugin/csi/impl/AdviceGeneratorImpl.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -344,12 +344,10 @@ private static void writeAdviceMethodCall(
344344
final MethodCallExpr invokeStatic =
345345
new MethodCallExpr()
346346
.setScope(new NameExpr("handler"))
347-
.setName("method")
348-
.addArgument(opCode("INVOKESTATIC"))
347+
.setName("advice")
349348
.addArgument(new StringLiteralExpr(method.getOwner().getInternalName()))
350349
.addArgument(new StringLiteralExpr(method.getMethodName()))
351-
.addArgument(new StringLiteralExpr(method.getMethodType().getDescriptor()))
352-
.addArgument(new BooleanLiteralExpr(false));
350+
.addArgument(new StringLiteralExpr(method.getMethodType().getDescriptor()));
353351
body.addStatement(invokeStatic);
354352
}
355353
if (requiresCast(advice)) {

‎buildSrc/call-site-instrumentation-plugin/src/test/groovy/datadog/trace/plugin/csi/impl/AdviceGeneratorTest.groovy‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
4747
pointcut('java/security/MessageDigest', 'getInstance', '(Ljava/lang/String;)Ljava/security/MessageDigest;')
4848
statements(
4949
'handler.dupParameters(descriptor, StackDupMode.COPY);',
50-
'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$BeforeAdvice", "before", "(Ljava/lang/String;)V", false);',
50+
'handler.advice("datadog/trace/plugin/csi/impl/AdviceGeneratorTest$BeforeAdvice", "before", "(Ljava/lang/String;)V");',
5151
'handler.method(opcode, owner, name, descriptor, isInterface);'
5252
)
5353
}
@@ -78,7 +78,7 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
7878
advices(0) {
7979
pointcut('java/lang/String', 'replaceAll', '(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;')
8080
statements(
81-
'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$AroundAdvice", "around", "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;", false);'
81+
'handler.advice("datadog/trace/plugin/csi/impl/AdviceGeneratorTest$AroundAdvice", "around", "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;");'
8282
)
8383
}
8484
}
@@ -110,7 +110,7 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
110110
statements(
111111
'handler.dupInvoke(owner, descriptor, StackDupMode.COPY);',
112112
'handler.method(opcode, owner, name, descriptor, isInterface);',
113-
'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$AfterAdvice", "after", "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;", false);',
113+
'handler.advice("datadog/trace/plugin/csi/impl/AdviceGeneratorTest$AfterAdvice", "after", "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;");',
114114
)
115115
}
116116
}
@@ -142,7 +142,7 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
142142
statements(
143143
'handler.dupParameters(descriptor, StackDupMode.PREPEND_ARRAY_CTOR);',
144144
'handler.method(opcode, owner, name, descriptor, isInterface);',
145-
'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$AfterAdviceCtor", "after", "([Ljava/lang/Object;Ljava/net/URL;)Ljava/net/URL;", false);',
145+
'handler.advice("datadog/trace/plugin/csi/impl/AdviceGeneratorTest$AfterAdviceCtor", "after", "([Ljava/lang/Object;Ljava/net/URL;)Ljava/net/URL;");',
146146
)
147147
}
148148
}
@@ -208,7 +208,7 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
208208
statements(
209209
'handler.dupParameters(descriptor, StackDupMode.PREPEND_ARRAY);',
210210
'handler.invokeDynamic(name, descriptor, bootstrapMethodHandle, bootstrapMethodArguments);',
211-
'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$InvokeDynamicAfterAdvice", "after", "([Ljava/lang/Object;Ljava/lang/String;)Ljava/lang/String;", false);'
211+
'handler.advice("datadog/trace/plugin/csi/impl/AdviceGeneratorTest$InvokeDynamicAfterAdvice", "after", "([Ljava/lang/Object;Ljava/lang/String;)Ljava/lang/String;");'
212212
)
213213
}
214214
}
@@ -297,7 +297,7 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
297297
'handler.dupParameters(descriptor, StackDupMode.PREPEND_ARRAY);',
298298
'handler.invokeDynamic(name, descriptor, bootstrapMethodHandle, bootstrapMethodArguments);',
299299
'handler.loadConstantArray(bootstrapMethodArguments);',
300-
'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$InvokeDynamicWithConstantsAdvice", "after", "([Ljava/lang/Object;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/String;", false);'
300+
'handler.advice("datadog/trace/plugin/csi/impl/AdviceGeneratorTest$InvokeDynamicWithConstantsAdvice", "after", "([Ljava/lang/Object;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/String;");'
301301
)
302302
}
303303
}
@@ -393,7 +393,7 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
393393
statements(
394394
'int[] parameterIndices = new int[] { 0 };',
395395
'handler.dupParameters(descriptor, parameterIndices, owner);',
396-
'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$PartialArgumentsBeforeAdvice", "before", "(Ljava/lang/String;)V", false);',
396+
'handler.advice("datadog/trace/plugin/csi/impl/AdviceGeneratorTest$PartialArgumentsBeforeAdvice", "before", "(Ljava/lang/String;)V");',
397397
'handler.method(opcode, owner, name, descriptor, isInterface);',
398398
)
399399
}
@@ -402,7 +402,7 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
402402
statements(
403403
'int[] parameterIndices = new int[] { 1 };',
404404
'handler.dupParameters(descriptor, parameterIndices, null);',
405-
'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$PartialArgumentsBeforeAdvice", "before", "([Ljava/lang/Object;)V", false);',
405+
'handler.advice("datadog/trace/plugin/csi/impl/AdviceGeneratorTest$PartialArgumentsBeforeAdvice", "before", "([Ljava/lang/Object;)V");',
406406
'handler.method(opcode, owner, name, descriptor, isInterface);',
407407
)
408408
}
@@ -411,7 +411,7 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
411411
statements(
412412
'int[] parameterIndices = new int[] { 0 };',
413413
'handler.dupInvoke(owner, descriptor, parameterIndices);',
414-
'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$PartialArgumentsBeforeAdvice", "before", "(Ljava/lang/String;I)V", false);',
414+
'handler.advice("datadog/trace/plugin/csi/impl/AdviceGeneratorTest$PartialArgumentsBeforeAdvice", "before", "(Ljava/lang/String;I)V");',
415415
'handler.method(opcode, owner, name, descriptor, isInterface);',
416416
)
417417
}
@@ -443,7 +443,7 @@ final class AdviceGeneratorTest extends BaseCsiPluginTest {
443443
statements(
444444
'handler.dupParameters(descriptor, StackDupMode.PREPEND_ARRAY_CTOR);',
445445
'handler.method(opcode, owner, name, descriptor, isInterface);',
446-
'handler.method(Opcodes.INVOKESTATIC, "datadog/trace/plugin/csi/impl/AdviceGeneratorTest$SuperTypeReturnAdvice", "after", "([Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;", false);',
446+
'handler.advice("datadog/trace/plugin/csi/impl/AdviceGeneratorTest$SuperTypeReturnAdvice", "after", "([Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;");',
447447
'handler.instruction(Opcodes.CHECKCAST, "java/lang/StringBuilder");'
448448
)
449449
}

‎dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/CallSiteTransformer.java‎

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package datadog.trace.agent.tooling.bytebuddy.csi;
22

3+
import static datadog.trace.api.telemetry.LogCollector.SEND_TELEMETRY;
34
import static net.bytebuddy.jar.asm.ClassWriter.COMPUTE_MAXS;
45

56
import datadog.trace.agent.tooling.HelperInjector;
@@ -9,6 +10,8 @@
910
import datadog.trace.agent.tooling.csi.InvokeAdvice;
1011
import datadog.trace.agent.tooling.csi.InvokeDynamicAdvice;
1112
import java.security.ProtectionDomain;
13+
import java.util.Deque;
14+
import java.util.LinkedList;
1215
import javax.annotation.Nonnull;
1316
import net.bytebuddy.asm.AsmVisitorWrapper;
1417
import net.bytebuddy.description.field.FieldDescription;
@@ -24,9 +27,13 @@
2427
import net.bytebuddy.jar.asm.Type;
2528
import net.bytebuddy.pool.TypePool;
2629
import net.bytebuddy.utility.JavaModule;
30+
import org.slf4j.Logger;
31+
import org.slf4j.LoggerFactory;
2732

2833
public class CallSiteTransformer implements Instrumenter.TransformingAdvice {
2934

35+
private static Logger LOGGER = LoggerFactory.getLogger(CallSiteTransformer.class);
36+
3037
private static final Instrumenter.TransformingAdvice NO_OP =
3138
(builder, typeDescription, classLoader, module, pd) -> builder;
3239

@@ -111,7 +118,9 @@ public MethodVisitor visitMethod(
111118
final String[] exceptions) {
112119
final MethodVisitor delegated =
113120
super.visitMethod(access, name, descriptor, signature, exceptions);
114-
return new CallSiteMethodVisitor(advices, delegated);
121+
return "<init>".equals(name)
122+
? new CallSiteCtorMethodVisitor(advices, delegated)
123+
: new CallSiteMethodVisitor(advices, delegated);
115124
}
116125
}
117126

@@ -132,6 +141,7 @@ public void visitMethodInsn(
132141
final String name,
133142
final String descriptor,
134143
final boolean isInterface) {
144+
135145
CallSiteAdvice advice = advices.findAdvice(owner, name, descriptor);
136146
if (advice instanceof InvokeAdvice) {
137147
((InvokeAdvice) advice).apply(this, opcode, owner, name, descriptor, isInterface);
@@ -197,6 +207,11 @@ public void method(
197207
mv.visitMethodInsn(opcode, owner, name, descriptor, isInterface);
198208
}
199209

210+
@Override
211+
public void advice(String owner, String name, String descriptor) {
212+
mv.visitMethodInsn(Opcodes.INVOKESTATIC, owner, name, descriptor, false);
213+
}
214+
200215
@Override
201216
public void invokeDynamic(
202217
final String name,
@@ -265,4 +280,72 @@ private Type[] methodParamTypesWithThis(String owner, String methodDescriptor) {
265280
return methodParameterTypesWithThis;
266281
}
267282
}
283+
284+
private static class CallSiteCtorMethodVisitor extends CallSiteMethodVisitor {
285+
286+
private final Deque<String> newInvocations = new LinkedList<>();
287+
private boolean isSuperCall = false;
288+
289+
private CallSiteCtorMethodVisitor(
290+
@Nonnull final Advices advices, @Nonnull final MethodVisitor delegated) {
291+
super(advices, delegated);
292+
}
293+
294+
@Override
295+
public void visitEnd() {
296+
super.visitEnd();
297+
if (!newInvocations.isEmpty()) {
298+
LOGGER.debug(
299+
SEND_TELEMETRY,
300+
"There is an issue handling NEW bytecodes, remaining types {}",
301+
newInvocations);
302+
}
303+
}
304+
305+
@Override
306+
public void visitTypeInsn(final int opcode, final String type) {
307+
if (opcode == Opcodes.NEW) {
308+
newInvocations.addLast(type);
309+
}
310+
super.visitTypeInsn(opcode, type);
311+
}
312+
313+
@Override
314+
public void visitMethodInsn(
315+
final int opcode,
316+
final String owner,
317+
final String name,
318+
final String descriptor,
319+
final boolean isInterface) {
320+
try {
321+
if (opcode == Opcodes.INVOKESPECIAL && "<init>".equals(name)) {
322+
if (owner.equals(newInvocations.peekLast())) {
323+
newInvocations.removeLast();
324+
isSuperCall = false;
325+
} else {
326+
// no new before call to <init>
327+
isSuperCall = true;
328+
}
329+
}
330+
super.visitMethodInsn(opcode, owner, name, descriptor, isInterface);
331+
} finally {
332+
isSuperCall = false;
333+
}
334+
}
335+
336+
@Override
337+
public void advice(final String owner, final String name, final String descriptor) {
338+
if (isSuperCall) {
339+
// append this to the stack after super call
340+
mv.visitIntInsn(Opcodes.ALOAD, 0);
341+
}
342+
mv.visitMethodInsn(Opcodes.INVOKESTATIC, owner, name, descriptor, false);
343+
}
344+
345+
@Override
346+
public void dupParameters(final String methodDescriptor, final StackDupMode mode) {
347+
super.dupParameters(
348+
methodDescriptor, isSuperCall ? StackDupMode.PREPEND_ARRAY_SUPER_CTOR : mode);
349+
}
350+
}
268351
}

‎dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/CallSiteUtils.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ public static void dup(final MethodVisitor mv, final Type[] parameters, final St
151151
break;
152152
case PREPEND_ARRAY:
153153
case PREPEND_ARRAY_CTOR:
154+
case PREPEND_ARRAY_SUPER_CTOR:
154155
case APPEND_ARRAY:
155156
dupN(mv, parameters, mode);
156157
break;
@@ -280,11 +281,21 @@ private static void dupN(
280281
mv.visitInsn(POP);
281282
break;
282283
case PREPEND_ARRAY_CTOR:
283-
// move the array before the NEW and DUP opcodes
284+
// move the array before the uninitialized entry created by NEW and DUP
285+
// stack start = [uninitialized, uninitialized, arg_0, ..., arg_n]
286+
// stack end = [array, uninitialized, uninitialized, arg_0, ..., arg_n]
284287
mv.visitInsn(DUP_X2);
285288
loadArray(mv, arraySize, parameters);
286289
mv.visitInsn(POP);
287290
break;
291+
case PREPEND_ARRAY_SUPER_CTOR:
292+
// move the array before the uninitialized entry
293+
// stack start = [uninitialized, arg_0, ..., arg_n]
294+
// stack end = [array, uninitialized, arg_0, ..., arg_n]
295+
mv.visitInsn(DUP_X1);
296+
loadArray(mv, arraySize, parameters);
297+
mv.visitInsn(POP);
298+
break;
288299
case APPEND_ARRAY:
289300
loadArray(mv, arraySize, parameters);
290301
break;

‎dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/csi/CallSiteAdvice.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ interface MethodHandler {
2727
/** Performs a method invocation (static, special, virtual, interface...) */
2828
void method(int opcode, String owner, String name, String descriptor, boolean isInterface);
2929

30+
/** Performs an advice invocation (always static) */
31+
void advice(String owner, String name, String descriptor);
32+
3033
/** Performs a dynamic method invocation */
3134
void invokeDynamic(
3235
String name,
@@ -62,11 +65,10 @@ enum StackDupMode {
6265
COPY,
6366
/** Copies the parameters in an array and prepends it */
6467
PREPEND_ARRAY,
65-
/**
66-
* Copies the parameters in an array, prepends it and swaps the array with the uninitialized
67-
* instance in a ctor
68-
*/
68+
/** Copies the parameters in an array and adds it between NEW and DUP opcodes */
6969
PREPEND_ARRAY_CTOR,
70+
/** Copies the parameters in an array and adds it before the uninitialized instance in a ctor */
71+
PREPEND_ARRAY_SUPER_CTOR,
7072
/** Copies the parameters in an array and appends it */
7173
APPEND_ARRAY
7274
}

‎dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/csi/BaseCallSiteTest.groovy‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import net.bytebuddy.jar.asm.Type
1515
import net.bytebuddy.matcher.ElementMatcher
1616
import net.bytebuddy.utility.JavaModule
1717
import net.bytebuddy.utility.nullability.MaybeNull
18+
19+
import java.lang.reflect.Constructor
1820
import java.security.MessageDigest
1921

2022

@@ -81,6 +83,10 @@ class BaseCallSiteTest extends DDSpecification {
8183
return buildPointcut(String.getDeclaredMethod('concat', String))
8284
}
8385

86+
protected static Pointcut stringReaderPointcut() {
87+
return buildPointcut(StringReader.getDeclaredConstructor(String))
88+
}
89+
8490
protected static Pointcut messageDigestGetInstancePointcut() {
8591
return buildPointcut(MessageDigest.getDeclaredMethod('getInstance', String))
8692
}
@@ -100,6 +106,10 @@ class BaseCallSiteTest extends DDSpecification {
100106
return buildPointcut(Type.getType(executable.getDeclaringClass()).internalName, executable.name, Type.getType(executable).descriptor)
101107
}
102108

109+
protected static Pointcut buildPointcut(final Constructor<?> executable) {
110+
return buildPointcut(Type.getType(executable.getDeclaringClass()).internalName, "<init>", Type.getType(executable).descriptor)
111+
}
112+
103113
protected static Pointcut buildPointcut(final String type, final String method, final String descriptor) {
104114
return new Pointcut(type: type, method: method, descriptor: descriptor)
105115
}
@@ -157,6 +167,13 @@ class BaseCallSiteTest extends DDSpecification {
157167
return clazz.getConstructor().newInstance()
158168
}
159169

170+
protected static Class<?> loadClass(final Type type,
171+
final byte[] data,
172+
final ClassLoader loader = Thread.currentThread().contextClassLoader) {
173+
final classLoader = new ByteArrayClassLoader(loader, [(type.className): data])
174+
return classLoader.loadClass(type.className)
175+
}
176+
160177
protected static byte[] transformType(final Type source,
161178
final Type target,
162179
final CallSiteTransformer transformer,

0 commit comments

Comments
 (0)