From 4d1e4defd462e5d6732bf703c39f36dd56055d5f Mon Sep 17 00:00:00 2001 From: Sarah Chen Date: Thu, 29 Jan 2026 16:35:37 -0500 Subject: [PATCH 1/5] Avoid cloning final fields --- .../java/datadog/trace/util/UnsafeUtils.java | 8 ++-- .../datadog/trace/util/UnsafeUtilsTest.groovy | 42 +++++++++++++++---- 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java b/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java index 06cd5f832a0..5261a04d56d 100644 --- a/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java +++ b/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java @@ -1,6 +1,5 @@ package datadog.trace.util; -import de.thetaphi.forbiddenapis.SuppressForbidden; import java.lang.reflect.Field; import java.lang.reflect.Modifier; import org.slf4j.Logger; @@ -58,11 +57,12 @@ public static T tryShallowClone(T original) { } } - // TODO: JEP 500 - avoid mutating final fields - @SuppressForbidden private static void cloneFields(Class clazz, Object original, Object clone) throws Exception { for (Field field : clazz.getDeclaredFields()) { - if ((field.getModifiers() & Modifier.STATIC) != 0) { + if ((field.getModifiers() & Modifier.FINAL) != 0) { + log.debug( + "Skipping cloning final field {}. Final fields cannot be mutated. See JEP 500 for more details.", + field.getName()); continue; } field.setAccessible(true); diff --git a/internal-api/src/test/groovy/datadog/trace/util/UnsafeUtilsTest.groovy b/internal-api/src/test/groovy/datadog/trace/util/UnsafeUtilsTest.groovy index 7bfa3976f83..8a75618e2a3 100644 --- a/internal-api/src/test/groovy/datadog/trace/util/UnsafeUtilsTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/util/UnsafeUtilsTest.groovy @@ -4,7 +4,7 @@ import spock.lang.Specification class UnsafeUtilsTest extends Specification { - def "test try shallow clone"() { + def "test try shallow clone does not clone final fields"() { given: def inner = new MyClass("a", false, [], "b", 2, null, null) def instance = new MyClass("aaa", true, [ 4, 5, 6, ], "ddd", 1, new int[] { @@ -16,13 +16,27 @@ class UnsafeUtilsTest extends Specification { then: clone !== instance - clone.a === instance.a - clone.b == instance.b - clone.c === instance.c - clone.d === instance.d - clone.e == instance.e - clone.f === instance.f - clone.g === instance.g + clone.a == null + clone.b == false + clone.c == null + clone.d == null + clone.e == 0 + clone.f == null + clone.g == null + } + + def "test try shallow clone clones non-final fields"() { + given: + def instance = new MyNonFinalClass("a", 1, [2, 3, 4]) + + when: + def clone = UnsafeUtils.tryShallowClone(instance) + + then: + clone !== instance + clone.h == instance.h + clone.j == instance.j + clone.k === instance.k } private static class MyParentClass { @@ -65,4 +79,16 @@ class UnsafeUtilsTest extends Specification { this.g = g } } + + private static class MyNonFinalClass { + private String h + private int j + private List k + + MyNonFinalClass(String h, int j, List k) { + this.h = h + this.j = j + this.k = k + } + } } From f66b1b55ebe7d467201b03eb807f997e2d24cccb Mon Sep 17 00:00:00 2001 From: Sarah Chen Date: Thu, 29 Jan 2026 16:52:50 -0500 Subject: [PATCH 2/5] Oops still need to suppressforbidden --- .../src/main/java/datadog/trace/util/UnsafeUtils.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java b/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java index 5261a04d56d..5c2e50c90b3 100644 --- a/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java +++ b/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java @@ -1,5 +1,6 @@ package datadog.trace.util; +import de.thetaphi.forbiddenapis.SuppressForbidden; import java.lang.reflect.Field; import java.lang.reflect.Modifier; import org.slf4j.Logger; @@ -57,6 +58,10 @@ public static T tryShallowClone(T original) { } } + // Field::set() is forbidden because it may be used to mutate final fields, disallowed by + // https://openjdk.org/jeps/500. + // However, in this case we skip final fields, so it is safe. + @SuppressForbidden private static void cloneFields(Class clazz, Object original, Object clone) throws Exception { for (Field field : clazz.getDeclaredFields()) { if ((field.getModifiers() & Modifier.FINAL) != 0) { From 47184656aa0d65c27fca2294cf0f882b318be71a Mon Sep 17 00:00:00 2001 From: Sarah Chen Date: Fri, 30 Jan 2026 17:01:46 -0500 Subject: [PATCH 3/5] First attempt at nonfinal cloning --- internal-api/build.gradle.kts | 2 + .../java/datadog/trace/util/UnsafeUtils.java | 53 +++++++++++++++++-- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/internal-api/build.gradle.kts b/internal-api/build.gradle.kts index b78e5b38b14..8ad3716f97e 100644 --- a/internal-api/build.gradle.kts +++ b/internal-api/build.gradle.kts @@ -270,6 +270,8 @@ dependencies { // it contains annotations that are also present in the instrumented application classes api("com.datadoghq:dd-javac-plugin-client:0.2.2") + implementation(libs.bytebuddy) + testImplementation("org.snakeyaml:snakeyaml-engine:2.9") testImplementation(project(":utils:test-utils")) testImplementation(libs.bundles.junit5) diff --git a/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java b/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java index 5c2e50c90b3..0b4f983233a 100644 --- a/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java +++ b/internal-api/src/main/java/datadog/trace/util/UnsafeUtils.java @@ -1,8 +1,16 @@ package datadog.trace.util; +import static net.bytebuddy.matcher.ElementMatchers.isFinal; + import de.thetaphi.forbiddenapis.SuppressForbidden; import java.lang.reflect.Field; import java.lang.reflect.Modifier; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import net.bytebuddy.ByteBuddy; +import net.bytebuddy.asm.ModifierAdjustment; +import net.bytebuddy.description.modifier.FieldManifestation; +import net.bytebuddy.dynamic.DynamicType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import sun.misc.Unsafe; @@ -13,6 +21,8 @@ public abstract class UnsafeUtils { private static final Unsafe UNSAFE = getUnsafe(); + private static final Map> CLASS_CACHE = new ConcurrentHashMap<>(); + private static Unsafe getUnsafe() { try { Field f = Unsafe.class.getDeclaredField("theUnsafe"); @@ -37,7 +47,7 @@ private static Unsafe getUnsafe() { * @param Type of the object being cloned */ @SuppressWarnings("unchecked") - public static T tryShallowClone(T original) { + public static T originalTryShallowClone(T original) { if (UNSAFE == null) { log.debug("Unsafe is unavailable, {} will not be cloned", original); return original; @@ -58,16 +68,49 @@ public static T tryShallowClone(T original) { } } + public static T tryShallowClone(T original) { + if (UNSAFE == null) { + log.debug("Unsafe is unavailable, {} will not be cloned", original); + return original; + } + try { + Class clazz = original.getClass(); + if (!CLASS_CACHE.containsKey(clazz.getName())) { + CLASS_CACHE.put(clazz.getName(), createNonFinalSubclass(clazz)); + } + Class nonFinalSubclass = CLASS_CACHE.get(clazz.getName()); + + T clone = (T) UNSAFE.allocateInstance(nonFinalSubclass); + + while (clazz != Object.class) { + cloneFields(clazz, original, clone); + clazz = clazz.getSuperclass(); + } + return clone; + + } catch (Throwable t) { + log.debug("Error while cloning {}: {}", original, t); + t.printStackTrace(); + return original; + } + } + + private static Class createNonFinalSubclass(Class clazz) throws Exception { + DynamicType.Unloaded dynamicType = + new ByteBuddy() + .subclass(clazz) + .visit(new ModifierAdjustment().withFieldModifiers(isFinal(), FieldManifestation.PLAIN)) + .make(); + return dynamicType.load(clazz.getClassLoader()).getLoaded(); + } + // Field::set() is forbidden because it may be used to mutate final fields, disallowed by // https://openjdk.org/jeps/500. // However, in this case we skip final fields, so it is safe. @SuppressForbidden private static void cloneFields(Class clazz, Object original, Object clone) throws Exception { for (Field field : clazz.getDeclaredFields()) { - if ((field.getModifiers() & Modifier.FINAL) != 0) { - log.debug( - "Skipping cloning final field {}. Final fields cannot be mutated. See JEP 500 for more details.", - field.getName()); + if ((field.getModifiers() & Modifier.STATIC) != 0) { continue; } field.setAccessible(true); From 8e402c03e020a2243e2ed646876e5b90bd1e87e5 Mon Sep 17 00:00:00 2001 From: Sarah Chen Date: Mon, 2 Feb 2026 09:35:12 -0500 Subject: [PATCH 4/5] Replace shallowClone with new instance of class --- .../execution/TestDescriptorHandle.java | 101 +++++++++++++++++- 1 file changed, 98 insertions(+), 3 deletions(-) diff --git a/dd-java-agent/instrumentation/junit/junit-5/junit-5.3/src/main/java/datadog/trace/instrumentation/junit5/execution/TestDescriptorHandle.java b/dd-java-agent/instrumentation/junit/junit-5/junit-5.3/src/main/java/datadog/trace/instrumentation/junit5/execution/TestDescriptorHandle.java index b89fc1113e4..00942cbf983 100644 --- a/dd-java-agent/instrumentation/junit/junit-5/junit-5.3/src/main/java/datadog/trace/instrumentation/junit5/execution/TestDescriptorHandle.java +++ b/dd-java-agent/instrumentation/junit/junit-5/junit-5.3/src/main/java/datadog/trace/instrumentation/junit5/execution/TestDescriptorHandle.java @@ -2,10 +2,14 @@ import datadog.trace.agent.tooling.muzzle.Reference; import datadog.trace.util.MethodHandles; -import datadog.trace.util.UnsafeUtils; import java.lang.invoke.MethodHandle; +import java.lang.reflect.Constructor; +import java.lang.reflect.Field; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; +import java.util.List; import java.util.Map; import org.junit.platform.commons.util.ClassLoaderUtils; import org.junit.platform.engine.TestDescriptor; @@ -40,7 +44,7 @@ public TestDescriptorHandle(TestDescriptor testDescriptor) { * Cloning has to be done before each test retry to * compensate for the state modifications. */ - this.testDescriptor = UnsafeUtils.tryShallowClone(testDescriptor); + this.testDescriptor = cloneIfNeeded(testDescriptor); } public TestDescriptor withIdSuffix(Map suffices) { @@ -49,8 +53,99 @@ public TestDescriptor withIdSuffix(Map suffices) { updatedId = updatedId.append(e.getKey(), String.valueOf(e.getValue())); } - TestDescriptor descriptorClone = UnsafeUtils.tryShallowClone(testDescriptor); + TestDescriptor descriptorClone = cloneIfNeeded(testDescriptor); METHOD_HANDLES.invoke(UNIQUE_ID_SETTER, descriptorClone, updatedId); return descriptorClone; } + + private TestDescriptor cloneIfNeeded(TestDescriptor original) { + Class clazz = original.getClass(); + String name = clazz.getName(); + + if (name.endsWith(".TestMethodTestDescriptor")) { + // No need to clone for TestMethodTestDescriptor because no states are modified + return original; + } + + if (name.endsWith(".TestTemplateInvocationTestDescriptor")) { + return copyConstructor( + original, + clazz, + "uniqueId", + "testClass", + "testMethod", + "invocationContext", + "index", + "configuration"); + } + + if (name.endsWith(".DynamicTestTestDescriptor")) { + return copyConstructor( + original, clazz, "uniqueId", "index", "dynamicTest", "source", "configuration"); + } + + throw new IllegalStateException("Unexpected class of TestDescriptor: " + name); + } + + private TestDescriptor copyConstructor(TestDescriptor original, Class clazz, String... names) { + try { + Map fields = getFields(clazz, original); + for (Constructor ctr : clazz.getDeclaredConstructors()) { + Object[] args = match(ctr, fields, original, names); + if (args != null) { + ctr.setAccessible(true); + return (TestDescriptor) ctr.newInstance(args); + } + } + throw new IllegalStateException( + "Failed to find appropriate constructor to clone: " + clazz.getName()); + } catch (Throwable e) { + throw new IllegalStateException("Failed to clone via constructor", e); + } + } + + private Map getFields(Class clazz, Object original) + throws IllegalAccessException { + Map fields = new HashMap<>(); + while (clazz != null && clazz != Object.class) { + for (Field field : clazz.getDeclaredFields()) { + field.setAccessible(true); + Object value = field.get(original); + + // Constructor may need `testClass` and `testMethod` parameters, but they are wrapped by + // `methodInfo`. + // Expand `methodInfo` to be used in constructor later. + if (field.getName().equals("methodInfo")) { + Map methodInfoFields = getFields(field.getType(), value); + fields.putAll(methodInfoFields); + } + + fields.put(field.getName(), value); + } + clazz = clazz.getSuperclass(); + } + return fields; + } + + private Object[] match( + Constructor ctr, Map fields, Object original, String... names) + throws IllegalAccessException { + int cnt = ctr.getParameterCount(); + + List args = new ArrayList<>(); + + for (String name : names) { + for (Map.Entry field : fields.entrySet()) { + String k = field.getKey(); + Object v = field.getValue(); + + if (k.equals(name)) { + args.add(v); + break; + } + } + } + + return args.size() == cnt ? args.toArray() : null; + } } From 83bde9a554b3cc29a7c8be81be25936da4bcddaa Mon Sep 17 00:00:00 2001 From: Sarah Chen Date: Mon, 2 Feb 2026 10:14:26 -0500 Subject: [PATCH 5/5] Allow zip64 for shadowjarconfig --- dd-java-agent/build.gradle | 1 + 1 file changed, 1 insertion(+) diff --git a/dd-java-agent/build.gradle b/dd-java-agent/build.gradle index 9885181de25..1d6a67ce200 100644 --- a/dd-java-agent/build.gradle +++ b/dd-java-agent/build.gradle @@ -67,6 +67,7 @@ dependencies { def generalShadowJarConfig(ShadowJar shadowJarTask) { shadowJarTask.with { mergeServiceFiles() + zip64 = true duplicatesStrategy = DuplicatesStrategy.FAIL