Avoid temporary Jar file collisions by dougqh · Pull Request #1285 · DataDog/dd-trace-java

dougqh · 2020-03-03T21:51:23Z

This change aims to avoid JAR file collisions by creating a per-injection temp dir inside the usual java.io.tmpdir.

The HelperInjector selects a temporary directory using Files.createTempDirectory for each injection. This compensates for ByteBuddy's ClassInjector.UsingInstrumentation not using File.createTempFile which leads to occasional collisions.

This scheme was chosen in part to avoid modifying or recreating the logic in ClassInjector.UsingInstrumentation. While the logic of that class is not complicated, many of the helper classes it calls are not public.

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.

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.

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.

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.

dougqh · 2020-03-03T21:52:26Z

  private static final ClassLoader BOOTSTRAP_CLASSLOADER_PLACEHOLDER =
      new SecureClassLoader(null) {};

+  private final String requestingName;


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

dougqh · 2020-03-03T22:00:26Z

+      final Map<String, byte[]> classnameToBytes) {
+    TEMP_DIR.prepare();
+    try {
+      return ClassInjector.UsingInstrumentation.of(


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.

tylerbenson

A few minor questions...

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.

dougqh added 4 commits March 3, 2020 16:08

dougqh requested a review from a team as a code owner March 3, 2020 21:51

dougqh requested a review from randomanderson March 3, 2020 21:51

dougqh commented Mar 3, 2020

View reviewed changes

Comment thread dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java

dougqh commented Mar 3, 2020

View reviewed changes