From 836f49205b7c4435d9d22226ff49e8222e6e4000 Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Sat, 7 Jul 2018 14:01:08 -0400 Subject: [PATCH 01/11] TraceConfigInstrumentation implements Instrumenter directly --- .../trace/agent/tooling/Instrumenter.java | 4 +-- .../TraceConfigInstrumentation.java | 25 ++++++++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java index 6003586367..48539b81e5 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java @@ -105,11 +105,11 @@ public interface Instrumenter { return getConfigEnabled("dd.integrations.enabled", true); } - protected static String getPropOrEnv(final String name) { + public static String getPropOrEnv(final String name) { return System.getProperty(name, System.getenv(propToEnvName(name))); } - private static String propToEnvName(final String name) { + public static String propToEnvName(final String name) { return name.toUpperCase().replace(".", "_"); } } diff --git a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java index 502eef6bdd..b0b8e9e991 100644 --- a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java +++ b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java @@ -17,9 +17,18 @@ import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; +/** + * TraceConfig Instrumentation does not extend Default. + * + *

Instead it directly implements Instrumenter#instrument() and adds one default Instrumenter for + * every configured class+method-list. + * + *

If this becomes a more common use case the building logic should be abstracted out into a + * super class. + */ @Slf4j @AutoService(Instrumenter.class) -public class TraceConfigInstrumentation extends Instrumenter.Default { +public class TraceConfigInstrumentation implements Instrumenter { private static final String CONFIG_NAME = "dd.trace.methods"; static final String PACKAGE_CLASS_NAME_REGEX = "[\\w.\\$]+"; @@ -38,9 +47,7 @@ public class TraceConfigInstrumentation extends Instrumenter.Default { private final Map> classMethodsToTrace; public TraceConfigInstrumentation() { - super("trace", "trace-config"); - - final String configString = getPropOrEnv(CONFIG_NAME); + final String configString = Default.getPropOrEnv(CONFIG_NAME); if (configString == null || configString.trim().isEmpty()) { classMethodsToTrace = Collections.emptyMap(); @@ -130,6 +137,16 @@ public class TraceConfigInstrumentation extends Instrumenter.Default { throw new RuntimeException("TracerConfigInstrumentation must not use TypeMatcher"); } + @Override + public ElementMatcher classLoaderMatcher() { + throw new RuntimeException("TracerConfigInstrumentation must not use classLoaderMatcher"); + } + + @Override + public String[] helperClassNames() { + throw new RuntimeException("TracerConfigInstrumentation must not use helperClassNames"); + } + @Override public Map transformers() { throw new RuntimeException("TracerConfigInstrumentation must not use transformers."); From d61f65f4ff79bee63a61c4e17e6f4b184831886e Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Sat, 7 Jul 2018 13:13:45 -0400 Subject: [PATCH 02/11] Muzzle outline in Instrumenter.Default --- .../trace/agent/tooling/Instrumenter.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java index 48539b81e5..6dc57a9c40 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java @@ -3,14 +3,18 @@ package datadog.trace.agent.tooling; import static datadog.trace.agent.tooling.Utils.getConfigEnabled; import static net.bytebuddy.matcher.ElementMatchers.any; +import java.security.ProtectionDomain; import java.util.Arrays; import java.util.HashSet; import java.util.Map; import java.util.Set; + +import datadog.trace.agent.tooling.muzzle.ReferenceMatcher; import lombok.extern.slf4j.Slf4j; import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; +import net.bytebuddy.utility.JavaModule; /** * Built-in bytebuddy-based instrumentation for the datadog javaagent. @@ -74,6 +78,13 @@ public interface Instrumenter { AgentBuilder.Identified.Extendable advice = agentBuilder .type(typeMatcher(), classLoaderMatcher()) + .and(new AgentBuilder.RawMatcher() { + @Override + public boolean matches(TypeDescription typeDescription, ClassLoader classLoader, JavaModule module, Class classBeingRedefined, ProtectionDomain protectionDomain) { + // Optimization: calling getMuzzleReferenceMatcher() inside this method prevents unnecessary loading of muzzle references during agentBuilder setup. + return getInstrumentationMuzzle().matches(classLoader); + } + }) .transform(DDTransformers.defaultTransformers()); final String[] helperClassNames = helperClassNames(); if (helperClassNames.length > 0) { @@ -85,6 +96,16 @@ public interface Instrumenter { return advice.asDecorator(); } + /** + * This method is implemented dynamically by compile-time bytecode transformations. + * + * TODO bytecode magic and documentation + */ + // TODO: Make final + protected ReferenceMatcher getInstrumentationMuzzle() { + return null; + } + @Override public String[] helperClassNames() { return new String[0]; From 541a6998a4b342f95ee985d41de04c67f4ed4091 Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Sat, 7 Jul 2018 13:55:31 -0400 Subject: [PATCH 03/11] Hook up muzzle to Instrumenter.Default's matcher --- .../muzzle/MuzzleBytecodeTransformTest.groovy | 9 +- .../trace/agent/tooling/AgentInstaller.java | 20 +-- .../trace/agent/tooling/Instrumenter.java | 40 +++-- .../tooling/muzzle/MuzzleGradlePlugin.java | 27 ++-- .../agent/tooling/muzzle/MuzzleVisitor.java | 152 +++++------------- .../tooling/muzzle/ReferenceMatcher.java | 67 +++----- .../src/lagomTest/java/EchoService.java | 1 + .../TraceConfigInstrumentation.java | 5 + .../muzzle/AdviceReferenceVisitorTest.groovy | 1 - 9 files changed, 124 insertions(+), 198 deletions(-) diff --git a/dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/muzzle/MuzzleBytecodeTransformTest.groovy b/dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/muzzle/MuzzleBytecodeTransformTest.groovy index 1e2bc31284..0269047eb5 100644 --- a/dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/muzzle/MuzzleBytecodeTransformTest.groovy +++ b/dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/muzzle/MuzzleBytecodeTransformTest.groovy @@ -8,13 +8,17 @@ import spock.lang.Specification class MuzzleBytecodeTransformTest extends Specification { - /* def "muzzle fields added to all instrumentation"() { setup: List unMuzzledClasses = [] List nonLazyFields = [] List unInitFields = [] - for (final Object instrumenter : ServiceLoader.load(IntegrationTestUtils.getAgentClassLoader().loadClass("datadog.trace.agent.tooling.Instrumenter"), IntegrationTestUtils.getAgentClassLoader())) { + for (Object instrumenter : ServiceLoader.load(IntegrationTestUtils.getAgentClassLoader().loadClass("datadog.trace.agent.tooling.Instrumenter"), IntegrationTestUtils.getAgentClassLoader())) { + if (instrumenter.getClass().getName().endsWith("TraceConfigInstrumentation")) { + // TraceConfigInstrumentation doesn't do muzzle checks + // check on TracerClassInstrumentation instead + instrumenter = IntegrationTestUtils.getAgentClassLoader().loadClass(instrumenter.getClass().getName() + '$TracerClassInstrumentation').newInstance() + } Field f Method m try { @@ -45,6 +49,5 @@ class MuzzleBytecodeTransformTest extends Specification { nonLazyFields == [] unInitFields == [] } - */ } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java index 685d88e9ec..7fecb25b68 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java @@ -8,8 +8,6 @@ import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.not; -import datadog.trace.agent.tooling.muzzle.Reference.Mismatch; -import datadog.trace.agent.tooling.muzzle.ReferenceMatcher.MismatchException; import java.lang.instrument.Instrumentation; import java.util.ServiceLoader; import lombok.extern.slf4j.Slf4j; @@ -91,19 +89,11 @@ public class AgentInstaller { final JavaModule module, final boolean loaded, final Throwable throwable) { - if (throwable instanceof MismatchException) { - final MismatchException mismatchException = (MismatchException) throwable; - log.debug("{}", mismatchException.getMessage()); - for (final Mismatch mismatch : mismatchException.getMismatches()) { - log.debug("--{}", mismatch); - } - } else { - log.debug( - "Failed to handle {} for transformation on classloader {}: {}", - typeName, - classLoader, - throwable.getMessage()); - } + log.debug( + "Failed to handle {} for transformation on classloader {}: {}", + typeName, + classLoader, + throwable.getMessage()); } @Override diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java index 6dc57a9c40..9377183556 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java @@ -3,13 +3,14 @@ package datadog.trace.agent.tooling; import static datadog.trace.agent.tooling.Utils.getConfigEnabled; import static net.bytebuddy.matcher.ElementMatchers.any; +import datadog.trace.agent.tooling.muzzle.Reference; +import datadog.trace.agent.tooling.muzzle.ReferenceMatcher; import java.security.ProtectionDomain; import java.util.Arrays; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; - -import datadog.trace.agent.tooling.muzzle.ReferenceMatcher; import lombok.extern.slf4j.Slf4j; import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.description.type.TypeDescription; @@ -46,11 +47,13 @@ public interface Instrumenter { @Slf4j abstract class Default implements Instrumenter { private final Set instrumentationNames; + private final String instrumentationPrimaryName; protected final boolean enabled; public Default(final String instrumentationName, final String... additionalNames) { this.instrumentationNames = new HashSet<>(Arrays.asList(additionalNames)); instrumentationNames.add(instrumentationName); + instrumentationPrimaryName = instrumentationName; // If default is enabled, we want to enable individually, // if default is disabled, we want to disable individually. @@ -78,13 +81,30 @@ public interface Instrumenter { AgentBuilder.Identified.Extendable advice = agentBuilder .type(typeMatcher(), classLoaderMatcher()) - .and(new AgentBuilder.RawMatcher() { - @Override - public boolean matches(TypeDescription typeDescription, ClassLoader classLoader, JavaModule module, Class classBeingRedefined, ProtectionDomain protectionDomain) { - // Optimization: calling getMuzzleReferenceMatcher() inside this method prevents unnecessary loading of muzzle references during agentBuilder setup. - return getInstrumentationMuzzle().matches(classLoader); - } - }) + .and( + new AgentBuilder.RawMatcher() { + @Override + public boolean matches( + TypeDescription typeDescription, + ClassLoader classLoader, + JavaModule module, + Class classBeingRedefined, + ProtectionDomain protectionDomain) { + // Optimization: calling getMuzzleReferenceMatcher() inside this method prevents unnecessary loading of muzzle references during agentBuilder setup. + final ReferenceMatcher muzzle = getInstrumentationMuzzle(); + if (null != muzzle) { + List mismatches = muzzle.getMismatchedReferenceSources(classLoader); + if (mismatches.size() > 0) { + log.debug("Instrumentation muzzled: {} on {}", instrumentationPrimaryName, classLoader); + } + for (Reference.Mismatch mismatch : mismatches) { + log.debug("-- {}", mismatch); + } + return mismatches.size() == 0; + } + return true; + } + }) .transform(DDTransformers.defaultTransformers()); final String[] helperClassNames = helperClassNames(); if (helperClassNames.length > 0) { @@ -99,7 +119,7 @@ public interface Instrumenter { /** * This method is implemented dynamically by compile-time bytecode transformations. * - * TODO bytecode magic and documentation + *

TODO bytecode magic and documentation */ // TODO: Make final protected ReferenceMatcher getInstrumentationMuzzle() { diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java index 4e5d83aa5c..bda30f93fa 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java @@ -8,10 +8,6 @@ import net.bytebuddy.dynamic.DynamicType.Builder; public class MuzzleGradlePlugin implements Plugin { // TODO: - // - Optimizations - // - Cache safe and unsafe classloaders - // - Do reference generation at compile time - // - lazy-load reference muzzle field // - Additional references to check // - Fields // - methods @@ -26,29 +22,38 @@ public class MuzzleGradlePlugin implements Plugin { // - Expose config instead of hardcoding datadog namespace (or reconfigure classpath) // - Run muzzle in matching phase (may require a rewrite of the instrumentation api) // - Documentation + // - Fix TraceConfigInstrumentation + // - assert no muzzle field defined in instrumentation + // - make getMuzzle final in default and remove in gradle plugin + // - pull muzzle field + method names into static constants - private static final TypeDescription InstrumenterTypeDesc = - new TypeDescription.ForLoadedType(Instrumenter.class); + private static final TypeDescription DefaultInstrumenterTypeDesc = + new TypeDescription.ForLoadedType(Instrumenter.Default.class); @Override public boolean matches(final TypeDescription target) { - // AutoService annotation is not retained at runtime. Check for instrumenter supertype + // AutoService annotation is not retained at runtime. Check for Instrumenter.Default supertype boolean isInstrumenter = false; TypeDefinition instrumenter = target; while (instrumenter != null) { - if (instrumenter.getInterfaces().contains(InstrumenterTypeDesc)) { + if (instrumenter.equals(DefaultInstrumenterTypeDesc)) { isInstrumenter = true; break; } instrumenter = instrumenter.getSuperClass(); } - // return isInstrumenter; - return false; + return isInstrumenter; } @Override public Builder apply(Builder builder, TypeDescription typeDescription) { - return builder.visit(new MuzzleVisitor()); + if (typeDescription.equals(DefaultInstrumenterTypeDesc)) { + // FIXME + System.out.println("~~~~FIXME: remove final modifier on Default: " + typeDescription); + return builder; + } else { + return builder.visit(new MuzzleVisitor()); + } } public static class NoOp implements Plugin { diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java index 81057d164b..bfca141143 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java @@ -1,6 +1,7 @@ package datadog.trace.agent.tooling.muzzle; -import java.util.Collection; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.agent.tooling.Utils; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -15,8 +16,9 @@ import net.bytebuddy.jar.asm.*; import net.bytebuddy.pool.TypePool; /** - * Visit a class and add: 1) a private instrumenationMuzzle field and getter AND 2) logic to the end - * of the instrumentation transformer to assert classpath is safe to apply instrumentation to. + * TODO: update doc Visit a class and add: 1) a private instrumenationMuzzle field and getter AND 2) + * logic to the end of the instrumentation transformer to assert classpath is safe to apply + * instrumentation to. */ public class MuzzleVisitor implements AsmVisitorWrapper { @Override @@ -44,7 +46,7 @@ public class MuzzleVisitor implements AsmVisitorWrapper { public static class InsertSafetyMatcher extends ClassVisitor { private String instrumentationClassName; - private Set adviceClassNames = new HashSet<>(); + private Instrumenter.Default instrumenter; public InsertSafetyMatcher(ClassVisitor classVisitor) { super(Opcodes.ASM6, classVisitor); @@ -59,6 +61,17 @@ public class MuzzleVisitor implements AsmVisitorWrapper { final String superName, final String[] interfaces) { this.instrumentationClassName = name; + try { + instrumenter = + (Instrumenter.Default) + MuzzleVisitor.class + .getClassLoader() + .loadClass(Utils.getClassName(instrumentationClassName)) + .getDeclaredConstructor() + .newInstance(); + } catch (Exception e) { + throw new RuntimeException(e); + } super.visit(version, access, name, signature, superName, interfaces); } @@ -74,13 +87,18 @@ public class MuzzleVisitor implements AsmVisitorWrapper { if ("".equals(name)) { methodVisitor = new InitializeFieldVisitor(methodVisitor); } - return new InsertMuzzleTransformer(methodVisitor); + return methodVisitor; } public Reference[] generateReferences() { // track sources we've generated references from to avoid recursion final Set referenceSources = new HashSet<>(); final Map references = new HashMap<>(); + final Set adviceClassNames = new HashSet<>(); + + for (String adviceClassName : instrumenter.transformers().values()) { + adviceClassNames.add(adviceClassName); + } for (String adviceClass : adviceClassNames) { if (!referenceSources.contains(adviceClass)) { @@ -105,10 +123,11 @@ public class MuzzleVisitor implements AsmVisitorWrapper { public void visitEnd() { { // generate getInstrumentationMuzzle method /* - * private synchronized ReferenceMatcher getInstrumentationMuzzle() { + * protected synchronized ReferenceMatcher getInstrumentationMuzzle() { * if (null == this.instrumentationMuzzle) { - * this.instrumentationMuzzle = new ReferenceMatcher(new Reference[]{ - * //reference builders + * this.instrumentationMuzzle = new ReferenceMatcher(this.helperClassNames(), + * new Reference[]{ + * //reference builders * }); * } * return this.instrumentationMuzzle; @@ -116,7 +135,7 @@ public class MuzzleVisitor implements AsmVisitorWrapper { */ final MethodVisitor mv = visitMethod( - Opcodes.ACC_PRIVATE + Opcodes.ACC_SYNCHRONIZED, + Opcodes.ACC_PROTECTED + Opcodes.ACC_SYNCHRONIZED, "getInstrumentationMuzzle", "()Ldatadog/trace/agent/tooling/muzzle/ReferenceMatcher;", null, @@ -137,10 +156,21 @@ public class MuzzleVisitor implements AsmVisitorWrapper { "Ldatadog/trace/agent/tooling/muzzle/ReferenceMatcher;"); mv.visitJumpInsn(Opcodes.IF_ACMPNE, ret); - final Reference[] references = generateReferences(); + mv.visitVarInsn(Opcodes.ALOAD, 0); + mv.visitTypeInsn(Opcodes.NEW, "datadog/trace/agent/tooling/muzzle/ReferenceMatcher"); mv.visitInsn(Opcodes.DUP); + + mv.visitVarInsn(Opcodes.ALOAD, 0); + mv.visitMethodInsn( + Opcodes.INVOKEVIRTUAL, + instrumentationClassName, + "helperClassNames", + "()[Ljava/lang/String;", + false); + + final Reference[] references = generateReferences(); mv.visitIntInsn(Opcodes.BIPUSH, references.length); mv.visitTypeInsn(Opcodes.ANEWARRAY, "datadog/trace/agent/tooling/muzzle/Reference"); @@ -275,7 +305,7 @@ public class MuzzleVisitor implements AsmVisitorWrapper { Opcodes.INVOKESPECIAL, "datadog/trace/agent/tooling/muzzle/ReferenceMatcher", "", - "([Ldatadog/trace/agent/tooling/muzzle/Reference;)V", + "([Ljava/lang/String;[Ldatadog/trace/agent/tooling/muzzle/Reference;)V", false); mv.visitFieldInsn( Opcodes.PUTFIELD, @@ -308,106 +338,6 @@ public class MuzzleVisitor implements AsmVisitorWrapper { super.visitEnd(); } - /** - * Changes this:
- *  .transform(DDAdvice.create().advice(named("fooMethod", FooAdvice.class.getname()))) - *  .asDecorator(); Into this:
- *  .transform(DDAdvice.create().advice(named("fooMethod", FooAdvice.class.getname()))) - *  .transform(this.instrumentationMuzzle.assertSafeTransformation("foo.package.FooAdvice")); - *  .asDecorator(); className) - */ - public class InsertMuzzleTransformer extends MethodVisitor { - // it would be nice to manage the state with an enum, but that requires this class to be non-static - private final int INIT = 0; - // SomeClass - private final int PREVIOUS_INSTRUCTION_LDC = 1; - // SomeClass.getName() - private final int PREVIOUS_INSTRUCTION_GET_CLASS_NAME = 2; - - private String lastClassLDC = null; - - private Collection adviceClassNames = new HashSet<>(); - private int STATE = INIT; - - public InsertMuzzleTransformer(MethodVisitor methodVisitor) { - super(Opcodes.ASM6, methodVisitor); - } - - public void reset() { - STATE = INIT; - lastClassLDC = null; - adviceClassNames.clear(); - } - - @Override - public void visitMethodInsn( - final int opcode, - final String owner, - final String name, - final String descriptor, - final boolean isInterface) { - if (name.equals("getName")) { - if (STATE == PREVIOUS_INSTRUCTION_LDC) { - STATE = PREVIOUS_INSTRUCTION_GET_CLASS_NAME; - } - } else if (name.equals("advice")) { - if (STATE == PREVIOUS_INSTRUCTION_GET_CLASS_NAME) { - adviceClassNames.add(lastClassLDC); - InsertSafetyMatcher.this.adviceClassNames.add(lastClassLDC); - } - // add last LDC/ToString to adivce list - } else if (name.equals("asDecorator")) { - this.visitVarInsn(Opcodes.ALOAD, 0); - mv.visitMethodInsn( - Opcodes.INVOKESPECIAL, - instrumentationClassName, - "getInstrumentationMuzzle", - "()Ldatadog/trace/agent/tooling/muzzle/ReferenceMatcher;", - false); - mv.visitIntInsn(Opcodes.BIPUSH, adviceClassNames.size()); - mv.visitTypeInsn(Opcodes.ANEWARRAY, "java/lang/String"); - int i = 0; - for (String adviceClassName : adviceClassNames) { - mv.visitInsn(Opcodes.DUP); - mv.visitIntInsn(Opcodes.BIPUSH, i); - mv.visitLdcInsn(adviceClassName); - mv.visitInsn(Opcodes.AASTORE); - ++i; - } - mv.visitMethodInsn( - Opcodes.INVOKEVIRTUAL, - "datadog/trace/agent/tooling/muzzle/ReferenceMatcher", - "assertSafeTransformation", - "([Ljava/lang/String;)Lnet/bytebuddy/agent/builder/AgentBuilder$Transformer;", - false); - mv.visitMethodInsn( - Opcodes.INVOKEINTERFACE, - "net/bytebuddy/agent/builder/AgentBuilder$Identified$Narrowable", - "transform", - "(Lnet/bytebuddy/agent/builder/AgentBuilder$Transformer;)Lnet/bytebuddy/agent/builder/AgentBuilder$Identified$Extendable;", - true); - reset(); - } else { - STATE = INIT; - lastClassLDC = null; - } - super.visitMethodInsn(opcode, owner, name, descriptor, isInterface); - } - - @Override - public void visitLdcInsn(final Object value) { - if (value instanceof Type) { - Type type = (Type) value; - if (type.getSort() == Type.OBJECT) { - lastClassLDC = type.getClassName(); - STATE = PREVIOUS_INSTRUCTION_LDC; - type.getClassName(); - } - } - super.visitLdcInsn(value); - } - } - /** Append a field initializer to the end of a method. */ public class InitializeFieldVisitor extends MethodVisitor { public InitializeFieldVisitor(MethodVisitor methodVisitor) { diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index cc89db7b0c..17fde8b047 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -7,9 +7,7 @@ import java.security.ProtectionDomain; import java.util.*; import lombok.extern.slf4j.Slf4j; import net.bytebuddy.agent.builder.AgentBuilder; -import net.bytebuddy.agent.builder.AgentBuilder.Transformer; import net.bytebuddy.description.type.TypeDescription; -import net.bytebuddy.dynamic.DynamicType; import net.bytebuddy.utility.JavaModule; /** @@ -21,9 +19,15 @@ public class ReferenceMatcher implements AgentBuilder.RawMatcher { private final Map> mismatchCache = Collections.synchronizedMap(new WeakHashMap>()); private final Reference[] references; + private final Set helperClassNames; public ReferenceMatcher(Reference... references) { + this(new String[0], references); + } + + public ReferenceMatcher(String[] helperClassNames, Reference[] references) { this.references = references; + this.helperClassNames = new HashSet<>(Arrays.asList(helperClassNames)); } @Override @@ -52,54 +56,23 @@ public class ReferenceMatcher implements AgentBuilder.RawMatcher { if (loader == BOOTSTRAP_LOADER) { loader = Utils.getBootstrapProxy(); } - final List mismatchedReferences = new ArrayList<>(0); - for (Reference reference : references) { - mismatchedReferences.addAll(reference.checkMatch(loader)); - } - return mismatchedReferences; - } - - /** - * Create a bytebuddy matcher which throws a MismatchException when there are mismatches with the - * classloader under transformation. - */ - public Transformer assertSafeTransformation(String... adviceClassNames) { - return new Transformer() { - @Override - public DynamicType.Builder transform( - DynamicType.Builder builder, - TypeDescription typeDescription, - ClassLoader classLoader, - JavaModule module) { - if (classLoader == BOOTSTRAP_LOADER) { - classLoader = Utils.getBootstrapProxy(); - } - List mismatches = - mismatchCache.get(getMismatchedReferenceSources(classLoader)); + List mismatches = mismatchCache.get(loader); + if (null == mismatches) { + synchronized (loader) { + mismatches = mismatchCache.get(loader); if (null == mismatches) { - // okay if entered by multiple callers during initialization - mismatches = getMismatchedReferenceSources(classLoader); - mismatchCache.put(classLoader, mismatches); - } - if (mismatches.size() == 0) { - return builder; - } else { - throw new MismatchException(classLoader, mismatches); + mismatches = new ArrayList<>(0); + for (Reference reference : references) { + // Don't reference-check helper classes. + // They will be injected by the instrumentation's HelperInjector. + if (!helperClassNames.contains(reference.getClassName())) { + mismatches.addAll(reference.checkMatch(loader)); + } + } + mismatchCache.put(loader, mismatches); } } - }; - } - - public static class MismatchException extends RuntimeException { - private final List mismatches; - - public MismatchException(ClassLoader classLoader, List mismatches) { - super(mismatches.size() + " mismatches on classloader: " + classLoader); - this.mismatches = mismatches; - } - - public List getMismatches() { - return this.mismatches; } + return mismatches; } } diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/java/EchoService.java b/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/java/EchoService.java index 747804b3d5..74db83c669 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/java/EchoService.java +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/java/EchoService.java @@ -10,6 +10,7 @@ public interface EchoService extends Service { ServiceCall, Source> error(); + @Override default Descriptor descriptor() { return named("echo").withCalls(namedCall("echo", this::echo), namedCall("error", this::error)); } diff --git a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java index b0b8e9e991..79744ec1c4 100644 --- a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java +++ b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java @@ -104,6 +104,11 @@ public class TraceConfigInstrumentation implements Instrumenter { private final String className; private final Set methodNames; + /** No-arg constructor only used by muzzle and tests. */ + public TracerClassInstrumentation() { + this("noop", Collections.singleton("noop")); + } + public TracerClassInstrumentation(String className, Set methodNames) { super("trace", "trace-config"); this.className = className; diff --git a/dd-java-agent/testing/src/test/groovy/muzzle/AdviceReferenceVisitorTest.groovy b/dd-java-agent/testing/src/test/groovy/muzzle/AdviceReferenceVisitorTest.groovy index 097b37f8af..49e17dd6b3 100644 --- a/dd-java-agent/testing/src/test/groovy/muzzle/AdviceReferenceVisitorTest.groovy +++ b/dd-java-agent/testing/src/test/groovy/muzzle/AdviceReferenceVisitorTest.groovy @@ -24,7 +24,6 @@ class AdviceReferenceVisitorTest extends AgentTestRunner { setup: Reference[] refs = AdviceReferenceVisitor.createReferencesFrom(AdviceClass.getName(), this.getClass().getClassLoader()).values().toArray(new Reference[0]) ReferenceMatcher refMatcher = new ReferenceMatcher(refs) - refMatcher.assertSafeTransformation() ClassLoader safeClassloader = new URLClassLoader([TestUtils.createJarWithClasses(AdviceClass$A, AdviceClass$SomeInterface, AdviceClass$SomeImplementation)] as URL[], From 628f4929dceba649a2eb357bf655966373990dd0 Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Mon, 9 Jul 2018 14:06:44 -0400 Subject: [PATCH 04/11] Muzzle Assertions and gradle plugin --- .circleci/config.yml | 4 +- .../src/main/groovy/MuzzleExtension.groovy | 4 ++ buildSrc/src/main/groovy/MuzzlePlugin.groovy | 53 ++++++++++++++++++ .../META-INF/gradle-plugins/muzzle.properties | 1 + .../trace/agent/tooling/AgentInstaller.java | 8 +-- .../trace/agent/tooling/Instrumenter.java | 8 ++- .../tooling/muzzle/MuzzleGradlePlugin.java | 55 +++++++++++++++++++ .../agent/tooling/muzzle/MuzzleVisitor.java | 23 ++++---- .../ApacheHttpClientInstrumentation.java | 2 +- .../instrumentation/instrumentation.gradle | 1 + .../mongo-async-3.3/mongo-async-3.3.gradle | 1 + 11 files changed, 139 insertions(+), 21 deletions(-) create mode 100644 buildSrc/src/main/groovy/MuzzleExtension.groovy create mode 100644 buildSrc/src/main/groovy/MuzzlePlugin.groovy create mode 100644 buildSrc/src/main/resources/META-INF/gradle-plugins/muzzle.properties diff --git a/.circleci/config.yml b/.circleci/config.yml index 17f5183cbb..6a53445270 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -180,8 +180,8 @@ jobs: - dd-trace-java-version-scan - run: - name: Verify Version Scan - command: ./gradlew verifyVersionScan --parallel --stacktrace --no-daemon --max-workers=6 + name: Verify Version Scan and Muzzle + command: ./gradlew verifyVersionScan muzzle --parallel --stacktrace --no-daemon --max-workers=6 - save_cache: key: dd-trace-java-version-scan-{{ checksum "dd-trace-java.gradle" }} diff --git a/buildSrc/src/main/groovy/MuzzleExtension.groovy b/buildSrc/src/main/groovy/MuzzleExtension.groovy new file mode 100644 index 0000000000..d8af853c9e --- /dev/null +++ b/buildSrc/src/main/groovy/MuzzleExtension.groovy @@ -0,0 +1,4 @@ +class MuzzleExtension { + String group + String module +} diff --git a/buildSrc/src/main/groovy/MuzzlePlugin.groovy b/buildSrc/src/main/groovy/MuzzlePlugin.groovy new file mode 100644 index 0000000000..433992e1d3 --- /dev/null +++ b/buildSrc/src/main/groovy/MuzzlePlugin.groovy @@ -0,0 +1,53 @@ +import org.gradle.api.Plugin +import org.gradle.api.Project + +import java.lang.reflect.Method + +class MuzzlePlugin implements Plugin { + @Override + void apply(Project project) { + def bootstrapProject = project.rootProject.getChildProjects().get('dd-java-agent').getChildProjects().get('agent-bootstrap') + def toolingProject = project.rootProject.getChildProjects().get('dd-java-agent').getChildProjects().get('agent-tooling') + project.extensions.create("muzzle", MuzzleExtension) + def muzzle = project.task('muzzle') { + group = 'Muzzle' + description = "Run instrumentation muzzle on compile time dependencies" + doLast { + List userUrls = new ArrayList<>() + project.getLogger().info("Creating user classpath for: " + project.getName()) + for (File f : project.configurations.compileOnly.getFiles()) { + project.getLogger().info( '--' + f) + userUrls.add(f.toURI().toURL()) + } + for (File f : bootstrapProject.sourceSets.main.runtimeClasspath.getFiles()) { + project.getLogger().info( '--' + f) + userUrls.add(f.toURI().toURL()) + } + final ClassLoader userCL = new URLClassLoader(userUrls.toArray(new URL[0]), (ClassLoader) null) + + project.getLogger().info("Creating dd classpath for: " + project.getName()) + Set ddUrls = new HashSet<>() + for (File f : toolingProject.sourceSets.main.runtimeClasspath.getFiles()) { + project.getLogger().info( '--' + f) + ddUrls.add(f.toURI().toURL()) + } + for(File f : project.sourceSets.main.runtimeClasspath.getFiles()) { + project.getLogger().info( '--' + f) + ddUrls.add(f.toURI().toURL()) + } + + final ClassLoader agentCL = new URLClassLoader(ddUrls.toArray(new URL[0]), (ClassLoader) null) + // find all instrumenters, get muzzle, and assert + Method assertionMethod = agentCL.loadClass('datadog.trace.agent.tooling.muzzle.MuzzleGradlePlugin') + .getMethod('assertAllInstrumentationIsMuzzled', ClassLoader.class) + assertionMethod.invoke(null, userCL) + } + } + // project.tasks.muzzle.dependsOn(bootstrapProject.tasks.shadowJar) + project.tasks.muzzle.dependsOn(bootstrapProject.tasks.compileJava) + project.tasks.muzzle.dependsOn(toolingProject.tasks.compileJava) + project.afterEvaluate { + project.tasks.muzzle.dependsOn(project.tasks.compileJava) + } + } +} diff --git a/buildSrc/src/main/resources/META-INF/gradle-plugins/muzzle.properties b/buildSrc/src/main/resources/META-INF/gradle-plugins/muzzle.properties new file mode 100644 index 0000000000..61b482e54a --- /dev/null +++ b/buildSrc/src/main/resources/META-INF/gradle-plugins/muzzle.properties @@ -0,0 +1 @@ +implementation-class=MuzzlePlugin diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java index 7fecb25b68..3a9aa0be71 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java @@ -90,10 +90,10 @@ public class AgentInstaller { final boolean loaded, final Throwable throwable) { log.debug( - "Failed to handle {} for transformation on classloader {}: {}", - typeName, - classLoader, - throwable.getMessage()); + "Failed to handle {} for transformation on classloader {}: {}", + typeName, + classLoader, + throwable.getMessage()); } @Override diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java index 9377183556..36eeecd581 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java @@ -93,9 +93,13 @@ public interface Instrumenter { // Optimization: calling getMuzzleReferenceMatcher() inside this method prevents unnecessary loading of muzzle references during agentBuilder setup. final ReferenceMatcher muzzle = getInstrumentationMuzzle(); if (null != muzzle) { - List mismatches = muzzle.getMismatchedReferenceSources(classLoader); + List mismatches = + muzzle.getMismatchedReferenceSources(classLoader); if (mismatches.size() > 0) { - log.debug("Instrumentation muzzled: {} on {}", instrumentationPrimaryName, classLoader); + log.debug( + "Instrumentation muzzled: {} on {}", + instrumentationPrimaryName, + classLoader); } for (Reference.Mismatch mismatch : mismatches) { log.debug("-- {}", mismatch); diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java index bda30f93fa..898003997d 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java @@ -1,6 +1,10 @@ package datadog.trace.agent.tooling.muzzle; +import datadog.trace.agent.tooling.HelperInjector; import datadog.trace.agent.tooling.Instrumenter; +import java.lang.reflect.Method; +import java.util.List; +import java.util.ServiceLoader; import net.bytebuddy.build.Plugin; import net.bytebuddy.description.type.TypeDefinition; import net.bytebuddy.description.type.TypeDescription; @@ -67,4 +71,55 @@ public class MuzzleGradlePlugin implements Plugin { return builder; } } + + public static void assertAllInstrumentationIsMuzzled(ClassLoader cl) throws Exception { + for (Instrumenter instrumenter : + ServiceLoader.load(Instrumenter.class, MuzzleGradlePlugin.class.getClassLoader())) { + if (instrumenter.getClass().getName().endsWith("TraceConfigInstrumentation")) { + // TraceConfigInstrumentation doesn't do muzzle checks + // check on TracerClassInstrumentation instead + instrumenter = + (Instrumenter) + MuzzleGradlePlugin.class + .getClassLoader() + .loadClass(instrumenter.getClass().getName() + "$TracerClassInstrumentation") + .getDeclaredConstructor() + .newInstance(); + } + Method m = null; + try { + m = instrumenter.getClass().getDeclaredMethod("getInstrumentationMuzzle"); + m.setAccessible(true); + ReferenceMatcher muzzle = (ReferenceMatcher) m.invoke(instrumenter); + List mismatches = muzzle.getMismatchedReferenceSources(cl); + if (mismatches.size() > 0) { + System.err.println( + "FAILED MUZZLE VALIDATION: " + instrumenter.getClass().getName() + " mismatches:"); + for (Reference.Mismatch mismatch : mismatches) { + System.err.println("-- " + mismatch); + } + throw new RuntimeException("Instrumentation failed Muzzle validation"); + } + } finally { + if (null != m) { + m.setAccessible(false); + } + } + try { + // Ratpack injects the scope manager as a helper. + // This is likely a bug, but we'll grandfather it out of the helper checks for now. + if (!instrumenter.getClass().getName().contains("Ratpack")) { + // verify helper injector works + final String[] helperClassNames = instrumenter.helperClassNames(); + if (helperClassNames.length > 0) { + new HelperInjector(helperClassNames).transform(null, null, cl, null); + } + } + } catch (Exception e) { + System.err.println( + "FAILED HELPER INJECTION. Are Helpers being injected in the correct order?"); + throw e; + } + } + } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java index bfca141143..d33567c8d2 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java @@ -63,12 +63,12 @@ public class MuzzleVisitor implements AsmVisitorWrapper { this.instrumentationClassName = name; try { instrumenter = - (Instrumenter.Default) - MuzzleVisitor.class - .getClassLoader() - .loadClass(Utils.getClassName(instrumentationClassName)) - .getDeclaredConstructor() - .newInstance(); + (Instrumenter.Default) + MuzzleVisitor.class + .getClassLoader() + .loadClass(Utils.getClassName(instrumentationClassName)) + .getDeclaredConstructor() + .newInstance(); } catch (Exception e) { throw new RuntimeException(e); } @@ -156,7 +156,6 @@ public class MuzzleVisitor implements AsmVisitorWrapper { "Ldatadog/trace/agent/tooling/muzzle/ReferenceMatcher;"); mv.visitJumpInsn(Opcodes.IF_ACMPNE, ret); - mv.visitVarInsn(Opcodes.ALOAD, 0); mv.visitTypeInsn(Opcodes.NEW, "datadog/trace/agent/tooling/muzzle/ReferenceMatcher"); @@ -164,11 +163,11 @@ public class MuzzleVisitor implements AsmVisitorWrapper { mv.visitVarInsn(Opcodes.ALOAD, 0); mv.visitMethodInsn( - Opcodes.INVOKEVIRTUAL, - instrumentationClassName, - "helperClassNames", - "()[Ljava/lang/String;", - false); + Opcodes.INVOKEVIRTUAL, + instrumentationClassName, + "helperClassNames", + "()[Ljava/lang/String;", + false); final Reference[] references = generateReferences(); mv.visitIntInsn(Opcodes.BIPUSH, references.length); diff --git a/dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java b/dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java index 1efe58cd48..319f7ea30d 100644 --- a/dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java +++ b/dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java @@ -45,7 +45,7 @@ public class ApacheHttpClientInstrumentation extends Instrumenter.Default { public String[] helperClassNames() { return new String[] { "datadog.trace.instrumentation.apachehttpclient.DDTracingClientExec", - "datadog.trace.instrumentation.apachehttpclient.DDTracingClientExec$HttpHeadersInjectAdapter" + "datadog.trace.instrumentation.apachehttpclient.DDTracingClientExec$HttpHeadersInjectAdapter", }; } diff --git a/dd-java-agent/instrumentation/instrumentation.gradle b/dd-java-agent/instrumentation/instrumentation.gradle index 96d379b9de..2106cc51bd 100644 --- a/dd-java-agent/instrumentation/instrumentation.gradle +++ b/dd-java-agent/instrumentation/instrumentation.gradle @@ -14,6 +14,7 @@ Project instr_project = project subprojects { subProj -> if (subProj.getParent() == instr_project) { apply plugin: "net.bytebuddy.byte-buddy" + apply plugin: 'muzzle' subProj.byteBuddy { transformation { diff --git a/dd-java-agent/instrumentation/mongo-async-3.3/mongo-async-3.3.gradle b/dd-java-agent/instrumentation/mongo-async-3.3/mongo-async-3.3.gradle index 1fc6ce75d4..3970abbb98 100644 --- a/dd-java-agent/instrumentation/mongo-async-3.3/mongo-async-3.3.gradle +++ b/dd-java-agent/instrumentation/mongo-async-3.3/mongo-async-3.3.gradle @@ -17,6 +17,7 @@ dependencies { compile(project(':dd-java-agent:instrumentation:mongo-3.1')) { transitive = false } + compileOnly group: 'org.mongodb', name: 'mongo-java-driver', version: '3.4.2' compileOnly group: 'org.mongodb', name: 'mongodb-driver-async', version: '3.4.2' compile project(':dd-java-agent:agent-tooling') From ce73c403bfd5f50de52f1273e15df0a1a8d74cda Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Mon, 9 Jul 2018 14:07:12 -0400 Subject: [PATCH 05/11] Elasticsearch5 helpers --- .../Elasticsearch5RestClientInstrumentation.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dd-java-agent/instrumentation/elasticsearch-rest-5/src/main/java/datadog/trace/instrumentation/elasticsearch5/Elasticsearch5RestClientInstrumentation.java b/dd-java-agent/instrumentation/elasticsearch-rest-5/src/main/java/datadog/trace/instrumentation/elasticsearch5/Elasticsearch5RestClientInstrumentation.java index 49e485b338..256c04e90e 100644 --- a/dd-java-agent/instrumentation/elasticsearch-rest-5/src/main/java/datadog/trace/instrumentation/elasticsearch5/Elasticsearch5RestClientInstrumentation.java +++ b/dd-java-agent/instrumentation/elasticsearch-rest-5/src/main/java/datadog/trace/instrumentation/elasticsearch5/Elasticsearch5RestClientInstrumentation.java @@ -35,6 +35,11 @@ public class Elasticsearch5RestClientInstrumentation extends Instrumenter.Defaul return false; } + @Override + public String[] helperClassNames() { + return new String[] {"datadog.trace.instrumentation.elasticsearch5.RestResponseListener"}; + } + @Override public ElementMatcher typeMatcher() { return not(isInterface()).and(named("org.elasticsearch.client.RestClient")); From 6faf63d6d88be96092af977169077d5d90d57f42 Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Mon, 9 Jul 2018 14:07:25 -0400 Subject: [PATCH 06/11] Add missing memcached helper --- .../spymemcached/MemcachedClientInstrumentation.java | 1 + 1 file changed, 1 insertion(+) diff --git a/dd-java-agent/instrumentation/spymemcached-2.12/src/main/java/datadog/trace/instrumentation/spymemcached/MemcachedClientInstrumentation.java b/dd-java-agent/instrumentation/spymemcached-2.12/src/main/java/datadog/trace/instrumentation/spymemcached/MemcachedClientInstrumentation.java index c3cc01de82..eb6923144f 100644 --- a/dd-java-agent/instrumentation/spymemcached-2.12/src/main/java/datadog/trace/instrumentation/spymemcached/MemcachedClientInstrumentation.java +++ b/dd-java-agent/instrumentation/spymemcached-2.12/src/main/java/datadog/trace/instrumentation/spymemcached/MemcachedClientInstrumentation.java @@ -49,6 +49,7 @@ public final class MemcachedClientInstrumentation extends Instrumenter.Default { public String[] helperClassNames() { return new String[] { HELPERS_PACKAGE + ".CompletionListener", + HELPERS_PACKAGE + ".SyncCompletionListener", HELPERS_PACKAGE + ".GetCompletionListener", HELPERS_PACKAGE + ".OperationCompletionListener", HELPERS_PACKAGE + ".BulkGetCompletionListener" From 33ec3f03043d5956e152daf4625f930d1340a60d Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Mon, 9 Jul 2018 14:07:42 -0400 Subject: [PATCH 07/11] Add missing ratpack helper --- .../trace/instrumentation/ratpack/RatpackInstrumentation.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/RatpackInstrumentation.java b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/RatpackInstrumentation.java index 1502300456..a536a86c45 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/RatpackInstrumentation.java +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/main/java/datadog/trace/instrumentation/ratpack/RatpackInstrumentation.java @@ -54,6 +54,8 @@ public final class RatpackInstrumentation extends Instrumenter.Default { // service registry helpers "datadog.trace.instrumentation.ratpack.impl.RatpackRequestExtractAdapter", "datadog.trace.instrumentation.ratpack.impl.RatpackScopeManager", + "datadog.trace.instrumentation.ratpack.impl.RatpackScopeManager$RatpackScope", + "datadog.trace.instrumentation.ratpack.impl.RatpackServerAdvice", "datadog.trace.instrumentation.ratpack.impl.RatpackServerAdvice$RatpackServerRegistryAdvice", "datadog.trace.instrumentation.ratpack.impl.TracingHandler" }; From a8a4ff2c23ed82091b5b9235f8f3d59f658f672f Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Mon, 9 Jul 2018 14:08:08 -0400 Subject: [PATCH 08/11] Remove classloader instrumentation --- settings.gradle | 1 - 1 file changed, 1 deletion(-) diff --git a/settings.gradle b/settings.gradle index eab8e57289..c4f843e258 100644 --- a/settings.gradle +++ b/settings.gradle @@ -13,7 +13,6 @@ include ':dd-java-agent:instrumentation:akka-http-10.0' include ':dd-java-agent:instrumentation:apache-httpclient-4.3' include ':dd-java-agent:instrumentation:aws-java-sdk-1.11.0' include ':dd-java-agent:instrumentation:aws-java-sdk-1.11.106' -include ':dd-java-agent:instrumentation:classloaders' include ':dd-java-agent:instrumentation:datastax-cassandra-3.2' include ':dd-java-agent:instrumentation:elasticsearch-rest-5' include ':dd-java-agent:instrumentation:elasticsearch-transport-2' From 8e182edc03d69a8d6cd4f87ba00e4ca1d00ed711 Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Tue, 10 Jul 2018 13:07:18 -0400 Subject: [PATCH 09/11] Replace BIPUSH with LDC to prevent overflow --- .../agent/tooling/muzzle/MuzzleVisitor.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java index d33567c8d2..fd662158f7 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java @@ -170,12 +170,12 @@ public class MuzzleVisitor implements AsmVisitorWrapper { false); final Reference[] references = generateReferences(); - mv.visitIntInsn(Opcodes.BIPUSH, references.length); + mv.visitLdcInsn(references.length); mv.visitTypeInsn(Opcodes.ANEWARRAY, "datadog/trace/agent/tooling/muzzle/Reference"); for (int i = 0; i < references.length; ++i) { mv.visitInsn(Opcodes.DUP); - mv.visitIntInsn(Opcodes.BIPUSH, i); + mv.visitLdcInsn(i); mv.visitTypeInsn(Opcodes.NEW, "datadog/trace/agent/tooling/muzzle/Reference$Builder"); mv.visitInsn(Opcodes.DUP); mv.visitLdcInsn(references[i].getClassName()); @@ -187,7 +187,7 @@ public class MuzzleVisitor implements AsmVisitorWrapper { false); for (Reference.Source source : references[i].getSources()) { mv.visitLdcInsn(source.getName()); - mv.visitIntInsn(Opcodes.BIPUSH, source.getLine()); + mv.visitLdcInsn(source.getLine()); mv.visitMethodInsn( Opcodes.INVOKEVIRTUAL, "datadog/trace/agent/tooling/muzzle/Reference$Builder", @@ -228,14 +228,14 @@ public class MuzzleVisitor implements AsmVisitorWrapper { } for (Reference.Field field : references[i].getFields()) { mv.visitLdcInsn(field.getName()); - mv.visitIntInsn(Opcodes.BIPUSH, field.getFlags().size()); + mv.visitLdcInsn(field.getFlags().size()); mv.visitTypeInsn( Opcodes.ANEWARRAY, "datadog/trace/agent/tooling/muzzle/Reference$Flag"); int j = 0; for (Reference.Flag flag : field.getFlags()) { mv.visitInsn(Opcodes.DUP); - mv.visitIntInsn(Opcodes.BIPUSH, j); + mv.visitLdcInsn(j); mv.visitFieldInsn( Opcodes.GETSTATIC, "datadog/trace/agent/tooling/muzzle/Reference$Flag", @@ -255,13 +255,13 @@ public class MuzzleVisitor implements AsmVisitorWrapper { for (Reference.Method method : references[i].getMethods()) { mv.visitLdcInsn(method.getName()); - mv.visitIntInsn(Opcodes.BIPUSH, method.getFlags().size()); + mv.visitLdcInsn(method.getFlags().size()); mv.visitTypeInsn( Opcodes.ANEWARRAY, "datadog/trace/agent/tooling/muzzle/Reference$Flag"); int j = 0; for (Reference.Flag flag : method.getFlags()) { mv.visitInsn(Opcodes.DUP); - mv.visitIntInsn(Opcodes.BIPUSH, j); + mv.visitLdcInsn(j); mv.visitFieldInsn( Opcodes.GETSTATIC, "datadog/trace/agent/tooling/muzzle/Reference$Flag", @@ -273,13 +273,13 @@ public class MuzzleVisitor implements AsmVisitorWrapper { mv.visitLdcInsn(method.getReturnType()); - mv.visitIntInsn(Opcodes.BIPUSH, method.getParameterTypes().size()); + mv.visitLdcInsn(method.getParameterTypes().size()); mv.visitTypeInsn(Opcodes.ANEWARRAY, "java/lang/String"); int k = 0; for (String parameterType : method.getParameterTypes()) { mv.visitInsn(Opcodes.DUP); - mv.visitIntInsn(Opcodes.BIPUSH, k); + mv.visitLdcInsn(k); mv.visitLdcInsn(parameterType); mv.visitInsn(Opcodes.AASTORE); } From 2925df8de5de99d2c7ff55395758ac2fbcca37db Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Tue, 10 Jul 2018 10:41:19 -0400 Subject: [PATCH 10/11] Clean up and document --- buildSrc/src/main/groovy/MuzzlePlugin.groovy | 9 +- .../trace/agent/tooling/Instrumenter.java | 3 +- .../tooling/muzzle/MuzzleGradlePlugin.java | 138 ++++++++---------- .../muzzle/MuzzleVersionScanPlugin.java | 86 +++++++++++ .../agent/tooling/muzzle/MuzzleVisitor.java | 8 +- .../trace/agent/tooling/muzzle/Reference.java | 2 +- ...enceVisitor.java => ReferenceCreator.java} | 109 +++++++------- .../tooling/muzzle/ReferenceMatcher.java | 21 +-- .../muzzle/AdviceReferenceVisitorTest.groovy | 6 +- 9 files changed, 217 insertions(+), 165 deletions(-) create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVersionScanPlugin.java rename dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/{AdviceReferenceVisitor.java => ReferenceCreator.java} (92%) diff --git a/buildSrc/src/main/groovy/MuzzlePlugin.groovy b/buildSrc/src/main/groovy/MuzzlePlugin.groovy index 433992e1d3..3e4a502ef6 100644 --- a/buildSrc/src/main/groovy/MuzzlePlugin.groovy +++ b/buildSrc/src/main/groovy/MuzzlePlugin.groovy @@ -3,6 +3,11 @@ import org.gradle.api.Project import java.lang.reflect.Method +/** + * POC muzzle task plugin which runs muzzle validation against an instrumentation's compile-time dependencies. + * + *

TODO: merge this with version scan + */ class MuzzlePlugin implements Plugin { @Override void apply(Project project) { @@ -38,8 +43,8 @@ class MuzzlePlugin implements Plugin { final ClassLoader agentCL = new URLClassLoader(ddUrls.toArray(new URL[0]), (ClassLoader) null) // find all instrumenters, get muzzle, and assert - Method assertionMethod = agentCL.loadClass('datadog.trace.agent.tooling.muzzle.MuzzleGradlePlugin') - .getMethod('assertAllInstrumentationIsMuzzled', ClassLoader.class) + Method assertionMethod = agentCL.loadClass('datadog.trace.agent.tooling.muzzle.MuzzleVersionScanPlugin') + .getMethod('assertInstrumentationNotMuzzled', ClassLoader.class) assertionMethod.invoke(null, userCL) } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java index 36eeecd581..3e3be40726 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java @@ -123,9 +123,8 @@ public interface Instrumenter { /** * This method is implemented dynamically by compile-time bytecode transformations. * - *

TODO bytecode magic and documentation + *

{@see datadog.trace.agent.tooling.muzzle.MuzzleGradlePlugin} */ - // TODO: Make final protected ReferenceMatcher getInstrumentationMuzzle() { return null; } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java index 898003997d..b2703ea16c 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java @@ -1,36 +1,22 @@ package datadog.trace.agent.tooling.muzzle; -import datadog.trace.agent.tooling.HelperInjector; import datadog.trace.agent.tooling.Instrumenter; -import java.lang.reflect.Method; -import java.util.List; -import java.util.ServiceLoader; +import net.bytebuddy.asm.AsmVisitorWrapper; import net.bytebuddy.build.Plugin; +import net.bytebuddy.description.field.FieldDescription; +import net.bytebuddy.description.field.FieldList; +import net.bytebuddy.description.method.MethodList; import net.bytebuddy.description.type.TypeDefinition; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.dynamic.DynamicType.Builder; +import net.bytebuddy.implementation.Implementation; +import net.bytebuddy.jar.asm.ClassVisitor; +import net.bytebuddy.jar.asm.MethodVisitor; +import net.bytebuddy.jar.asm.Opcodes; +import net.bytebuddy.pool.TypePool; +/** Bytebuddy gradle plugin which creates muzzle-references at compile time. */ public class MuzzleGradlePlugin implements Plugin { - // TODO: - // - Additional references to check - // - Fields - // - methods - // - visit annotations - // - visit parameter types - // - visit method instructions - // - method invoke type - // - access flags (including implicit package-private) - // - supertypes - // - Misc - // - Also match interfaces which extend Instrumenter - // - Expose config instead of hardcoding datadog namespace (or reconfigure classpath) - // - Run muzzle in matching phase (may require a rewrite of the instrumentation api) - // - Documentation - // - Fix TraceConfigInstrumentation - // - assert no muzzle field defined in instrumentation - // - make getMuzzle final in default and remove in gradle plugin - // - pull muzzle field + method names into static constants - private static final TypeDescription DefaultInstrumenterTypeDesc = new TypeDescription.ForLoadedType(Instrumenter.Default.class); @@ -38,7 +24,7 @@ public class MuzzleGradlePlugin implements Plugin { public boolean matches(final TypeDescription target) { // AutoService annotation is not retained at runtime. Check for Instrumenter.Default supertype boolean isInstrumenter = false; - TypeDefinition instrumenter = target; + TypeDefinition instrumenter = null == target ? null : target.getSuperClass(); while (instrumenter != null) { if (instrumenter.equals(DefaultInstrumenterTypeDesc)) { isInstrumenter = true; @@ -51,15 +37,10 @@ public class MuzzleGradlePlugin implements Plugin { @Override public Builder apply(Builder builder, TypeDescription typeDescription) { - if (typeDescription.equals(DefaultInstrumenterTypeDesc)) { - // FIXME - System.out.println("~~~~FIXME: remove final modifier on Default: " + typeDescription); - return builder; - } else { - return builder.visit(new MuzzleVisitor()); - } + return builder.visit(new MuzzleVisitor()); } + /** Compile-time Optimization used by gradle buildscripts. */ public static class NoOp implements Plugin { @Override public boolean matches(final TypeDescription target) { @@ -72,53 +53,56 @@ public class MuzzleGradlePlugin implements Plugin { } } - public static void assertAllInstrumentationIsMuzzled(ClassLoader cl) throws Exception { - for (Instrumenter instrumenter : - ServiceLoader.load(Instrumenter.class, MuzzleGradlePlugin.class.getClassLoader())) { - if (instrumenter.getClass().getName().endsWith("TraceConfigInstrumentation")) { - // TraceConfigInstrumentation doesn't do muzzle checks - // check on TracerClassInstrumentation instead - instrumenter = - (Instrumenter) - MuzzleGradlePlugin.class - .getClassLoader() - .loadClass(instrumenter.getClass().getName() + "$TracerClassInstrumentation") - .getDeclaredConstructor() - .newInstance(); + private static class RemoveFinalFlagVisitor implements AsmVisitorWrapper { + final String methodName; + + public RemoveFinalFlagVisitor(String methodName) { + this.methodName = methodName; + } + + @Override + public int mergeWriter(int flags) { + return flags; + } + + @Override + public int mergeReader(int flags) { + return flags; + } + + @Override + public ClassVisitor wrap( + TypeDescription instrumentedType, + ClassVisitor classVisitor, + Implementation.Context implementationContext, + TypePool typePool, + FieldList fields, + MethodList methods, + int writerFlags, + int readerFlags) { + return new Visitor(classVisitor); + } + + private class Visitor extends ClassVisitor { + public Visitor(ClassVisitor cv) { + super(Opcodes.ASM6, cv); } - Method m = null; - try { - m = instrumenter.getClass().getDeclaredMethod("getInstrumentationMuzzle"); - m.setAccessible(true); - ReferenceMatcher muzzle = (ReferenceMatcher) m.invoke(instrumenter); - List mismatches = muzzle.getMismatchedReferenceSources(cl); - if (mismatches.size() > 0) { - System.err.println( - "FAILED MUZZLE VALIDATION: " + instrumenter.getClass().getName() + " mismatches:"); - for (Reference.Mismatch mismatch : mismatches) { - System.err.println("-- " + mismatch); - } - throw new RuntimeException("Instrumentation failed Muzzle validation"); + + @Override + public MethodVisitor visitMethod( + final int access, + final String name, + final String descriptor, + final String signature, + final String[] exceptions) { + MethodVisitor methodVisitor = + super.visitMethod(access, name, descriptor, signature, exceptions); + if (name.equals(methodName) && (access & Opcodes.ACC_FINAL) != 0) { + return super.visitMethod( + access ^ Opcodes.ACC_FINAL, name, descriptor, signature, exceptions); + } else { + return super.visitMethod(access, name, descriptor, signature, exceptions); } - } finally { - if (null != m) { - m.setAccessible(false); - } - } - try { - // Ratpack injects the scope manager as a helper. - // This is likely a bug, but we'll grandfather it out of the helper checks for now. - if (!instrumenter.getClass().getName().contains("Ratpack")) { - // verify helper injector works - final String[] helperClassNames = instrumenter.helperClassNames(); - if (helperClassNames.length > 0) { - new HelperInjector(helperClassNames).transform(null, null, cl, null); - } - } - } catch (Exception e) { - System.err.println( - "FAILED HELPER INJECTION. Are Helpers being injected in the correct order?"); - throw e; } } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVersionScanPlugin.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVersionScanPlugin.java new file mode 100644 index 0000000000..f6b37051cc --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVersionScanPlugin.java @@ -0,0 +1,86 @@ +package datadog.trace.agent.tooling.muzzle; + +import datadog.trace.agent.tooling.HelperInjector; +import datadog.trace.agent.tooling.Instrumenter; +import java.lang.reflect.Method; +import java.util.List; +import java.util.ServiceLoader; + +/** + * Entry point for muzzle version scan gradle plugin. + * + *

For each instrumenter on the classpath, run muzzle validation and throw an exception if any + * mismatches are detected. + * + *

Additionally, after a successful muzzle validation run each instrumenter's helper injector. + */ +public class MuzzleVersionScanPlugin { + public static void assertInstrumentationNotMuzzled(ClassLoader cl) throws Exception { + // muzzle validate all instrumenters + for (Instrumenter instrumenter : + ServiceLoader.load(Instrumenter.class, MuzzleGradlePlugin.class.getClassLoader())) { + if (instrumenter.getClass().getName().endsWith("TraceConfigInstrumentation")) { + // TraceConfigInstrumentation doesn't do muzzle checks + // check on TracerClassInstrumentation instead + instrumenter = + (Instrumenter) + MuzzleGradlePlugin.class + .getClassLoader() + .loadClass(instrumenter.getClass().getName() + "$TracerClassInstrumentation") + .getDeclaredConstructor() + .newInstance(); + } + Method m = null; + try { + m = instrumenter.getClass().getDeclaredMethod("getInstrumentationMuzzle"); + m.setAccessible(true); + ReferenceMatcher muzzle = (ReferenceMatcher) m.invoke(instrumenter); + List mismatches = muzzle.getMismatchedReferenceSources(cl); + if (mismatches.size() > 0) { + System.err.println( + "FAILED MUZZLE VALIDATION: " + instrumenter.getClass().getName() + " mismatches:"); + for (Reference.Mismatch mismatch : mismatches) { + System.err.println("-- " + mismatch); + } + throw new RuntimeException("Instrumentation failed Muzzle validation"); + } + } finally { + if (null != m) { + m.setAccessible(false); + } + } + } + // run helper injector on all instrumenters + for (Instrumenter instrumenter : + ServiceLoader.load(Instrumenter.class, MuzzleGradlePlugin.class.getClassLoader())) { + if (instrumenter.getClass().getName().endsWith("TraceConfigInstrumentation")) { + // TraceConfigInstrumentation doesn't do muzzle checks + // check on TracerClassInstrumentation instead + instrumenter = + (Instrumenter) + MuzzleGradlePlugin.class + .getClassLoader() + .loadClass(instrumenter.getClass().getName() + "$TracerClassInstrumentation") + .getDeclaredConstructor() + .newInstance(); + } + try { + // Ratpack injects the scope manager as a helper. + // This is likely a bug, but we'll grandfather it out of the helper checks for now. + if (!instrumenter.getClass().getName().contains("Ratpack")) { + // verify helper injector works + final String[] helperClassNames = instrumenter.helperClassNames(); + if (helperClassNames.length > 0) { + new HelperInjector(helperClassNames).transform(null, null, cl, null); + } + } + } catch (Exception e) { + System.err.println( + "FAILED HELPER INJECTION. Are Helpers being injected in the correct order?"); + throw e; + } + } + } + + private MuzzleVersionScanPlugin() {} +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java index fd662158f7..9fb91d0d51 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVisitor.java @@ -15,11 +15,7 @@ import net.bytebuddy.implementation.Implementation; import net.bytebuddy.jar.asm.*; import net.bytebuddy.pool.TypePool; -/** - * TODO: update doc Visit a class and add: 1) a private instrumenationMuzzle field and getter AND 2) - * logic to the end of the instrumentation transformer to assert classpath is safe to apply - * instrumentation to. - */ +/** Visit a class and add: a private instrumenationMuzzle field and getter */ public class MuzzleVisitor implements AsmVisitorWrapper { @Override public int mergeWriter(int flags) { @@ -104,7 +100,7 @@ public class MuzzleVisitor implements AsmVisitorWrapper { if (!referenceSources.contains(adviceClass)) { referenceSources.add(adviceClass); for (Map.Entry entry : - AdviceReferenceVisitor.createReferencesFrom( + ReferenceCreator.createReferencesFrom( adviceClass, ReferenceMatcher.class.getClassLoader()) .entrySet()) { if (references.containsKey(entry.getKey())) { diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java index d44bf46247..f8a2184a8f 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java @@ -5,7 +5,7 @@ import static datadog.trace.agent.tooling.ClassLoaderMatcher.BOOTSTRAP_CLASSLOAD import datadog.trace.agent.tooling.Utils; import java.util.*; -/** An immutable reference to a single class file. */ +/** An immutable reference to a jvm class. */ public class Reference { private final Set sources; private final String className; diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/AdviceReferenceVisitor.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceCreator.java similarity index 92% rename from dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/AdviceReferenceVisitor.java rename to dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceCreator.java index c018a98c4b..12ce1fe4f7 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/AdviceReferenceVisitor.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceCreator.java @@ -7,11 +7,63 @@ import java.util.*; import net.bytebuddy.jar.asm.*; /** Visit a class and collect all references made by the visited class. */ -public class AdviceReferenceVisitor extends ClassVisitor { +public class ReferenceCreator extends ClassVisitor { + /** + * Generate all references reachable from a given class. + * + * @param entryPointClassName Starting point for generating references. + * @param loader Classloader used to read class bytes. + * @return Map of [referenceClassName -> Reference] + */ + public static Map createReferencesFrom( + String entryPointClassName, ClassLoader loader) { + final Set visitedSources = new HashSet<>(); + final Map references = new HashMap<>(); + + final Queue instrumentationQueue = new ArrayDeque<>(); + instrumentationQueue.add(entryPointClassName); + + while (!instrumentationQueue.isEmpty()) { + final String className = instrumentationQueue.remove(); + visitedSources.add(className); + try { + final InputStream in = loader.getResourceAsStream(Utils.getResourceName(className)); + try { + final ReferenceCreator cv = new ReferenceCreator(null); + final ClassReader reader = new ClassReader(in); + reader.accept(cv, ClassReader.SKIP_FRAMES); + + Map instrumentationReferences = cv.getReferences(); + for (Map.Entry entry : instrumentationReferences.entrySet()) { + // Don't generate references created outside of the datadog instrumentation package. + if (!visitedSources.contains(entry.getKey()) + && entry.getKey().startsWith("datadog.trace.instrumentation.")) { + instrumentationQueue.add(entry.getKey()); + } + if (references.containsKey(entry.getKey())) { + references.put( + entry.getKey(), references.get(entry.getKey()).merge(entry.getValue())); + } else { + references.put(entry.getKey(), entry.getValue()); + } + } + + } finally { + if (in != null) { + in.close(); + } + } + } catch (IOException ioe) { + throw new IllegalStateException(ioe); + } + } + return references; + } + private Map references = new HashMap<>(); private String refSourceClassName; - public AdviceReferenceVisitor(ClassVisitor classVisitor) { + private ReferenceCreator(ClassVisitor classVisitor) { super(Opcodes.ASM6, classVisitor); } @@ -51,7 +103,6 @@ public class AdviceReferenceVisitor extends ClassVisitor { } private class AdviceReferenceMethodVisitor extends MethodVisitor { - private boolean isAdviceMethod = false; private int currentLineNumber = -1; public AdviceReferenceMethodVisitor(MethodVisitor methodVisitor) { @@ -75,56 +126,4 @@ public class AdviceReferenceVisitor extends ClassVisitor { new Reference.Builder(owner).withSource(refSourceClassName, currentLineNumber).build()); } } - - /** - * Generate all references reachable from a given class. - * - * @param entryPointClassName Starting point for generating references. - * @param loader Classloader used to read class bytes. - * @return Map of [referenceClassName -> Reference] - */ - public static Map createReferencesFrom( - String entryPointClassName, ClassLoader loader) { - final Set visitedSources = new HashSet<>(); - final Map references = new HashMap<>(); - - final Queue instrumentationQueue = new ArrayDeque<>(); - instrumentationQueue.add(entryPointClassName); - - while (!instrumentationQueue.isEmpty()) { - final String className = instrumentationQueue.remove(); - visitedSources.add(className); - try { - final InputStream in = loader.getResourceAsStream(Utils.getResourceName(className)); - try { - final AdviceReferenceVisitor cv = new AdviceReferenceVisitor(null); - final ClassReader reader = new ClassReader(in); - reader.accept(cv, ClassReader.SKIP_FRAMES); - - Map instrumentationReferences = cv.getReferences(); - for (Map.Entry entry : instrumentationReferences.entrySet()) { - // TODO: expose config instead of hardcoding datadog instrumentation namespace - if (!visitedSources.contains(entry.getKey()) - && entry.getKey().startsWith("datadog.trace.instrumentation.")) { - instrumentationQueue.add(entry.getKey()); - } - if (references.containsKey(entry.getKey())) { - references.put( - entry.getKey(), references.get(entry.getKey()).merge(entry.getValue())); - } else { - references.put(entry.getKey(), entry.getValue()); - } - } - - } finally { - if (in != null) { - in.close(); - } - } - } catch (IOException ioe) { - throw new IllegalStateException(ioe); - } - } - return references; - } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java index 17fde8b047..cfcbdf4895 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/ReferenceMatcher.java @@ -3,19 +3,12 @@ package datadog.trace.agent.tooling.muzzle; import static net.bytebuddy.dynamic.loading.ClassLoadingStrategy.BOOTSTRAP_LOADER; import datadog.trace.agent.tooling.Utils; -import java.security.ProtectionDomain; import java.util.*; import lombok.extern.slf4j.Slf4j; -import net.bytebuddy.agent.builder.AgentBuilder; -import net.bytebuddy.description.type.TypeDescription; -import net.bytebuddy.utility.JavaModule; -/** - * A bytebuddy matcher that matches if expected references (classes, fields, methods, visibility) - * are present on the classpath. - */ +/** Matches a set of references against a classloader. */ @Slf4j -public class ReferenceMatcher implements AgentBuilder.RawMatcher { +public class ReferenceMatcher { private final Map> mismatchCache = Collections.synchronizedMap(new WeakHashMap>()); private final Reference[] references; @@ -30,16 +23,6 @@ public class ReferenceMatcher implements AgentBuilder.RawMatcher { this.helperClassNames = new HashSet<>(Arrays.asList(helperClassNames)); } - @Override - public boolean matches( - TypeDescription typeDescription, - ClassLoader classLoader, - JavaModule module, - Class classBeingRedefined, - ProtectionDomain protectionDomain) { - return matches(classLoader); - } - /** * @param loader Classloader to validate against (or null for bootstrap) * @return true if all references match the classpath of loader diff --git a/dd-java-agent/testing/src/test/groovy/muzzle/AdviceReferenceVisitorTest.groovy b/dd-java-agent/testing/src/test/groovy/muzzle/AdviceReferenceVisitorTest.groovy index 49e17dd6b3..e05ca43c07 100644 --- a/dd-java-agent/testing/src/test/groovy/muzzle/AdviceReferenceVisitorTest.groovy +++ b/dd-java-agent/testing/src/test/groovy/muzzle/AdviceReferenceVisitorTest.groovy @@ -2,7 +2,7 @@ package muzzle import datadog.trace.agent.test.AgentTestRunner import datadog.trace.agent.test.TestUtils -import datadog.trace.agent.tooling.muzzle.AdviceReferenceVisitor +import datadog.trace.agent.tooling.muzzle.ReferenceCreator import datadog.trace.agent.tooling.muzzle.ReferenceMatcher import datadog.trace.agent.tooling.muzzle.Reference @@ -10,7 +10,7 @@ class AdviceReferenceVisitorTest extends AgentTestRunner { def "methods body references"() { setup: - Map references = AdviceReferenceVisitor.createReferencesFrom(AdviceClass.getName(), this.getClass().getClassLoader()) + Map references = ReferenceCreator.createReferencesFrom(AdviceClass.getName(), this.getClass().getClassLoader()) expect: references.get('java.lang.Object') != null @@ -22,7 +22,7 @@ class AdviceReferenceVisitorTest extends AgentTestRunner { def "match safe classpaths"() { setup: - Reference[] refs = AdviceReferenceVisitor.createReferencesFrom(AdviceClass.getName(), this.getClass().getClassLoader()).values().toArray(new Reference[0]) + Reference[] refs = ReferenceCreator.createReferencesFrom(AdviceClass.getName(), this.getClass().getClassLoader()).values().toArray(new Reference[0]) ReferenceMatcher refMatcher = new ReferenceMatcher(refs) ClassLoader safeClassloader = new URLClassLoader([TestUtils.createJarWithClasses(AdviceClass$A, AdviceClass$SomeInterface, From 23d0439b128f39fa9ba0a7f11de1d6fa00fbe4bd Mon Sep 17 00:00:00 2001 From: Andrew Kent Date: Thu, 12 Jul 2018 17:25:17 -0400 Subject: [PATCH 11/11] Remove muzzle dead code, doc cleanup, and better logging --- buildSrc/src/main/groovy/MuzzlePlugin.groovy | 2 +- .../trace/agent/tooling/Instrumenter.java | 7 ++- .../tooling/muzzle/MuzzleGradlePlugin.java | 63 ------------------- .../ApacheHttpClientInstrumentation.java | 2 +- 4 files changed, 7 insertions(+), 67 deletions(-) diff --git a/buildSrc/src/main/groovy/MuzzlePlugin.groovy b/buildSrc/src/main/groovy/MuzzlePlugin.groovy index 3e4a502ef6..3c85179a39 100644 --- a/buildSrc/src/main/groovy/MuzzlePlugin.groovy +++ b/buildSrc/src/main/groovy/MuzzlePlugin.groovy @@ -4,7 +4,7 @@ import org.gradle.api.Project import java.lang.reflect.Method /** - * POC muzzle task plugin which runs muzzle validation against an instrumentation's compile-time dependencies. + * muzzle task plugin which runs muzzle validation against an instrumentation's compile-time dependencies. * *

TODO: merge this with version scan */ diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java index 3e3be40726..06f65dac34 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java @@ -90,15 +90,16 @@ public interface Instrumenter { JavaModule module, Class classBeingRedefined, ProtectionDomain protectionDomain) { - // Optimization: calling getMuzzleReferenceMatcher() inside this method prevents unnecessary loading of muzzle references during agentBuilder setup. + // Optimization: calling getInstrumentationMuzzle() inside this method prevents unnecessary loading of muzzle references during agentBuilder setup. final ReferenceMatcher muzzle = getInstrumentationMuzzle(); if (null != muzzle) { List mismatches = muzzle.getMismatchedReferenceSources(classLoader); if (mismatches.size() > 0) { log.debug( - "Instrumentation muzzled: {} on {}", + "Instrumentation muzzled: {} -- {} on {}", instrumentationPrimaryName, + this.getClass().getName(), classLoader); } for (Reference.Mismatch mismatch : mismatches) { @@ -149,6 +150,8 @@ public interface Instrumenter { return getConfigEnabled("dd.integrations.enabled", true); } + // TODO: move common config helpers to Utils + public static String getPropOrEnv(final String name) { return System.getProperty(name, System.getenv(propToEnvName(name))); } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java index b2703ea16c..ebaa6b7dfb 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleGradlePlugin.java @@ -1,19 +1,10 @@ package datadog.trace.agent.tooling.muzzle; import datadog.trace.agent.tooling.Instrumenter; -import net.bytebuddy.asm.AsmVisitorWrapper; import net.bytebuddy.build.Plugin; -import net.bytebuddy.description.field.FieldDescription; -import net.bytebuddy.description.field.FieldList; -import net.bytebuddy.description.method.MethodList; import net.bytebuddy.description.type.TypeDefinition; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.dynamic.DynamicType.Builder; -import net.bytebuddy.implementation.Implementation; -import net.bytebuddy.jar.asm.ClassVisitor; -import net.bytebuddy.jar.asm.MethodVisitor; -import net.bytebuddy.jar.asm.Opcodes; -import net.bytebuddy.pool.TypePool; /** Bytebuddy gradle plugin which creates muzzle-references at compile time. */ public class MuzzleGradlePlugin implements Plugin { @@ -52,58 +43,4 @@ public class MuzzleGradlePlugin implements Plugin { return builder; } } - - private static class RemoveFinalFlagVisitor implements AsmVisitorWrapper { - final String methodName; - - public RemoveFinalFlagVisitor(String methodName) { - this.methodName = methodName; - } - - @Override - public int mergeWriter(int flags) { - return flags; - } - - @Override - public int mergeReader(int flags) { - return flags; - } - - @Override - public ClassVisitor wrap( - TypeDescription instrumentedType, - ClassVisitor classVisitor, - Implementation.Context implementationContext, - TypePool typePool, - FieldList fields, - MethodList methods, - int writerFlags, - int readerFlags) { - return new Visitor(classVisitor); - } - - private class Visitor extends ClassVisitor { - public Visitor(ClassVisitor cv) { - super(Opcodes.ASM6, cv); - } - - @Override - public MethodVisitor visitMethod( - final int access, - final String name, - final String descriptor, - final String signature, - final String[] exceptions) { - MethodVisitor methodVisitor = - super.visitMethod(access, name, descriptor, signature, exceptions); - if (name.equals(methodName) && (access & Opcodes.ACC_FINAL) != 0) { - return super.visitMethod( - access ^ Opcodes.ACC_FINAL, name, descriptor, signature, exceptions); - } else { - return super.visitMethod(access, name, descriptor, signature, exceptions); - } - } - } - } } diff --git a/dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java b/dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java index 319f7ea30d..1efe58cd48 100644 --- a/dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java +++ b/dd-java-agent/instrumentation/apache-httpclient-4.3/src/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientInstrumentation.java @@ -45,7 +45,7 @@ public class ApacheHttpClientInstrumentation extends Instrumenter.Default { public String[] helperClassNames() { return new String[] { "datadog.trace.instrumentation.apachehttpclient.DDTracingClientExec", - "datadog.trace.instrumentation.apachehttpclient.DDTracingClientExec$HttpHeadersInjectAdapter", + "datadog.trace.instrumentation.apachehttpclient.DDTracingClientExec$HttpHeadersInjectAdapter" }; }