Fix ConstructorConstructor creating mismatching Collection and Map instances

gson/src/main/java/com/google/gson/Gson.java

@@ -1104,8 +1104,7 @@ public JsonReader newJsonReader(Reader reader) {
    * @see #fromJson(String, TypeToken)
    */
   public <T> T fromJson(String json, Class<T> classOfT) throws JsonSyntaxException {
-    T object = fromJson(json, TypeToken.get(classOfT));
-    return Primitives.wrap(classOfT).cast(object);
+    return fromJson(json, TypeToken.get(classOfT));
   }
 
   /**
@@ -1196,8 +1195,7 @@ public <T> T fromJson(String json, TypeToken<T> typeOfT) throws JsonSyntaxExcept
    */
   public <T> T fromJson(Reader json, Class<T> classOfT)
       throws JsonSyntaxException, JsonIOException {
-    T object = fromJson(json, TypeToken.get(classOfT));
-    return Primitives.wrap(classOfT).cast(object);
+    return fromJson(json, TypeToken.get(classOfT));
   }
 
   /**
@@ -1358,7 +1356,19 @@ public <T> T fromJson(JsonReader reader, TypeToken<T> typeOfT)
       JsonToken unused = reader.peek();
       isEmpty = false;
       TypeAdapter<T> typeAdapter = getAdapter(typeOfT);
-      return typeAdapter.read(reader);
+      T object = typeAdapter.read(reader);
+      Class<?> expectedTypeWrapped = Primitives.wrap(typeOfT.getRawType());
+      if (object != null && !expectedTypeWrapped.isInstance(object)) {
+        throw new ClassCastException(
+            "Type adapter '"
+                + typeAdapter
+                + "' returned wrong type; requested "
+                + typeOfT.getRawType()
+                + " but got instance of "
+                + object.getClass()
+                + "\nVerify that the adapter was registered for the correct type.");
+      }
+      return object;
     } catch (EOFException e) {
       /*
        * For compatibility with JSON 1.5 and earlier, we return null for empty
@@ -1403,8 +1413,7 @@ public <T> T fromJson(JsonReader reader, TypeToken<T> typeOfT)
    * @see #fromJson(JsonElement, TypeToken)
    */
   public <T> T fromJson(JsonElement json, Class<T> classOfT) throws JsonSyntaxException {
-    T object = fromJson(json, TypeToken.get(classOfT));
-    return Primitives.wrap(classOfT).cast(object);
+    return fromJson(json, TypeToken.get(classOfT));
   }
 
   /**

gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java

@@ -36,15 +36,9 @@
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Queue;
-import java.util.Set;
-import java.util.SortedMap;
-import java.util.SortedSet;
 import java.util.TreeMap;
 import java.util.TreeSet;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.ConcurrentNavigableMap;
 import java.util.concurrent.ConcurrentSkipListMap;
 
 /** Returns a function that can construct an instance of a requested type. */
@@ -94,7 +88,19 @@ static String checkInstantiable(Class<?> c) {
     return null;
   }
 
+  /** Calls {@link #get(TypeToken, boolean)}, and allows usage of JDK Unsafe. */
   public <T> ObjectConstructor<T> get(TypeToken<T> typeToken) {
+    return get(typeToken, true);
+  }
+
+  /**
+   * Retrieves an object constructor for the given type.
+   *
+   * @param typeToken type for which a constructor should be retrieved
+   * @param allowUnsafe whether to allow usage of JDK Unsafe; has no effect if {@link #useJdkUnsafe}
+   *     is false
+   */
+  public <T> ObjectConstructor<T> get(TypeToken<T> typeToken, boolean allowUnsafe) {
     Type type = typeToken.getType();
     Class<? super T> rawType = typeToken.getRawType();
 
@@ -142,12 +148,19 @@ public <T> ObjectConstructor<T> get(TypeToken<T> typeToken) {
       };
     }
 
+    if (!allowUnsafe) {
+      String message =
+          "Unable to create instance of "
+              + rawType
+              + "; Register an InstanceCreator or a TypeAdapter for this type.";
+      return () -> {
+        throw new JsonIOException(message);
+      };
+    }
+
     // Consider usage of Unsafe as reflection, so don't use if BLOCK_ALL
     // Additionally, since it is not calling any constructor at all, don't use if BLOCK_INACCESSIBLE
-    if (filterResult == FilterResult.ALLOW) {
-      // finally try unsafe
-      return newUnsafeAllocator(rawType);
-    } else {
+    if (filterResult != FilterResult.ALLOW) {
       String message =
           "Unable to create instance of "
               + rawType
@@ -158,6 +171,9 @@ public <T> ObjectConstructor<T> get(TypeToken<T> typeToken) {
         throw new JsonIOException(message);
       };
     }
+
+    // finally try unsafe
+    return newUnsafeAllocator(rawType);
   }
 
   /**
@@ -290,7 +306,6 @@ private static <T> ObjectConstructor<T> newDefaultConstructor(
   }
 
   /** Constructors for common interface types like Map and List and their subtypes. */
-  @SuppressWarnings("unchecked") // use runtime checks to guarantee that 'T' is what it is
   private static <T> ObjectConstructor<T> newDefaultImplementationConstructor(
       Type type, Class<? super T> rawType) {
 
@@ -303,33 +318,84 @@ private static <T> ObjectConstructor<T> newDefaultImplementationConstructor(
      */
 
     if (Collection.class.isAssignableFrom(rawType)) {
-      if (SortedSet.class.isAssignableFrom(rawType)) {
-        return () -> (T) new TreeSet<>();
-      } else if (Set.class.isAssignableFrom(rawType)) {
-        return () -> (T) new LinkedHashSet<>();
-      } else if (Queue.class.isAssignableFrom(rawType)) {
-        return () -> (T) new ArrayDeque<>();
-      } else {
-        return () -> (T) new ArrayList<>();
-      }
+      @SuppressWarnings("unchecked")
+      ObjectConstructor<T> constructor = (ObjectConstructor<T>) newCollectionConstructor(rawType);
+      return constructor;
     }
 
     if (Map.class.isAssignableFrom(rawType)) {
-      if (ConcurrentNavigableMap.class.isAssignableFrom(rawType)) {
-        return () -> (T) new ConcurrentSkipListMap<>();
-      } else if (ConcurrentMap.class.isAssignableFrom(rawType)) {
-        return () -> (T) new ConcurrentHashMap<>();
-      } else if (SortedMap.class.isAssignableFrom(rawType)) {
-        return () -> (T) new TreeMap<>();
-      } else if (type instanceof ParameterizedType
-          && !String.class.isAssignableFrom(
-              TypeToken.get(((ParameterizedType) type).getActualTypeArguments()[0]).getRawType())) {
-        return () -> (T) new LinkedHashMap<>();
-      } else {
-        return () -> (T) new LinkedTreeMap<>();
-      }
+      @SuppressWarnings("unchecked")
+      ObjectConstructor<T> constructor = (ObjectConstructor<T>) newMapConstructor(type, rawType);
+      return constructor;
+    }
+
+    // Unsupported type; try other means of creating constructor
+    return null;
+  }
+
+  private static ObjectConstructor<? extends Collection<? extends Object>> newCollectionConstructor(
+      Class<?> rawType) {
+
+    // First try List implementation
+    if (rawType.isAssignableFrom(ArrayList.class)) {
+      return () -> new ArrayList<>();
+    }
+    // Then try Set implementation
+    else if (rawType.isAssignableFrom(LinkedHashSet.class)) {
+      return () -> new LinkedHashSet<>();
+    }
+    // Then try SortedSet / NavigableSet implementation
+    else if (rawType.isAssignableFrom(TreeSet.class)) {
+      return () -> new TreeSet<>();
+    }
+    // Then try Queue implementation
+    else if (rawType.isAssignableFrom(ArrayDeque.class)) {
+      return () -> new ArrayDeque<>();
+    }
+
+    // Was unable to create matching Collection constructor
+    return null;
+  }
+
+  private static boolean hasStringKeyType(Type mapType) {
+    // If mapType is not parameterized, assume it might have String as key type
+    if (!(mapType instanceof ParameterizedType)) {
+      return true;
+    }
+
+    Type[] typeArguments = ((ParameterizedType) mapType).getActualTypeArguments();
+    if (typeArguments.length == 0) {
+      return false;
+    }
+    return $Gson$Types.getRawType(typeArguments[0]) == String.class;
+  }
+
+  private static ObjectConstructor<? extends Map<? extends Object, Object>> newMapConstructor(
+      Type type, Class<?> rawType) {
+    // First try Map implementation
+    /*
+     * Legacy special casing for Map<String, ...> to avoid DoS from colliding String hashCode
+     * values for older JDKs; use own LinkedTreeMap<String, Object> instead
+     */
+    if (rawType.isAssignableFrom(LinkedTreeMap.class) && hasStringKeyType(type)) {
+      return () -> new LinkedTreeMap<>();
+    } else if (rawType.isAssignableFrom(LinkedHashMap.class)) {
+      return () -> new LinkedHashMap<>();
+    }
+    // Then try SortedMap / NavigableMap implementation
+    else if (rawType.isAssignableFrom(TreeMap.class)) {
+      return () -> new TreeMap<>();
+    }
+    // Then try ConcurrentMap implementation
+    else if (rawType.isAssignableFrom(ConcurrentHashMap.class)) {
+      return () -> new ConcurrentHashMap<>();
+    }
+    // Then try ConcurrentNavigableMap implementation
+    else if (rawType.isAssignableFrom(ConcurrentSkipListMap.class)) {
+      return () -> new ConcurrentSkipListMap<>();
     }
 
+    // Was unable to create matching Map constructor
     return null;
   }
 

gson/src/main/java/com/google/gson/internal/bind/CollectionTypeAdapterFactory.java

@@ -51,7 +51,10 @@ public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> typeToken) {
     TypeAdapter<?> elementTypeAdapter = gson.getAdapter(TypeToken.get(elementType));
     TypeAdapter<?> wrappedTypeAdapter =
         new TypeAdapterRuntimeTypeWrapper<>(gson, elementTypeAdapter, elementType);
-    ObjectConstructor<T> constructor = constructorConstructor.get(typeToken);
+    // Don't allow Unsafe usage to create instance; instances might be in broken state and calling
+    // Collection methods could lead to confusing exceptions
+    boolean allowUnsafe = false;
+    ObjectConstructor<T> constructor = constructorConstructor.get(typeToken, allowUnsafe);
 
     @SuppressWarnings({"unchecked", "rawtypes"}) // create() doesn't define a type parameter
     TypeAdapter<T> result = new Adapter(wrappedTypeAdapter, constructor);

gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java

@@ -90,7 +90,10 @@ private static Object createAdapter(
     // TODO: The exception messages created by ConstructorConstructor are currently written in the
     // context of deserialization and for example suggest usage of TypeAdapter, which would not work
     // for @JsonAdapter usage
-    return constructorConstructor.get(TypeToken.get(adapterClass)).construct();
+    // TODO: Should probably not allow usage of Unsafe; instances might be in broken state and
+    // calling adapter methods on them might lead to confusing exceptions
+    boolean allowUnsafe = true;
+    return constructorConstructor.get(TypeToken.get(adapterClass), allowUnsafe).construct();
   }
 
   private TypeAdapterFactory putFactoryAndGetCurrent(Class<?> rawType, TypeAdapterFactory factory) {

gson/src/main/java/com/google/gson/internal/bind/MapTypeAdapterFactory.java

@@ -140,7 +140,10 @@ public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> typeToken) {
     TypeAdapter<?> valueAdapter = gson.getAdapter(TypeToken.get(valueType));
     TypeAdapter<?> wrappedValueAdapter =
         new TypeAdapterRuntimeTypeWrapper<>(gson, valueAdapter, valueType);
-    ObjectConstructor<T> constructor = constructorConstructor.get(typeToken);
+    // Don't allow Unsafe usage to create instance; instances might be in broken state and calling
+    // Map methods could lead to confusing exceptions
+    boolean allowUnsafe = false;
+    ObjectConstructor<T> constructor = constructorConstructor.get(typeToken, allowUnsafe);
 
     @SuppressWarnings({"unchecked", "rawtypes"})
     // we don't define a type parameter for the key or value types

gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java

@@ -156,7 +156,7 @@ public String toString() {
       return adapter;
     }
 
-    ObjectConstructor<T> constructor = constructorConstructor.get(type);
+    ObjectConstructor<T> constructor = constructorConstructor.get(type, true);
     return new FieldReflectionAdapter<>(
         constructor, getBoundFields(gson, type, raw, blockInaccessible, false));
   }

gson/src/test/java/com/google/gson/GsonTest.java

@@ -134,6 +134,61 @@ public Object read(JsonReader in) {
     }
   }
 
+  @Test
+  public void testFromJson_WrongResultType() {
+    class IntegerAdapter extends TypeAdapter<Integer> {
+      @Override
+      public Integer read(JsonReader in) throws IOException {
+        in.skipValue();
+        return 3;
+      }
+
+      @Override
+      public void write(JsonWriter out, Integer value) {
+        throw new AssertionError("not needed for test");
+      }
+
+      @Override
+      public String toString() {
+        return "custom-adapter";
+      }
+    }
+
+    Gson gson = new GsonBuilder().registerTypeAdapter(Boolean.class, new IntegerAdapter()).create();
+    // Use `Class<?>` here to avoid that the JVM itself creates the ClassCastException (though the
+    // check below for the custom message would detect that as well)
+    Class<?> deserializedClass = Boolean.class;
+    var exception =
+        assertThrows(ClassCastException.class, () -> gson.fromJson("true", deserializedClass));
+    assertThat(exception)
+        .hasMessageThat()
+        .isEqualTo(
+            "Type adapter 'custom-adapter' returned wrong type; requested class java.lang.Boolean"
+                + " but got instance of class java.lang.Integer\n"
+                + "Verify that the adapter was registered for the correct type.");
+
+    // Returning boxed primitive should be allowed (e.g. returning `Integer` for `int`)
+    Gson gson2 = new GsonBuilder().registerTypeAdapter(int.class, new IntegerAdapter()).create();
+    assertThat(gson2.fromJson("0", int.class)).isEqualTo(3);
+
+    class NullAdapter extends TypeAdapter<Object> {
+      @Override
+      public Object read(JsonReader in) throws IOException {
+        in.skipValue();
+        return null;
+      }
+
+      @Override
+      public void write(JsonWriter out, Object value) {
+        throw new AssertionError("not needed for test");
+      }
+    }
+
+    // Returning `null` should be allowed
+    Gson gson3 = new GsonBuilder().registerTypeAdapter(Boolean.class, new NullAdapter()).create();
+    assertThat(gson3.fromJson("true", Boolean.class)).isNull();
+  }
+
   @Test
   public void testGetAdapter_Null() {
     Gson gson = new Gson();