Avoid calling synchronized wrappers when capturing

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<String> 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;
       }

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<>())));
+  }
+}

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();

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"));

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<String, String> 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<Snapshot> 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<String, Object> local = (Map<String, Object>) getLocalsFromJson(buffer).get("syncMap");
+      assertNull(local.get(ENTRIES));
+      assertNull(local.get(SIZE));
+      Map<String, Object> fields = (Map<String, Object>) 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<String, Object> doMapSize(int maxColSize) throws IOException {
     JsonAdapter<Snapshot> adapter = createSnapshotAdapter();
     Snapshot snapshot = createSnapshotForMapSize(maxColSize);