Skip to content

Avoid temporary Jar file collisions #1285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dougqh merged 5 commits into from
Mar 6, 2020
Merged

Avoid temporary Jar file collisions #1285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3417fd8
Improving debuggability by requestingName
dougqh Mar 3, 2020
c50899f
Creating helper methods for injection
dougqh Mar 3, 2020
d2ecce4
Switching to per-process temp dir
dougqh Mar 3, 2020
4ad51dd
Introducing TempDir helper
dougqh Mar 3, 2020
7a183e7
Switching to per-injection temp dir to avoid race
dougqh Mar 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 55 additions & 12 deletions dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
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;
Expand All @@ -33,6 +34,8 @@ public class HelperInjector implements Transformer {
private static final ClassLoader BOOTSTRAP_CLASSLOADER_PLACEHOLDER =
new SecureClassLoader(null) {};

private final String requestingName;
Copy link
Copy Markdown
Contributor Author

@dougqh dougqh Mar 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field was added to improved our ability to debug problems when failures occur.


private final Set<String> helperClassNames;
private final Map<String, byte[]> dynamicTypeMap = new LinkedHashMap<>();

Expand All @@ -48,21 +51,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<String, byte[]> helperMap) {
public HelperInjector(final String requestingName, final Map<String, byte[]> helperMap) {
this.requestingName = requestingName;

helperClassNames = helperMap.keySet();
dynamicTypeMap.putAll(helperMap);
}

public static HelperInjector forDynamicTypes(final Collection<DynamicType.Unloaded<?>> helpers) {
public static HelperInjector forDynamicTypes(
final String requestingName, final Collection<DynamicType.Unloaded<?>> helpers) {
final Map<String, byte[]> 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<String, byte[]> getHelperMap() throws IOException {
Expand Down Expand Up @@ -101,14 +109,9 @@ public DynamicType.Builder<?> transform(
final Map<String, byte[]> classnameToBytes = getHelperMap();
final Map<String, Class<?>> classes;
if (classLoader == BOOTSTRAP_CLASSLOADER_PLACEHOLDER) {
classes =
Comment thread
dougqh marked this conversation as resolved.
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
Expand All @@ -125,8 +128,9 @@ public DynamicType.Builder<?> transform(
: 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);
Expand All @@ -141,6 +145,31 @@ public DynamicType.Builder<?> transform(
return builder;
}

private Map<String, Class<?>> injectBootstrapClassLoader(
final Map<String, byte[]> 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(
Copy link
Copy Markdown
Contributor Author

@dougqh dougqh Mar 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a further level of defense, we could do couple things here.

The ClassInjector internally constructs a RandomString which in turn uses a Random.
From inspection, two threads injecting at the same time are unlikely to conflict because the Random objects should have different seeds.

Java seeds Random objects not just with time, but also with another value that is changed for each Random object created.

If wanted to provide a stronger guarantee, we could reuse the UsingInstrumentation object from injection to the next which would also reuse the underlying RandomString and Random objects. However, before doing that, we need to double check that all the classes involved are thread-safe.

tempDir,
ClassInjector.UsingInstrumentation.Target.BOOTSTRAP,
AgentInstaller.getInstrumentation())
.injectRaw(classnameToBytes);
} finally {
// Delete fails silently
deleteTempDir(tempDir);
}
}

private Map<String, Class<?>> injectClassLoader(
final ClassLoader classLoader, final Map<String, byte[]> 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<Object> helperModuleReference : helperModules) {
Expand All @@ -162,4 +191,18 @@ private void ensureModuleCanReadHelperModules(final JavaModule target) {
}
}
}

private static final File createTempDir() throws IOException {
return Files.createTempDirectory("datadog-temp-jars").toFile();
}

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();
}
}
}
4 changes: 3 additions & 1 deletion dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,9 @@ private AgentBuilder.Identified.Extendable injectHelperClasses(
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;
}
Expand Down
4 changes: 3 additions & 1 deletion .../agent-tooling/src/main/java/datadog/trace/agent/tooling/context/FieldBackedProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,10 @@ private AgentBuilder.Identified.Extendable injectHelpersIntoBootstrapClassloader
/** Get transformer that forces helper injection onto bootstrap classloader. */
private AgentBuilder.Transformer bootstrapHelperInjector(
final Collection<DynamicType.Unloaded<?>> 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(
Expand Down
4 changes: 3 additions & 1 deletion ...ent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/MuzzleVersionScanPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@ public static void assertInstrumentationMuzzled(
// 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) {
Expand Down
4 changes: 2 additions & 2 deletions ...a-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/HelperInjectionTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<URLClassLoader> emptyLoader = new AtomicReference<>(new URLClassLoader(new URL[0], (ClassLoader) null))

when:
Expand Down Expand Up @@ -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:
Expand Down