From 04fbb5085f358c2d83c8a2c384055e7678c1e032 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 24 Jan 2019 22:48:14 -0500 Subject: [PATCH] Improve OSGi class loader instrumentation It turns out that Eclipse's OSGi implementation has two problems: * It doesn't respect system properties by default * It has tricky classloader implementation that loads bootstrap classes from the classloader-has-delegation-to-bootstrap check but doesn't load bootstrap classes from 'normal' code. This should address second problem and make Eclipse's OSGi implementation 'safe' by default. --- .../agent/tooling/ClassLoaderMatcher.java | 6 ++ .../trace/agent/tooling/Constants.java | 45 ++++++++++ .../datadog/trace/agent/tooling/Utils.java | 33 +------ .../JBossClassloadingInstrumentation.java | 16 ++-- .../osgi-classloading.gradle | 7 +- .../osgi/OSGIClassloadingInstrumentation.java | 87 ++++++++++++++----- .../test/groovy/OSGIClassloadingTest.groovy | 25 +++++- .../agent/test/IntegrationTestUtils.java | 6 +- .../test/groovy/AgentTestRunnerTest.groovy | 8 +- 9 files changed, 161 insertions(+), 72 deletions(-) create mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Constants.java diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java index 068916e882..a9c2124063 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java @@ -91,6 +91,12 @@ public class ClassLoaderMatcher { } } + /** + * TODO: this turns out to be useless with OSGi: {@code + * }org.eclipse.osgi.internal.loader.BundleLoader#isRequestFromVM} returns {@code true} when + * class loading is issued from this check and {@code false} for 'real' class loads. We should + * come up with some sort of hack to avoid this problem. + */ private boolean delegatesToBootstrap(final ClassLoader loader) { boolean delegates = true; if (!loadsExpectedClass(loader, GlobalTracer.class)) { diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Constants.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Constants.java new file mode 100644 index 0000000000..a07fef2347 --- /dev/null +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Constants.java @@ -0,0 +1,45 @@ +package datadog.trace.agent.tooling; + +/** + * Some useful constants. + * + *

Idea here is to keep this class safe to inject into client's class loader. + */ +public final class Constants { + + /** + * packages which will be loaded on the bootstrap classloader + * + *

Updates should be mirrored in TestUtils#BOOTSTRAP_PACKAGE_PREFIXES_COPY + */ + public static final String[] BOOTSTRAP_PACKAGE_PREFIXES = { + "io.opentracing", + "datadog.slf4j", + "datadog.trace.bootstrap", + "datadog.trace.api", + "datadog.trace.context" + }; + + // This is used in IntegrationTestUtils.java + public static final String[] AGENT_PACKAGE_PREFIXES = { + "datadog.trace.common", + "datadog.trace.agent", + "datadog.trace.instrumentation", + // guava + "com.google.auto", + "com.google.common", + "com.google.thirdparty.publicsuffix", + // WeakConcurrentMap + "com.blogspot.mydailyjava.weaklockfree", + // bytebuddy + "net.bytebuddy", + // OT contribs for dd trace resolver + "io.opentracing.contrib", + // jackson + "org.msgpack", + "com.fasterxml.jackson", + "org.yaml.snakeyaml", + }; + + private Constants() {} +} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java index 63a7258fd1..391dcde97e 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java @@ -10,39 +10,8 @@ import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDefinition; public class Utils { - /** - * packages which will be loaded on the bootstrap classloader - * - *

Updates should be mirrored in TestUtils#BOOTSTRAP_PACKAGE_PREFIXES_COPY - */ - public static final String[] BOOTSTRAP_PACKAGE_PREFIXES = { - "io.opentracing", - "datadog.slf4j", - "datadog.trace.bootstrap", - "datadog.trace.api", - "datadog.trace.context" - }; - - public static final String[] AGENT_PACKAGE_PREFIXES = { - "datadog.trace.common", - "datadog.trace.agent", - "datadog.trace.instrumentation", - // guava - "com.google.auto", - "com.google.common", - "com.google.thirdparty.publicsuffix", - // WeakConcurrentMap - "com.blogspot.mydailyjava.weaklockfree", - // bytebuddy - "net.bytebuddy", - // OT contribs for dd trace resolver - "io.opentracing.contrib", - // jackson - "org.msgpack", - "com.fasterxml.jackson", - "org.yaml.snakeyaml", - }; + // This is used in HelperInjectionTest.groovy private static Method findLoadedClassMethod = null; private static final BootstrapClassLoaderProxy unitTestBootstrapProxy = diff --git a/dd-java-agent/instrumentation/jboss-classloading/src/main/java/datadog/trace/instrumentation/jboss/JBossClassloadingInstrumentation.java b/dd-java-agent/instrumentation/jboss-classloading/src/main/java/datadog/trace/instrumentation/jboss/JBossClassloadingInstrumentation.java index 69ac42ea58..184726b899 100644 --- a/dd-java-agent/instrumentation/jboss-classloading/src/main/java/datadog/trace/instrumentation/jboss/JBossClassloadingInstrumentation.java +++ b/dd-java-agent/instrumentation/jboss-classloading/src/main/java/datadog/trace/instrumentation/jboss/JBossClassloadingInstrumentation.java @@ -3,8 +3,8 @@ package datadog.trace.instrumentation.jboss; import static net.bytebuddy.matcher.ElementMatchers.named; import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Constants; import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.agent.tooling.Utils; import java.security.ProtectionDomain; import java.util.Collections; import java.util.Map; @@ -32,18 +32,18 @@ public final class JBossClassloadingInstrumentation extends Instrumenter.Default final Class classBeingRedefined, final ProtectionDomain protectionDomain) { // Set the system prop to tell jboss to delegate classloads for datadog bootstrap classes - final StringBuilder ddPrefixes = new StringBuilder(""); - for (int i = 0; i < Utils.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) { + final StringBuilder prefixes = new StringBuilder(""); + for (int i = 0; i < Constants.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) { if (i > 0) { - ddPrefixes.append(","); + prefixes.append(","); } - ddPrefixes.append(Utils.BOOTSTRAP_PACKAGE_PREFIXES[i]); + prefixes.append(Constants.BOOTSTRAP_PACKAGE_PREFIXES[i]); } final String existing = System.getProperty("jboss.modules.system.pkgs"); if (null == existing) { - System.setProperty("jboss.modules.system.pkgs", ddPrefixes.toString()); - } else if (!existing.contains(ddPrefixes)) { - System.setProperty("jboss.modules.system.pkgs", existing + "," + ddPrefixes.toString()); + System.setProperty("jboss.modules.system.pkgs", prefixes.toString()); + } else if (!existing.contains(prefixes)) { + System.setProperty("jboss.modules.system.pkgs", existing + "," + prefixes.toString()); } } diff --git a/dd-java-agent/instrumentation/osgi-classloading/osgi-classloading.gradle b/dd-java-agent/instrumentation/osgi-classloading/osgi-classloading.gradle index 18a452c8cc..d5cfd36202 100644 --- a/dd-java-agent/instrumentation/osgi-classloading/osgi-classloading.gradle +++ b/dd-java-agent/instrumentation/osgi-classloading/osgi-classloading.gradle @@ -6,6 +6,11 @@ dependencies { annotationProcessor deps.autoservice implementation deps.autoservice - testCompile group: 'org.osgi', name: 'org.osgi.core', version: '4.0.0' + // TODO: we should separate core and Eclipse tests at some point, + // but right now core-specific tests are quite dump and are run with + // core version provided by Eclipse implementation. + //testCompile group: 'org.osgi', name: 'org.osgi.core', version: '4.0.0' + testCompile group: 'org.eclipse.platform', name: 'org.eclipse.osgi', version: '3.13.200' + testCompile project(':dd-java-agent:testing') } diff --git a/dd-java-agent/instrumentation/osgi-classloading/src/main/java/datadog/trace/instrumentation/osgi/OSGIClassloadingInstrumentation.java b/dd-java-agent/instrumentation/osgi-classloading/src/main/java/datadog/trace/instrumentation/osgi/OSGIClassloadingInstrumentation.java index 0fe29f5256..ffd2c3f4de 100644 --- a/dd-java-agent/instrumentation/osgi-classloading/src/main/java/datadog/trace/instrumentation/osgi/OSGIClassloadingInstrumentation.java +++ b/dd-java-agent/instrumentation/osgi-classloading/src/main/java/datadog/trace/instrumentation/osgi/OSGIClassloadingInstrumentation.java @@ -1,13 +1,17 @@ package datadog.trace.instrumentation.osgi; -import static java.util.Collections.emptyMap; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Constants; import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.agent.tooling.Utils; import java.security.ProtectionDomain; import java.util.Map; +import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -21,8 +25,15 @@ public final class OSGIClassloadingInstrumentation extends Instrumenter.Default @Override public ElementMatcher typeMatcher() { - // OSGI Bundle class loads the sys prop which defines bootstrap classes - return named("org.osgi.framework.Bundle"); + // OSGi Bundle class loads the system property which defines bootstrap classes + return named("org.osgi.framework.Bundle").or(named("org.eclipse.osgi.launch.EquinoxFactory")); + } + + @Override + public String[] helperClassNames() { + return new String[] { + Constants.class.getName(), OSGIClassloadingInstrumentation.class.getName() + "$Helper" + }; } @Override @@ -32,27 +43,59 @@ public final class OSGIClassloadingInstrumentation extends Instrumenter.Default final JavaModule module, final Class classBeingRedefined, final ProtectionDomain protectionDomain) { - // Set the system prop to tell osgi to delegate classloads for datadog bootstrap classes - final StringBuilder ddPrefixes = new StringBuilder(""); - for (int i = 0; i < Utils.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) { - if (i > 0) { - // must append twice. Once for exact package and wildcard for child packages - ddPrefixes.append(","); - } - ddPrefixes.append(Utils.BOOTSTRAP_PACKAGE_PREFIXES[i]).append(".*,"); - ddPrefixes.append(Utils.BOOTSTRAP_PACKAGE_PREFIXES[i]); - } - final String existing = System.getProperty("org.osgi.framework.bootdelegation"); - if (null == existing) { - System.setProperty("org.osgi.framework.bootdelegation", ddPrefixes.toString()); - } else if (!existing.contains(ddPrefixes)) { - System.setProperty( - "org.osgi.framework.bootdelegation", existing + "," + ddPrefixes.toString()); - } + // Set the system prop to tell OSGi to delegate classloads for datadog bootstrap classes + System.setProperty( + Helper.PROPERTY_KEY, Helper.getNewValue(System.getProperty(Helper.PROPERTY_KEY))); } @Override public Map, String> transformers() { - return emptyMap(); + return singletonMap( + isMethod().and(isPublic()).and(named("newFramework")).and(takesArgument(0, Map.class)), + EquinoxFactoryAdvice.class.getName()); + } + + /** + * Sometimes OSGi doesn't read configuration from system properties. Handle this case for {@code + * EquinoxFactory}. + */ + public static class EquinoxFactoryAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void methodEnter(@Advice.Argument(0) final Map configuration) { + if (configuration != null) { + configuration.put( + Helper.PROPERTY_KEY, Helper.getNewValue(configuration.get(Helper.PROPERTY_KEY))); + } + } + } + + public static class Helper { + + public static final String PROPERTY_KEY = "org.osgi.framework.bootdelegation"; + public static final String PROPERTY_VALUE; + + static { + // Set the config option to tell osgi to delegate classloads for datadog bootstrap classes + final StringBuilder prefixes = new StringBuilder(""); + for (int i = 0; i < Constants.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) { + if (i > 0) { + // must append twice. Once for exact package and wildcard for child packages + prefixes.append(","); + } + prefixes.append(Constants.BOOTSTRAP_PACKAGE_PREFIXES[i]).append(".*,"); + prefixes.append(Constants.BOOTSTRAP_PACKAGE_PREFIXES[i]); + } + PROPERTY_VALUE = prefixes.toString(); + } + + public static String getNewValue(final String existingValue) { + if (null != existingValue + && !"".equals(existingValue) + && !existingValue.contains(PROPERTY_VALUE)) { + return existingValue + "," + PROPERTY_VALUE; + } else { + return PROPERTY_VALUE; + } + } } } diff --git a/dd-java-agent/instrumentation/osgi-classloading/src/test/groovy/OSGIClassloadingTest.groovy b/dd-java-agent/instrumentation/osgi-classloading/src/test/groovy/OSGIClassloadingTest.groovy index 8d3c4de5f3..37cf7a4ec9 100644 --- a/dd-java-agent/instrumentation/osgi-classloading/src/test/groovy/OSGIClassloadingTest.groovy +++ b/dd-java-agent/instrumentation/osgi-classloading/src/test/groovy/OSGIClassloadingTest.groovy @@ -1,11 +1,32 @@ import datadog.trace.agent.test.AgentTestRunner +import org.eclipse.osgi.launch.EquinoxFactory +import org.junit.Rule +import org.junit.contrib.java.lang.system.RestoreSystemProperties +import org.osgi.framework.launch.Framework +import org.osgi.framework.launch.FrameworkFactory class OSGIClassloadingTest extends AgentTestRunner { + + @Rule + public final RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties() + def "delegation property set on module load"() { - setup: + when: org.osgi.framework.Bundle.getName() - expect: + then: System.getProperty("org.osgi.framework.bootdelegation") == "io.opentracing.*,io.opentracing,datadog.slf4j.*,datadog.slf4j,datadog.trace.bootstrap.*,datadog.trace.bootstrap,datadog.trace.api.*,datadog.trace.api,datadog.trace.context.*,datadog.trace.context" } + + def "test Eclipse OSGi framework factory"() { + setup: + def config = ["osgi.support.class.certificate": "false"] + FrameworkFactory factory = new EquinoxFactory() + + when: + Framework framework = factory.newFramework(config) + + then: + framework != null + } } diff --git a/dd-java-agent/src/test/java/datadog/trace/agent/test/IntegrationTestUtils.java b/dd-java-agent/src/test/java/datadog/trace/agent/test/IntegrationTestUtils.java index 737e5e9304..83b48a0acb 100644 --- a/dd-java-agent/src/test/java/datadog/trace/agent/test/IntegrationTestUtils.java +++ b/dd-java-agent/src/test/java/datadog/trace/agent/test/IntegrationTestUtils.java @@ -99,7 +99,7 @@ public class IntegrationTestUtils { final Manifest manifest = new Manifest(); if (mainClassname != null) { - Attributes mainAttributes = manifest.getMainAttributes(); + final Attributes mainAttributes = manifest.getMainAttributes(); mainAttributes.put(Attributes.Name.MANIFEST_VERSION, "1.0"); mainAttributes.put(Attributes.Name.MAIN_CLASS, mainClassname); mainAttributes.put(new Attributes.Name("Premain-Class"), mainClassname); @@ -174,7 +174,7 @@ public class IntegrationTestUtils { public static String[] getBootstrapPackagePrefixes() throws Exception { final Field f = getAgentClassLoader() - .loadClass("datadog.trace.agent.tooling.Utils") + .loadClass("datadog.trace.agent.tooling.Constants") .getField("BOOTSTRAP_PACKAGE_PREFIXES"); return (String[]) f.get(null); } @@ -182,7 +182,7 @@ public class IntegrationTestUtils { public static String[] getAgentPackagePrefixes() throws Exception { final Field f = getAgentClassLoader() - .loadClass("datadog.trace.agent.tooling.Utils") + .loadClass("datadog.trace.agent.tooling.Constants") .getField("AGENT_PACKAGE_PREFIXES"); return (String[]) f.get(null); } diff --git a/dd-java-agent/testing/src/test/groovy/AgentTestRunnerTest.groovy b/dd-java-agent/testing/src/test/groovy/AgentTestRunnerTest.groovy index 33b68e49fa..f37e3733f9 100644 --- a/dd-java-agent/testing/src/test/groovy/AgentTestRunnerTest.groovy +++ b/dd-java-agent/testing/src/test/groovy/AgentTestRunnerTest.groovy @@ -2,7 +2,7 @@ import com.google.common.reflect.ClassPath import datadog.trace.agent.test.AgentTestRunner import datadog.trace.agent.test.SpockRunner import datadog.trace.agent.test.TestUtils -import datadog.trace.agent.tooling.Utils +import datadog.trace.agent.tooling.Constants import io.opentracing.Span import io.opentracing.Tracer import spock.lang.Shared @@ -29,15 +29,15 @@ class AgentTestRunnerTest extends AgentTestRunner { def "spock runner bootstrap prefixes correct for test setup"() { expect: - SpockRunner.BOOTSTRAP_PACKAGE_PREFIXES_COPY == Utils.BOOTSTRAP_PACKAGE_PREFIXES + SpockRunner.BOOTSTRAP_PACKAGE_PREFIXES_COPY == Constants.BOOTSTRAP_PACKAGE_PREFIXES } def "classpath setup"() { setup: final List bootstrapClassesIncorrectlyLoaded = [] for (ClassPath.ClassInfo info : TestUtils.getTestClasspath().getAllClasses()) { - for (int i = 0; i < Utils.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) { - if (info.getName().startsWith(Utils.BOOTSTRAP_PACKAGE_PREFIXES[i])) { + for (int i = 0; i < Constants.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) { + if (info.getName().startsWith(Constants.BOOTSTRAP_PACKAGE_PREFIXES[i])) { Class bootstrapClass = Class.forName(info.getName()) if (bootstrapClass.getClassLoader() != BOOTSTRAP_CLASSLOADER) { bootstrapClassesIncorrectlyLoaded.add(bootstrapClass)