From 0270d2217c17dc22e7c4d922bc8b444e1da74e7b Mon Sep 17 00:00:00 2001 From: datadog-bits <263423550+datadog-bits@users.noreply.github.com> Date: Wed, 29 Jul 2026 19:47:35 +0000 Subject: [PATCH 1/3] fix(debugger): do not call synchronized collection wrappers during serialization Calling size()/entrySet()/iterator() on java.util.Collections$Synchronized* wrappers acquires the wrapper mutex on the instrumented thread, which can deadlock the application. Treat them as unsafe so they are serialized as regular objects instead. --- .../debugger/util/WellKnownClasses.java | 25 ++++++----- .../agent/SnapshotSerializationTest.java | 44 +++++++++++++++++++ 2 files changed, 58 insertions(+), 11 deletions(-) diff --git a/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/util/WellKnownClasses.java b/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/util/WellKnownClasses.java index e80f9c0b836..d571510c0e6 100644 --- a/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/util/WellKnownClasses.java +++ b/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/util/WellKnownClasses.java @@ -205,6 +205,8 @@ public class WellKnownClasses { "org.agrona.collections." // Agrona ); + private static final String SYNCHRONIZED_WRAPPER_PREFIX = "java.util.Collections$Synchronized"; + /** * @return true if type is a final class and toString implementation is well known and side effect * free @@ -223,24 +225,25 @@ public static boolean isToStringSafe(String concreteType) { } /** - * @return true if collection implementation is safe to call (only in-memory) + * @return true if collection implementation is safe to call (only in-memory and lock free) */ public static boolean isSafe(Collection collection) { - String className = collection.getClass().getTypeName(); - for (String safePackage : SAFE_COLLECTION_PACKAGES) { - if (className.startsWith(safePackage)) { - return true; - } - } - return false; + return isSafe(collection.getClass().getTypeName(), SAFE_COLLECTION_PACKAGES); } /** - * @return true if map implementation is safe to call (only in-memory) + * @return true if map implementation is safe to call (only in-memory and lock free) */ public static boolean isSafe(Map map) { - String className = map.getClass().getTypeName(); - for (String safePackage : SAFE_MAP_PACKAGES) { + return isSafe(map.getClass().getTypeName(), SAFE_MAP_PACKAGES); + } + + private static boolean isSafe(String className, List safePackages) { + // synchronized wrappers lock on their mutex: calling them can deadlock the instrumented thread + if (className.startsWith(SYNCHRONIZED_WRAPPER_PREFIX)) { + return false; + } + for (String safePackage : safePackages) { if (className.startsWith(safePackage)) { return true; } diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SnapshotSerializationTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SnapshotSerializationTest.java index 0018c6d182b..84c55d38619 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SnapshotSerializationTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SnapshotSerializationTest.java @@ -55,6 +55,7 @@ import java.time.temporal.ChronoUnit; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.Date; import java.util.HashMap; import java.util.List; @@ -66,6 +67,8 @@ import java.util.Random; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.locks.LockSupport; import org.junit.jupiter.api.Assertions; @@ -755,6 +758,47 @@ public void map100() throws IOException { assertSize(locals, "strMap", "10"); } + @Test + public void synchronizedMapLockedByAnotherThread() throws Exception { + Map syncMap = Collections.synchronizedMap(new HashMap<>()); + syncMap.put("foo", "bar"); + CountDownLatch locked = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + Thread holder = + new Thread( + () -> { + synchronized (syncMap) { + locked.countDown(); + try { + release.await(); + } catch (InterruptedException ignored) { + } + } + }, + "mutex-holder"); + holder.start(); + assertTrue(locked.await(30, TimeUnit.SECONDS)); + try { + JsonAdapter adapter = createSnapshotAdapter(); + Snapshot snapshot = createSnapshot(); + CapturedContext context = new CapturedContext(); + context.addLocals( + new CapturedContext.CapturedValue[] { + capturedValueDepth("syncMap", syncMap.getClass().getTypeName(), syncMap, 1) + }); + snapshot.setExit(context); + String buffer = + CompletableFuture.supplyAsync(() -> adapter.toJson(snapshot)).get(30, TimeUnit.SECONDS); + Map local = (Map) getLocalsFromJson(buffer).get("syncMap"); + assertNull(local.get(ENTRIES)); + assertNull(local.get(SIZE)); + Assertions.assertNotNull(local.get(FIELDS)); + } finally { + release.countDown(); + holder.join(); + } + } + private Map doMapSize(int maxColSize) throws IOException { JsonAdapter adapter = createSnapshotAdapter(); Snapshot snapshot = createSnapshotForMapSize(maxColSize); From f1cb82af0b70effe38e3be035d28755d9a02cf48 Mon Sep 17 00:00:00 2001 From: Tyler Finethy Date: Wed, 29 Jul 2026 19:00:01 -0400 Subject: [PATCH 2/3] test(debugger): cover synchronized wrapper check Add a direct WellKnownClasses.isSafe test over the nine JDK Collections$Synchronized* types and the collections that must stay safe, so the prefix check cannot regress unnoticed. Harden the serialization regression test: the mutex holder is a daemon thread and the latch wait moved inside the try block, so a failed wait cannot leave the monitor held and block the test JVM. The assertion now checks the backing map is emitted as a field. --- .../debugger/util/WellKnownClassesTest.java | 40 +++++++++++++++++++ .../agent/SnapshotSerializationTest.java | 9 ++++- 2 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 dd-java-agent/agent-debugger/debugger-bootstrap/src/test/java/datadog/trace/bootstrap/debugger/util/WellKnownClassesTest.java diff --git a/dd-java-agent/agent-debugger/debugger-bootstrap/src/test/java/datadog/trace/bootstrap/debugger/util/WellKnownClassesTest.java b/dd-java-agent/agent-debugger/debugger-bootstrap/src/test/java/datadog/trace/bootstrap/debugger/util/WellKnownClassesTest.java new file mode 100644 index 00000000000..4bd01b09211 --- /dev/null +++ b/dd-java-agent/agent-debugger/debugger-bootstrap/src/test/java/datadog/trace/bootstrap/debugger/util/WellKnownClassesTest.java @@ -0,0 +1,40 @@ +package datadog.trace.bootstrap.debugger.util; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedList; +import java.util.TreeMap; +import java.util.TreeSet; +import java.util.concurrent.ConcurrentHashMap; +import org.junit.jupiter.api.Test; + +class WellKnownClassesTest { + + @Test + public void synchronizedWrappersAreNotSafe() { + assertFalse(WellKnownClasses.isSafe(Collections.synchronizedCollection(new ArrayList<>()))); + assertFalse(WellKnownClasses.isSafe(Collections.synchronizedList(new ArrayList<>()))); + assertFalse(WellKnownClasses.isSafe(Collections.synchronizedList(new LinkedList<>()))); + assertFalse(WellKnownClasses.isSafe(Collections.synchronizedSet(new HashSet<>()))); + assertFalse(WellKnownClasses.isSafe(Collections.synchronizedSortedSet(new TreeSet<>()))); + assertFalse(WellKnownClasses.isSafe(Collections.synchronizedNavigableSet(new TreeSet<>()))); + assertFalse(WellKnownClasses.isSafe(Collections.synchronizedMap(new HashMap<>()))); + assertFalse(WellKnownClasses.isSafe(Collections.synchronizedSortedMap(new TreeMap<>()))); + assertFalse(WellKnownClasses.isSafe(Collections.synchronizedNavigableMap(new TreeMap<>()))); + } + + @Test + public void plainCollectionsAreSafe() { + assertTrue(WellKnownClasses.isSafe(new ArrayList<>())); + assertTrue(WellKnownClasses.isSafe(new HashSet<>())); + assertTrue(WellKnownClasses.isSafe(new HashMap<>())); + assertTrue(WellKnownClasses.isSafe(new ConcurrentHashMap<>())); + assertTrue(WellKnownClasses.isSafe(Collections.emptyList())); + assertTrue(WellKnownClasses.isSafe(Collections.unmodifiableMap(new HashMap<>()))); + } +} diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SnapshotSerializationTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SnapshotSerializationTest.java index 84c55d38619..3c45c046014 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SnapshotSerializationTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SnapshotSerializationTest.java @@ -776,9 +776,11 @@ public void synchronizedMapLockedByAnotherThread() throws Exception { } }, "mutex-holder"); + // daemon: a failed await below would otherwise leave the mutex held and block the test JVM + holder.setDaemon(true); holder.start(); - assertTrue(locked.await(30, TimeUnit.SECONDS)); try { + assertTrue(locked.await(5, TimeUnit.SECONDS)); JsonAdapter adapter = createSnapshotAdapter(); Snapshot snapshot = createSnapshot(); CapturedContext context = new CapturedContext(); @@ -792,7 +794,10 @@ public void synchronizedMapLockedByAnotherThread() throws Exception { Map local = (Map) getLocalsFromJson(buffer).get("syncMap"); assertNull(local.get(ENTRIES)); assertNull(local.get(SIZE)); - Assertions.assertNotNull(local.get(FIELDS)); + Map fields = (Map) local.get(FIELDS); + Assertions.assertNotNull(fields); + // serialized through the object path: the backing map is a field, not entries + assertTrue(fields.containsKey("m")); } finally { release.countDown(); holder.join(); From 80f278e325903ed590a495fcc2e42ff60ac3865e Mon Sep 17 00:00:00 2001 From: Tyler Finethy Date: Thu, 30 Jul 2026 08:25:56 -0400 Subject: [PATCH 3/3] fix(debugger): guard isEmpty on unsafe collections ListValue.isEmpty() was the only expression language entry point that called into a collection without consulting WellKnownClasses, so an isEmpty() condition or log template over a Collections.synchronizedList could still take the wrapper mutex on the instrumented thread and deadlock. Mirror the guard already present in SetValue and MapValue. --- .../datadog/debugger/el/values/ListValue.java | 6 +++++- .../el/expressions/IsEmptyExpressionTest.java | 20 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/agent-debugger/debugger-el/src/main/java/com/datadog/debugger/el/values/ListValue.java b/dd-java-agent/agent-debugger/debugger-el/src/main/java/com/datadog/debugger/el/values/ListValue.java index 8c9ce5fbb87..3dc7e578d66 100644 --- a/dd-java-agent/agent-debugger/debugger-el/src/main/java/com/datadog/debugger/el/values/ListValue.java +++ b/dd-java-agent/agent-debugger/debugger-el/src/main/java/com/datadog/debugger/el/values/ListValue.java @@ -60,7 +60,11 @@ public boolean isUndefined() { public boolean isEmpty() { if (listHolder instanceof Collection) { - return ((Collection) listHolder).isEmpty(); + if (WellKnownClasses.isSafe((Collection) listHolder)) { + return ((Collection) listHolder).isEmpty(); + } + throw new UnsupportedOperationException( + "Unsupported Collection class: " + listHolder.getClass().getTypeName()); } else if (listHolder instanceof Value) { Value val = (Value) listHolder; return val.isNull() || val.isUndefined(); diff --git a/dd-java-agent/agent-debugger/debugger-el/src/test/java/com/datadog/debugger/el/expressions/IsEmptyExpressionTest.java b/dd-java-agent/agent-debugger/debugger-el/src/test/java/com/datadog/debugger/el/expressions/IsEmptyExpressionTest.java index 3581fd96907..9b26a91781b 100644 --- a/dd-java-agent/agent-debugger/debugger-el/src/test/java/com/datadog/debugger/el/expressions/IsEmptyExpressionTest.java +++ b/dd-java-agent/agent-debugger/debugger-el/src/test/java/com/datadog/debugger/el/expressions/IsEmptyExpressionTest.java @@ -15,8 +15,10 @@ import com.datadog.debugger.el.values.NumericValue; import com.datadog.debugger.el.values.StringValue; import datadog.trace.bootstrap.debugger.el.Values; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import org.junit.jupiter.api.Test; @@ -138,6 +140,24 @@ void testCollectionLiteral() { assertEquals("isEmpty(Set)", print(isEmpty6)); } + @Test + void testSynchronizedCollection() { + // calling isEmpty() on those wrappers takes the wrapper mutex and can deadlock the application + IsEmptyExpression syncList = + new IsEmptyExpression(new ListValue(Collections.synchronizedList(new ArrayList<>()))); + UnsupportedOperationException exception = + assertThrows(UnsupportedOperationException.class, () -> syncList.evaluate(evalContext)); + assertEquals( + "Unsupported Collection class: java.util.Collections$SynchronizedRandomAccessList", + exception.getMessage()); + IsEmptyExpression syncMap = + new IsEmptyExpression(new MapValue(Collections.synchronizedMap(new HashMap<>()))); + exception = + assertThrows(UnsupportedOperationException.class, () -> syncMap.evaluate(evalContext)); + assertEquals( + "Unsupported Map class: java.util.Collections$SynchronizedMap", exception.getMessage()); + } + @Test void testMapValue() { MapValue map = new MapValue(Collections.singletonMap("a", "b"));