From 3417fd8940e1bc03da6570d77710e1db03522673 Mon Sep 17 00:00:00 2001 From: dougqh Date: Tue, 3 Mar 2020 13:48:16 -0500 Subject: [PATCH 1/5] Improving debuggability by requestingName This change adds a requestingName field to help in diagnosing the source of any injection failures. Usually, the requestingName is the simple class name of the Instrumenter; however, this change doesn't propagate the Instumeneter named through FieldBackedProvider. --- .../trace/agent/tooling/HelperInjector.java | 18 +++++++++++++----- .../trace/agent/tooling/Instrumenter.java | 4 +++- .../tooling/context/FieldBackedProvider.java | 4 +++- .../muzzle/MuzzleVersionScanPlugin.java | 4 +++- .../agent/test/HelperInjectionTest.groovy | 4 ++-- 5 files changed, 24 insertions(+), 10 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java index f74997b82a..96b3a49de5 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java @@ -33,6 +33,8 @@ public class HelperInjector implements Transformer { private static final ClassLoader BOOTSTRAP_CLASSLOADER_PLACEHOLDER = new SecureClassLoader(null) {}; + private final String requestingName; + private final Set helperClassNames; private final Map dynamicTypeMap = new LinkedHashMap<>(); @@ -48,21 +50,26 @@ public class HelperInjector implements Transformer { * provided. This is important if there is interdependency between helper classes that * requires them to be injected in a specific order. */ - public HelperInjector(final String... helperClassNames) { + public HelperInjector(final String requestingName, final String... helperClassNames) { + this.requestingName = requestingName; + this.helperClassNames = new LinkedHashSet<>(Arrays.asList(helperClassNames)); } - public HelperInjector(final Map helperMap) { + public HelperInjector(final String requestingName, final Map helperMap) { + this.requestingName = requestingName; + helperClassNames = helperMap.keySet(); dynamicTypeMap.putAll(helperMap); } - public static HelperInjector forDynamicTypes(final Collection> helpers) { + public static HelperInjector forDynamicTypes( + final String requestingName, final Collection> helpers) { final Map bytes = new HashMap<>(helpers.size()); for (final DynamicType.Unloaded helper : helpers) { bytes.put(helper.getTypeDescription().getName(), helper.getBytes()); } - return new HelperInjector(bytes); + return new HelperInjector(requestingName, bytes); } private Map getHelperMap() throws IOException { @@ -125,8 +132,9 @@ public class HelperInjector implements Transformer { : classLoader.getClass().getName(); log.error( - "Error preparing helpers for {}. Failed to inject helper classes into instance {} of type {}", + "Error preparing helpers while processing {} for {}. Failed to inject helper classes into instance {} of type {}", typeDescription, + requestingName, classLoader, classLoaderType, e); 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 4b2bd2b4f3..8e01b6af9b 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 @@ -106,7 +106,9 @@ public interface Instrumenter { AgentBuilder.Identified.Extendable agentBuilder) { final String[] helperClassNames = helperClassNames(); if (helperClassNames.length > 0) { - agentBuilder = agentBuilder.transform(new HelperInjector(helperClassNames)); + agentBuilder = + agentBuilder.transform( + new HelperInjector(this.getClass().getSimpleName(), helperClassNames)); } return agentBuilder; } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/context/FieldBackedProvider.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/context/FieldBackedProvider.java index 9c083bb29f..d6631ad03d 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/context/FieldBackedProvider.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/context/FieldBackedProvider.java @@ -322,8 +322,10 @@ public class FieldBackedProvider implements InstrumentationContextProvider { /** Get transformer that forces helper injection onto bootstrap classloader. */ private AgentBuilder.Transformer bootstrapHelperInjector( final Collection> helpers) { + // TODO: Better to pass through the context of the Instrumenter return new AgentBuilder.Transformer() { - final HelperInjector injector = HelperInjector.forDynamicTypes(helpers); + final HelperInjector injector = + HelperInjector.forDynamicTypes(this.getClass().getSimpleName(), helpers); @Override public DynamicType.Builder transform( 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 index d06f23aabd..4f47ab5f98 100644 --- 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 @@ -102,7 +102,9 @@ public class MuzzleVersionScanPlugin { // verify helper injector works final String[] helperClassNames = defaultInstrumenter.helperClassNames(); if (helperClassNames.length > 0) { - new HelperInjector(createHelperMap(defaultInstrumenter)) + new HelperInjector( + MuzzleVersionScanPlugin.class.getSimpleName(), + createHelperMap(defaultInstrumenter)) .transform(null, null, userClassLoader, null); } } catch (final Exception e) { diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/HelperInjectionTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/HelperInjectionTest.groovy index fa7c97ccf0..47b5687693 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/HelperInjectionTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/HelperInjectionTest.groovy @@ -24,7 +24,7 @@ class HelperInjectionTest extends DDSpecification { def "helpers injected to non-delegating classloader"() { setup: String helperClassName = HelperInjectionTest.getPackage().getName() + '.HelperClass' - HelperInjector injector = new HelperInjector(helperClassName) + HelperInjector injector = new HelperInjector("test", helperClassName) AtomicReference emptyLoader = new AtomicReference<>(new URLClassLoader(new URL[0], (ClassLoader) null)) when: @@ -56,7 +56,7 @@ class HelperInjectionTest extends DDSpecification { ByteBuddyAgent.install() AgentInstaller.installBytebuddyAgent(ByteBuddyAgent.getInstrumentation()) String helperClassName = HelperInjectionTest.getPackage().getName() + '.HelperClass' - HelperInjector injector = new HelperInjector(helperClassName) + HelperInjector injector = new HelperInjector("test", helperClassName) URLClassLoader bootstrapChild = new URLClassLoader(new URL[0], (ClassLoader) null) when: From c50899f1845643cab256d7f380c699d35356f627 Mon Sep 17 00:00:00 2001 From: dougqh Date: Tue, 3 Mar 2020 13:56:18 -0500 Subject: [PATCH 2/5] Creating helper methods for injection Create helper methods injectBootstrapClassLoader & injectClassLoader. The directory creation and retry logic that will be added to the handling of the bootstrap will complicated the code. To keep the code readable, I broke the bootstrap handling into its own method. To keep the level abstraction consistent, I did the same with injectClassLoader. --- .../trace/agent/tooling/HelperInjector.java | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java index 96b3a49de5..07da371718 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java @@ -29,6 +29,7 @@ import net.bytebuddy.utility.JavaModule; /** Injects instrumentation helper classes into the user's classloader. */ @Slf4j public class HelperInjector implements Transformer { + // Need this because we can't put null into the injectedClassLoaders map. private static final ClassLoader BOOTSTRAP_CLASSLOADER_PLACEHOLDER = new SecureClassLoader(null) {}; @@ -108,14 +109,9 @@ public class HelperInjector implements Transformer { final Map classnameToBytes = getHelperMap(); final Map> classes; if (classLoader == BOOTSTRAP_CLASSLOADER_PLACEHOLDER) { - classes = - ClassInjector.UsingInstrumentation.of( - new File(System.getProperty("java.io.tmpdir")), - ClassInjector.UsingInstrumentation.Target.BOOTSTRAP, - AgentInstaller.getInstrumentation()) - .injectRaw(classnameToBytes); + classes = injectBootstrapClassLoader(classnameToBytes); } else { - classes = new ClassInjector.UsingReflection(classLoader).injectRaw(classnameToBytes); + classes = injectClassLoader(classLoader, classnameToBytes); } // All datadog helper classes are in the unnamed module @@ -149,6 +145,19 @@ public class HelperInjector implements Transformer { return builder; } + private Map> injectBootstrapClassLoader(final Map classnameToBytes) { + return ClassInjector.UsingInstrumentation.of( + new File(System.getProperty("java.io.tmpdir")), + ClassInjector.UsingInstrumentation.Target.BOOTSTRAP, + AgentInstaller.getInstrumentation()) + .injectRaw(classnameToBytes); + } + + private Map> injectClassLoader( + final ClassLoader classLoader, final Map classnameToBytes) { + return new ClassInjector.UsingReflection(classLoader).injectRaw(classnameToBytes); + } + private void ensureModuleCanReadHelperModules(final JavaModule target) { if (JavaModule.isSupported() && target != JavaModule.UNSUPPORTED && target.isNamed()) { for (final WeakReference helperModuleReference : helperModules) { From d2ecce477e0d3ce497b6e3379a543dc6b447d223 Mon Sep 17 00:00:00 2001 From: dougqh Date: Tue, 3 Mar 2020 14:40:10 -0500 Subject: [PATCH 3/5] Switching to per-process temp dir Switching to create a per-process temp dir as protection against file name collisions. This is done by introduced TEMP_DIR which is calculated during class initialization. The current selection process generates a random directory name and sees if that name is availabe in java.io.tmpdir. If it is that name is selected; otherwise, another name is generated. If no unique name is found after 10 rounds of generation, the code falls back to the old behavior of writing directly to java.io.tmpdir. --- .../trace/agent/tooling/HelperInjector.java | 49 ++++++++++++++++--- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java index 07da371718..9efea182bc 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java @@ -25,10 +25,12 @@ import net.bytebuddy.dynamic.ClassFileLocator; import net.bytebuddy.dynamic.DynamicType; import net.bytebuddy.dynamic.loading.ClassInjector; import net.bytebuddy.utility.JavaModule; +import net.bytebuddy.utility.RandomString; /** Injects instrumentation helper classes into the user's classloader. */ @Slf4j public class HelperInjector implements Transformer { + private static final File TEMP_DIR = computeTempDir(); // Need this because we can't put null into the injectedClassLoaders map. private static final ClassLoader BOOTSTRAP_CLASSLOADER_PLACEHOLDER = @@ -145,16 +147,28 @@ public class HelperInjector implements Transformer { return builder; } - private Map> injectBootstrapClassLoader(final Map classnameToBytes) { - return ClassInjector.UsingInstrumentation.of( - new File(System.getProperty("java.io.tmpdir")), - ClassInjector.UsingInstrumentation.Target.BOOTSTRAP, - AgentInstaller.getInstrumentation()) - .injectRaw(classnameToBytes); + private Map> injectBootstrapClassLoader( + final Map classnameToBytes) { + if (!TEMP_DIR.exists()) { + TEMP_DIR.mkdir(); + } + try { + return ClassInjector.UsingInstrumentation.of( + TEMP_DIR, + ClassInjector.UsingInstrumentation.Target.BOOTSTRAP, + AgentInstaller.getInstrumentation()) + .injectRaw(classnameToBytes); + } finally { + if (TEMP_DIR.list().length == 0) { + if (!TEMP_DIR.delete()) { + TEMP_DIR.deleteOnExit(); + } + } + } } private Map> injectClassLoader( - final ClassLoader classLoader, final Map classnameToBytes) { + final ClassLoader classLoader, final Map classnameToBytes) { return new ClassInjector.UsingReflection(classLoader).injectRaw(classnameToBytes); } @@ -179,4 +193,25 @@ public class HelperInjector implements Transformer { } } } + + /* + * Tries to temp file naming collisions by creating a unique directory per + * process. Generates up to random names. If a no file exists with a + * generated name, settles on using that name. If name can be found falls + * back using java.io.tmpdir directly. + */ + private static final File computeTempDir() { + File rootTempDir = new File(System.getProperty("java.io.tmpdir")); + rootTempDir.mkdir(); + + RandomString randString = new RandomString(16); + for (int i = 0; i < 10; ++i) { + String dirName = "datadog-temp-jars-" + randString.nextString(); + File processTempDir = new File(rootTempDir, dirName); + if (!processTempDir.exists()) { + return processTempDir; + } + } + return rootTempDir; + } } From 4ad51dd9fa91c4d306be3d581132456c03b92933 Mon Sep 17 00:00:00 2001 From: dougqh Date: Tue, 3 Mar 2020 15:42:31 -0500 Subject: [PATCH 4/5] Introducing TempDir helper Introduces a TempDir helper class This was done to help handle an oversight in the prior per-process temp dir change. The fallback mode is to use the shared java.io.tmpdir directly. When using java.io.tmpdir, we don't want to be creating and destorying, since it may be shared. The new TempDir object is constructed knowing whether the TempDir is per-process or shared -- and the behavior of its prepare and cleanup method change accordingly. When using a per-process temp dir, the dir is creating and deleted regularly. When using a shared temp dir, those methods become nops. Creating TempDir also made it easy to avoid repeatedly calling deleteOnExit in the event that delete fails, so I made that change as well. --- .../trace/agent/tooling/HelperInjector.java | 77 +++++++++++++++---- 1 file changed, 64 insertions(+), 13 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java index 9efea182bc..b05ab866db 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java @@ -30,7 +30,7 @@ import net.bytebuddy.utility.RandomString; /** Injects instrumentation helper classes into the user's classloader. */ @Slf4j public class HelperInjector implements Transformer { - private static final File TEMP_DIR = computeTempDir(); + private static final TempDir TEMP_DIR = computeTempDir(); // Need this because we can't put null into the injectedClassLoaders map. private static final ClassLoader BOOTSTRAP_CLASSLOADER_PLACEHOLDER = @@ -149,21 +149,15 @@ public class HelperInjector implements Transformer { private Map> injectBootstrapClassLoader( final Map classnameToBytes) { - if (!TEMP_DIR.exists()) { - TEMP_DIR.mkdir(); - } + TEMP_DIR.prepare(); try { return ClassInjector.UsingInstrumentation.of( - TEMP_DIR, + TEMP_DIR.dir, ClassInjector.UsingInstrumentation.Target.BOOTSTRAP, AgentInstaller.getInstrumentation()) .injectRaw(classnameToBytes); } finally { - if (TEMP_DIR.list().length == 0) { - if (!TEMP_DIR.delete()) { - TEMP_DIR.deleteOnExit(); - } - } + TEMP_DIR.cleanup(); } } @@ -200,7 +194,7 @@ public class HelperInjector implements Transformer { * generated name, settles on using that name. If name can be found falls * back using java.io.tmpdir directly. */ - private static final File computeTempDir() { + private static final TempDir computeTempDir() { File rootTempDir = new File(System.getProperty("java.io.tmpdir")); rootTempDir.mkdir(); @@ -209,9 +203,66 @@ public class HelperInjector implements Transformer { String dirName = "datadog-temp-jars-" + randString.nextString(); File processTempDir = new File(rootTempDir, dirName); if (!processTempDir.exists()) { - return processTempDir; + return TempDir.makePerProcess(processTempDir); } } - return rootTempDir; + return TempDir.makeShared(rootTempDir); + } + + static final class TempDir { + static final TempDir makePerProcess(final File dir) { + return new TempDir(dir, true); + } + + static final TempDir makeShared(final File dir) { + return new TempDir(dir, false); + } + + public final File dir; + private final boolean perProcess; + private volatile boolean scheduledDelete = false; + + TempDir(final File dir, final boolean perProcess) { + this.dir = dir; + this.perProcess = perProcess; + } + + void prepare() { + // If shared, we're using java.io.tmpdir which should already exist + if (!perProcess) { + return; + } + + // If per process, need to create directory for this process + dir.mkdirs(); + } + + void cleanup() { + // If not per process, no directory clean-up + if (!perProcess) { + return; + } + + // If per process, clean-up the directory -- if it is empty + if (dir.list().length != 0) { + return; + } + + // Try to delete -- if the delete is successful, we're done + // Otherwise, schedule a delete -- see below... + boolean deleted = dir.delete(); + if (deleted) { + return; + } + + // Avoid needlessly repeatedly scheduling a deleteOnExit + // deleteOnExit does maintain a set, so extra calls are benign + // just consume some extra CPU and create contention + if (scheduledDelete) { + return; + } + dir.deleteOnExit(); + scheduledDelete = true; + } } } From 7a183e755e31029c7ec7a1feed148fbf0503526f Mon Sep 17 00:00:00 2001 From: dougqh Date: Wed, 4 Mar 2020 15:14:07 -0500 Subject: [PATCH 5/5] Switching to per-injection temp dir to avoid race Upon review, we realized that there was a race not just between processes but also with multiple threads in the same process. This race happens because of the effort to proactively clean-up directories. This left two choices... - assume per-process exclusivity -- and resolve the race with reference counting - move to a temp dir per injection This change uses the later strategy which is potentially more expensive but safest. --- .../trace/agent/tooling/HelperInjector.java | 100 ++++-------------- 1 file changed, 20 insertions(+), 80 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java index b05ab866db..7aa4cd249c 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java @@ -7,6 +7,7 @@ import datadog.trace.bootstrap.WeakMap; import java.io.File; import java.io.IOException; import java.lang.ref.WeakReference; +import java.nio.file.Files; import java.security.SecureClassLoader; import java.util.Arrays; import java.util.Collection; @@ -25,13 +26,10 @@ import net.bytebuddy.dynamic.ClassFileLocator; import net.bytebuddy.dynamic.DynamicType; import net.bytebuddy.dynamic.loading.ClassInjector; import net.bytebuddy.utility.JavaModule; -import net.bytebuddy.utility.RandomString; /** Injects instrumentation helper classes into the user's classloader. */ @Slf4j public class HelperInjector implements Transformer { - private static final TempDir TEMP_DIR = computeTempDir(); - // Need this because we can't put null into the injectedClassLoaders map. private static final ClassLoader BOOTSTRAP_CLASSLOADER_PLACEHOLDER = new SecureClassLoader(null) {}; @@ -148,16 +146,22 @@ public class HelperInjector implements Transformer { } private Map> injectBootstrapClassLoader( - final Map classnameToBytes) { - TEMP_DIR.prepare(); + final Map classnameToBytes) throws IOException { + // Mar 2020: Since we're proactively cleaning up tempDirs, we cannot share dirs per thread. + // If this proves expensive, we could do a per-process tempDir with + // a reference count -- but for now, starting simple. + + // Failures to create a tempDir are propagated as IOException and handled by transform + File tempDir = createTempDir(); try { return ClassInjector.UsingInstrumentation.of( - TEMP_DIR.dir, + tempDir, ClassInjector.UsingInstrumentation.Target.BOOTSTRAP, AgentInstaller.getInstrumentation()) .injectRaw(classnameToBytes); } finally { - TEMP_DIR.cleanup(); + // Delete fails silently + deleteTempDir(tempDir); } } @@ -188,81 +192,17 @@ public class HelperInjector implements Transformer { } } - /* - * Tries to temp file naming collisions by creating a unique directory per - * process. Generates up to random names. If a no file exists with a - * generated name, settles on using that name. If name can be found falls - * back using java.io.tmpdir directly. - */ - private static final TempDir computeTempDir() { - File rootTempDir = new File(System.getProperty("java.io.tmpdir")); - rootTempDir.mkdir(); - - RandomString randString = new RandomString(16); - for (int i = 0; i < 10; ++i) { - String dirName = "datadog-temp-jars-" + randString.nextString(); - File processTempDir = new File(rootTempDir, dirName); - if (!processTempDir.exists()) { - return TempDir.makePerProcess(processTempDir); - } - } - return TempDir.makeShared(rootTempDir); + private static final File createTempDir() throws IOException { + return Files.createTempDirectory("datadog-temp-jars").toFile(); } - static final class TempDir { - static final TempDir makePerProcess(final File dir) { - return new TempDir(dir, true); - } - - static final TempDir makeShared(final File dir) { - return new TempDir(dir, false); - } - - public final File dir; - private final boolean perProcess; - private volatile boolean scheduledDelete = false; - - TempDir(final File dir, final boolean perProcess) { - this.dir = dir; - this.perProcess = perProcess; - } - - void prepare() { - // If shared, we're using java.io.tmpdir which should already exist - if (!perProcess) { - return; - } - - // If per process, need to create directory for this process - dir.mkdirs(); - } - - void cleanup() { - // If not per process, no directory clean-up - if (!perProcess) { - return; - } - - // If per process, clean-up the directory -- if it is empty - if (dir.list().length != 0) { - return; - } - - // Try to delete -- if the delete is successful, we're done - // Otherwise, schedule a delete -- see below... - boolean deleted = dir.delete(); - if (deleted) { - return; - } - - // Avoid needlessly repeatedly scheduling a deleteOnExit - // deleteOnExit does maintain a set, so extra calls are benign - // just consume some extra CPU and create contention - if (scheduledDelete) { - return; - } - dir.deleteOnExit(); - scheduledDelete = true; + private static final void deleteTempDir(final File file) { + // Not using Files.delete for deleting the directory because failures + // create Exceptions which may prove expensive. Instead using the + // older File API which simply returns a boolean. + boolean deleted = file.delete(); + if (!deleted) { + file.deleteOnExit(); } } }