Skip to content

Commit c5f8d47

Browse files
committed
Make advice protected call with MemberSubstitution
Currently fails with the following exception: ``` Bad access to protected data in invokevirtual Exception Details: Location: io/netty/util/concurrent/GlobalEventExecutor.execute(Ljava/lang/Runnable;)V @76: invokevirtual Reason: Type 'java/util/concurrent/AbstractExecutorService' (current frame, stack[0]) is not assignable to 'io/netty/util/concurrent/GlobalEventExecutor' Current Frame: bci: @76 flags: { } locals: { 'io/netty/util/concurrent/GlobalEventExecutor', 'java/lang/Runnable' } stack: { 'java/util/concurrent/AbstractExecutorService', 'java/lang/Runnable', null } Bytecode: 0x0000000: 2bc1 0111 9a00 2201 2ba5 001d b201 172b 0x0000010: b801 1d9a 0013 2bb6 0048 b601 2313 0125 0x0000020: b601 2b99 0006 a700 3a2b c101 2d99 0012 0x0000030: bb01 2f59 2bb8 0133 b701 354c a700 212a 0x0000040: c101 3799 0010 2ac0 0137 2b01 b601 3b4c 0x0000050: a700 0dbb 013d 592b 01b7 0140 4ca7 0003 0x0000060: 2a4d 2b4e 2dc7 000d bb00 a859 12a9 b700 0x0000070: acbf 2c2d b700 d62c b600 d89a 0007 2cb7 0x0000080: 00db a700 0301 4da7 0004 4d01 2ca5 0015 0x0000090: 2bc1 0111 9900 0e2b c001 1104 b901 4402 0x00000a0: 0057 a700 032c c600 052c bfb1 Exception Handler Table: bci [100, 133] => handler: 138 Stackmap Table: same_frame(@38) same_frame(@41) same_frame(@63) same_frame(@83) same_frame(@93) same_frame(@96) append_frame(@100,Object[#2],Object[#126]) same_frame(@114) same_frame(@130) full_frame(@133,{Object[#2],Object[#126]},{}) same_locals_1_stack_item_frame(@138,Object[#271]) append_frame(@139,Object[#271]) same_frame(@162) same_frame(@165) same_frame(@171) ```
1 parent cfd7f94 commit c5f8d47

File tree

7 files changed

+219
-94
lines changed

7 files changed

+219
-94
lines changed

dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/java/concurrent/ComparableRunnable.java

+4
Original file line numberDiff line numberDiff line change
@@ -35,4 +35,8 @@ public void run() {
3535
private TraceScope activate() {
3636
return null == continuation ? null : continuation.activate();
3737
}
38+
39+
public static <T extends Runnable & Comparable<T>> T cast(Runnable comparable) {
40+
return (T) comparable;
41+
}
3842
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
package datadog.trace.agent.tooling.bytebuddy;
2+
3+
import datadog.trace.agent.tooling.bytebuddy.advice.transformation.NewTaskForAdvicePlugin;
4+
import datadog.trace.agent.tooling.muzzle.MuzzleGradlePlugin;
5+
import java.io.IOException;
6+
import net.bytebuddy.build.Plugin;
7+
import net.bytebuddy.description.type.TypeDescription;
8+
import net.bytebuddy.dynamic.ClassFileLocator;
9+
import net.bytebuddy.dynamic.DynamicType;
10+
11+
public class CompositeGradlePlugin implements Plugin {
12+
private final Plugin[] delegates =
13+
new Plugin[] {new MuzzleGradlePlugin(), new NewTaskForAdvicePlugin()};
14+
15+
@Override
16+
public DynamicType.Builder<?> apply(
17+
DynamicType.Builder<?> builder,
18+
TypeDescription typeDescription,
19+
ClassFileLocator classFileLocator) {
20+
for (Plugin delegate : delegates) {
21+
if (delegate.matches(typeDescription)) {
22+
builder = delegate.apply(builder, typeDescription, classFileLocator);
23+
}
24+
}
25+
return builder;
26+
}
27+
28+
@Override
29+
public void close() throws IOException {
30+
for (Plugin delegate : delegates) {
31+
delegate.close();
32+
}
33+
}
34+
35+
@Override
36+
public boolean matches(TypeDescription target) {
37+
boolean matches = false;
38+
for (Plugin delegate : delegates) {
39+
matches |= delegate.matches(target);
40+
}
41+
return matches;
42+
}
43+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
package datadog.trace.agent.tooling.bytebuddy.advice.transformation;
2+
3+
import static net.bytebuddy.matcher.ElementMatchers.any;
4+
import static net.bytebuddy.matcher.ElementMatchers.named;
5+
6+
import java.lang.reflect.Method;
7+
import java.util.concurrent.AbstractExecutorService;
8+
import lombok.SneakyThrows;
9+
import net.bytebuddy.asm.MemberSubstitution;
10+
import net.bytebuddy.build.Plugin;
11+
import net.bytebuddy.description.ByteCodeElement;
12+
import net.bytebuddy.description.method.MethodDescription;
13+
import net.bytebuddy.description.type.TypeDescription;
14+
import net.bytebuddy.description.type.TypeList;
15+
import net.bytebuddy.dynamic.ClassFileLocator;
16+
import net.bytebuddy.dynamic.DynamicType;
17+
import net.bytebuddy.implementation.bytecode.StackManipulation;
18+
import net.bytebuddy.implementation.bytecode.member.MethodInvocation;
19+
import net.bytebuddy.pool.TypePool;
20+
import net.bytebuddy.utility.CompoundList;
21+
22+
/**
23+
* This Plugin is responsible for modifying WrapRunnableAsNewTaskInstrumentation's advice so it is
24+
* able to make a call to a protected method.
25+
*/
26+
public final class NewTaskForAdvicePlugin implements Plugin {
27+
Method newTaskForMethod;
28+
29+
@SneakyThrows
30+
public NewTaskForAdvicePlugin() {
31+
newTaskForMethod =
32+
AbstractExecutorService.class.getDeclaredMethod("newTaskFor", Runnable.class, Object.class);
33+
}
34+
35+
@Override
36+
public boolean matches(TypeDescription target) {
37+
return named(
38+
"datadog.trace.instrumentation.java.concurrent.WrapRunnableAsNewTaskInstrumentation$Wrap")
39+
.matches(target);
40+
}
41+
42+
@Override
43+
public DynamicType.Builder<?> apply(
44+
DynamicType.Builder<?> builder,
45+
TypeDescription typeDescription,
46+
ClassFileLocator classFileLocator) {
47+
return builder.visit(
48+
MemberSubstitution.strict()
49+
.method(named("superNewTaskFor"))
50+
// https://github.com/raphw/byte-buddy/issues/976
51+
// .replaceWith(newTaskForMethod)
52+
.replaceWith(new VisibilityIgnoringSubstitution(newTaskForMethod))
53+
.on(any()));
54+
}
55+
56+
@Override
57+
public void close() {}
58+
59+
// This is a hack until https://github.com/raphw/byte-buddy/issues/976 is resolved.
60+
private static final class VisibilityIgnoringSubstitution
61+
implements MemberSubstitution.Substitution.Factory {
62+
private final MethodDescription methodDescription;
63+
64+
public VisibilityIgnoringSubstitution(Method method) {
65+
this.methodDescription = new MethodDescription.ForLoadedMethod(method);
66+
}
67+
68+
@Override
69+
public MemberSubstitution.Substitution make(
70+
TypeDescription instrumentedType, MethodDescription instrumentedMethod, TypePool typePool) {
71+
return new VisibilityIgnoringSubstitutionInvocation(
72+
instrumentedType,
73+
new MemberSubstitution.Substitution.ForMethodInvocation.MethodResolver.Simple(
74+
methodDescription));
75+
}
76+
}
77+
78+
private static final class VisibilityIgnoringSubstitutionInvocation
79+
extends MemberSubstitution.Substitution.ForMethodInvocation {
80+
81+
/** The index of the this reference within a non-static method. */
82+
private static final int THIS_REFERENCE = 0;
83+
84+
private final MethodResolver methodResolver;
85+
86+
public VisibilityIgnoringSubstitutionInvocation(
87+
TypeDescription instrumentedType, MethodResolver methodResolver) {
88+
super(instrumentedType, methodResolver);
89+
this.methodResolver = methodResolver;
90+
}
91+
92+
// Copied from net.bytebuddy.asm.MemberSubstitution.Substitution.ForMethodInvocation so the
93+
// visibility restrictions could be removed.
94+
@Override
95+
public StackManipulation resolve(
96+
TypeDescription targetType,
97+
ByteCodeElement target,
98+
TypeList.Generic parameters,
99+
TypeDescription.Generic result,
100+
int freeOffset) {
101+
MethodDescription methodDescription =
102+
methodResolver.resolve(targetType, target, parameters, result);
103+
104+
// if (!methodDescription.isAccessibleTo(instrumentedType)) {
105+
// throw new IllegalStateException(instrumentedType + " cannot access " +
106+
// methodDescription);
107+
// }
108+
109+
TypeList.Generic mapped =
110+
methodDescription.isStatic()
111+
? methodDescription.getParameters().asTypeList()
112+
: new TypeList.Generic.Explicit(
113+
CompoundList.of(
114+
methodDescription.getDeclaringType(),
115+
methodDescription.getParameters().asTypeList()));
116+
117+
if (!methodDescription.getReturnType().asErasure().isAssignableTo(result.asErasure())) {
118+
throw new IllegalStateException(
119+
"Cannot assign return value of " + methodDescription + " to " + result);
120+
} else if (mapped.size() != parameters.size()) {
121+
throw new IllegalStateException(
122+
"Cannot invoke " + methodDescription + " on " + parameters.size() + " parameters");
123+
}
124+
for (int index = 0; index < mapped.size(); index++) {
125+
if (!parameters.get(index).asErasure().isAssignableTo(mapped.get(index).asErasure())) {
126+
throw new IllegalStateException(
127+
"Cannot invoke "
128+
+ methodDescription
129+
+ " on parameter "
130+
+ index
131+
+ " of type "
132+
+ parameters.get(index));
133+
}
134+
}
135+
return methodDescription.isVirtual()
136+
? MethodInvocation.invoke(methodDescription)
137+
.virtual(mapped.get(THIS_REFERENCE).asErasure())
138+
: MethodInvocation.invoke(methodDescription);
139+
}
140+
}
141+
}

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java

+1-21
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import datadog.trace.agent.tooling.Instrumenter;
44
import datadog.trace.bootstrap.WeakMap;
5-
import java.io.IOException;
65
import java.util.Collections;
76
import java.util.WeakHashMap;
87
import net.bytebuddy.build.Plugin;
@@ -54,24 +53,5 @@ public DynamicType.Builder<?> apply(
5453
}
5554

5655
@Override
57-
public void close() throws IOException {}
58-
59-
/** Compile-time Optimization used by gradle buildscripts. */
60-
public static class NoOp implements Plugin {
61-
@Override
62-
public boolean matches(final TypeDescription target) {
63-
return false;
64-
}
65-
66-
@Override
67-
public DynamicType.Builder<?> apply(
68-
final DynamicType.Builder<?> builder,
69-
final TypeDescription typeDescription,
70-
final ClassFileLocator classFileLocator) {
71-
return builder;
72-
}
73-
74-
@Override
75-
public void close() throws IOException {}
76-
}
56+
public void close() {}
7757
}

dd-java-agent/instrumentation/instrumentation.gradle

+2-2
Original file line numberDiff line numberDiff line change
@@ -24,15 +24,15 @@ subprojects { Project subProj ->
2424
transformation {
2525
// Applying NoOp optimizes build by applying bytebuddy plugin to only compileJava task
2626
tasks = ['compileJava', 'compileScala', 'compileKotlin']
27-
plugin = 'datadog.trace.agent.tooling.muzzle.MuzzleGradlePlugin$NoOp'
27+
plugin = 'net.bytebuddy.build.Plugin$NoOp'
2828
}
2929
}
3030

3131
subProj.afterEvaluate {
3232
subProj.byteBuddy {
3333
transformation {
3434
tasks = ['compileJava', 'compileScala', 'compileKotlin']
35-
plugin = 'datadog.trace.agent.tooling.muzzle.MuzzleGradlePlugin'
35+
plugin = 'datadog.trace.agent.tooling.bytebuddy.CompositeGradlePlugin'
3636
classPath = project(':dd-java-agent:agent-tooling').configurations.instrumentationMuzzle + subProj.configurations.compile + subProj.sourceSets.main.output
3737
}
3838
}

dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/NewTaskFor.java

-63
This file was deleted.

dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/WrapRunnableAsNewTaskInstrumentation.java

+28-8
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,20 @@
11
package datadog.trace.instrumentation.java.concurrent;
22

33
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.namedOneOf;
4-
import static datadog.trace.instrumentation.java.concurrent.NewTaskFor.newTaskFor;
4+
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ComparableRunnable.cast;
5+
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.ExcludeType.RUNNABLE;
6+
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.exclude;
57
import static java.util.Collections.singletonMap;
68
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
79
import static net.bytebuddy.matcher.ElementMatchers.named;
810

911
import com.google.auto.service.AutoService;
1012
import datadog.trace.agent.tooling.Instrumenter;
13+
import datadog.trace.bootstrap.instrumentation.java.concurrent.ComparableRunnable;
1114
import java.util.Map;
15+
import java.util.concurrent.AbstractExecutorService;
1216
import java.util.concurrent.Executor;
17+
import java.util.concurrent.FutureTask;
1318
import java.util.concurrent.RunnableFuture;
1419
import net.bytebuddy.asm.Advice;
1520
import net.bytebuddy.description.method.MethodDescription;
@@ -47,11 +52,6 @@ public ElementMatcher<? super TypeDescription> typeMatcher() {
4752
"org.glassfish.grizzly.threadpool.GrizzlyExecutorService");
4853
}
4954

50-
@Override
51-
public String[] helperClassNames() {
52-
return new String[] {packageName + ".NewTaskFor"};
53-
}
54-
5555
@Override
5656
public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
5757
return singletonMap(
@@ -66,8 +66,28 @@ public static final class Wrap {
6666
@Advice.OnMethodEnter
6767
public static void execute(
6868
@Advice.This Executor executor,
69-
@Advice.Argument(value = 0, readOnly = false) Runnable task) {
70-
task = newTaskFor(executor, task);
69+
@Advice.Argument(value = 0, readOnly = false) Runnable runnable) {
70+
// TODO write a slick instrumentation and instrument these types directly
71+
if (runnable instanceof RunnableFuture
72+
|| null == runnable
73+
|| exclude(RUNNABLE, runnable)
74+
|| runnable.getClass().getName().startsWith("slick.")) {
75+
return;
76+
}
77+
if (runnable instanceof Comparable) {
78+
runnable = new ComparableRunnable<>(cast(runnable));
79+
} else if (executor instanceof AbstractExecutorService) {
80+
runnable = superNewTaskFor((AbstractExecutorService) executor, runnable, null);
81+
} else {
82+
runnable = new FutureTask<>(runnable, null);
83+
}
84+
}
85+
86+
// This call is swapped out at compile time by
87+
// datadog.trace.agent.tooling.bytebuddy.advice.transformation.NewTaskForAdvicePlugin
88+
public static <T> RunnableFuture<T> superNewTaskFor(
89+
AbstractExecutorService executor, Runnable runnable, T value) {
90+
return null;
7191
}
7292

7393
@SuppressWarnings("rawtypes")

0 commit comments

Comments
 (0)