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/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/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")); 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..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 @@ -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,52 @@ 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"); + // daemon: a failed await below would otherwise leave the mutex held and block the test JVM + holder.setDaemon(true); + holder.start(); + try { + assertTrue(locked.await(5, TimeUnit.SECONDS)); + 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)); + 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(); + } + } + private Map doMapSize(int maxColSize) throws IOException { JsonAdapter adapter = createSnapshotAdapter(); Snapshot snapshot = createSnapshotForMapSize(maxColSize);