diff --git a/gson/src/main/java/com/google/gson/Gson.java b/gson/src/main/java/com/google/gson/Gson.java index e7fef48cec..5562961db3 100644 --- a/gson/src/main/java/com/google/gson/Gson.java +++ b/gson/src/main/java/com/google/gson/Gson.java @@ -1106,8 +1106,7 @@ public JsonReader newJsonReader(Reader reader) { * @see #fromJson(String, TypeToken) */ public T fromJson(String json, Class classOfT) throws JsonSyntaxException { - T object = fromJson(json, TypeToken.get(classOfT)); - return Primitives.wrap(classOfT).cast(object); + return fromJson(json, TypeToken.get(classOfT)); } /** @@ -1198,8 +1197,7 @@ public T fromJson(String json, TypeToken typeOfT) throws JsonSyntaxExcept */ public T fromJson(Reader json, Class classOfT) throws JsonSyntaxException, JsonIOException { - T object = fromJson(json, TypeToken.get(classOfT)); - return Primitives.wrap(classOfT).cast(object); + return fromJson(json, TypeToken.get(classOfT)); } /** @@ -1360,7 +1358,19 @@ public T fromJson(JsonReader reader, TypeToken typeOfT) JsonToken unused = reader.peek(); isEmpty = false; TypeAdapter 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 @@ -1405,8 +1415,7 @@ public T fromJson(JsonReader reader, TypeToken typeOfT) * @see #fromJson(JsonElement, TypeToken) */ public T fromJson(JsonElement json, Class classOfT) throws JsonSyntaxException { - T object = fromJson(json, TypeToken.get(classOfT)); - return Primitives.wrap(classOfT).cast(object); + return fromJson(json, TypeToken.get(classOfT)); } /** diff --git a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java index b4deb74726..8a6f29908a 100644 --- a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java +++ b/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. */ @@ -321,7 +315,6 @@ public T construct() { } /** 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 ObjectConstructor newDefaultImplementationConstructor( final Type type, Class rawType) { @@ -334,78 +327,135 @@ private static ObjectConstructor newDefaultImplementationConstructor( */ if (Collection.class.isAssignableFrom(rawType)) { - if (SortedSet.class.isAssignableFrom(rawType)) { - return new ObjectConstructor() { - @Override - public T construct() { - return (T) new TreeSet<>(); - } - }; - } else if (Set.class.isAssignableFrom(rawType)) { - return new ObjectConstructor() { - @Override - public T construct() { - return (T) new LinkedHashSet<>(); - } - }; - } else if (Queue.class.isAssignableFrom(rawType)) { - return new ObjectConstructor() { - @Override - public T construct() { - return (T) new ArrayDeque<>(); - } - }; - } else { - return new ObjectConstructor() { - @Override - public T construct() { - return (T) new ArrayList<>(); - } - }; - } + @SuppressWarnings("unchecked") + ObjectConstructor constructor = (ObjectConstructor) newCollectionConstructor(rawType); + return constructor; } if (Map.class.isAssignableFrom(rawType)) { - if (ConcurrentNavigableMap.class.isAssignableFrom(rawType)) { - return new ObjectConstructor() { - @Override - public T construct() { - return (T) new ConcurrentSkipListMap<>(); - } - }; - } else if (ConcurrentMap.class.isAssignableFrom(rawType)) { - return new ObjectConstructor() { - @Override - public T construct() { - return (T) new ConcurrentHashMap<>(); - } - }; - } else if (SortedMap.class.isAssignableFrom(rawType)) { - return new ObjectConstructor() { - @Override - public T construct() { - return (T) new TreeMap<>(); - } - }; - } else if (type instanceof ParameterizedType - && !String.class.isAssignableFrom( - TypeToken.get(((ParameterizedType) type).getActualTypeArguments()[0]).getRawType())) { - return new ObjectConstructor() { - @Override - public T construct() { - return (T) new LinkedHashMap<>(); - } - }; - } else { - return new ObjectConstructor() { - @Override - public T construct() { - return (T) new LinkedTreeMap<>(); - } - }; - } + @SuppressWarnings("unchecked") + ObjectConstructor constructor = (ObjectConstructor) newMapConstructor(type, rawType); + return constructor; + } + + // Unsupported type; try other means of creating constructor + return null; + } + + // Suppress Error Prone false positive: Pattern is reported for overridden methods, see + // https://github.com/google/error-prone/issues/4563 + @SuppressWarnings("NonApiType") + private static ObjectConstructor> newCollectionConstructor( + Class rawType) { + + // First try List implementation + if (rawType.isAssignableFrom(ArrayList.class)) { + return new ObjectConstructor>() { + @Override + public ArrayList construct() { + return new ArrayList<>(); + } + }; + } + // Then try Set implementation + else if (rawType.isAssignableFrom(LinkedHashSet.class)) { + return new ObjectConstructor>() { + @Override + public LinkedHashSet construct() { + return new LinkedHashSet<>(); + } + }; + } + // Then try SortedSet / NavigableSet implementation + else if (rawType.isAssignableFrom(TreeSet.class)) { + return new ObjectConstructor>() { + @Override + public TreeSet construct() { + return new TreeSet<>(); + } + }; + } + // Then try Queue implementation + else if (rawType.isAssignableFrom(ArrayDeque.class)) { + return new ObjectConstructor>() { + @Override + public ArrayDeque construct() { + 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; + } + + // Suppress Error Prone false positive: Pattern is reported for overridden methods, see + // https://github.com/google/error-prone/issues/4563 + @SuppressWarnings("NonApiType") + private static ObjectConstructor> newMapConstructor( + final Type type, Class rawType) { + // First try Map implementation + /* + * Legacy special casing for Map to avoid DoS from colliding String hashCode + * values for older JDKs; use own LinkedTreeMap instead + */ + if (rawType.isAssignableFrom(LinkedTreeMap.class) && hasStringKeyType(type)) { + return new ObjectConstructor>() { + @Override + public LinkedTreeMap construct() { + return new LinkedTreeMap<>(); + } + }; + } else if (rawType.isAssignableFrom(LinkedHashMap.class)) { + return new ObjectConstructor>() { + @Override + public LinkedHashMap construct() { + return new LinkedHashMap<>(); + } + }; + } + // Then try SortedMap / NavigableMap implementation + else if (rawType.isAssignableFrom(TreeMap.class)) { + return new ObjectConstructor>() { + @Override + public TreeMap construct() { + return new TreeMap<>(); + } + }; + } + // Then try ConcurrentMap implementation + else if (rawType.isAssignableFrom(ConcurrentHashMap.class)) { + return new ObjectConstructor>() { + @Override + public ConcurrentHashMap construct() { + return new ConcurrentHashMap<>(); + } + }; + } + // Then try ConcurrentNavigableMap implementation + else if (rawType.isAssignableFrom(ConcurrentSkipListMap.class)) { + return new ObjectConstructor>() { + @Override + public ConcurrentSkipListMap construct() { + return new ConcurrentSkipListMap<>(); + } + }; } + // Was unable to create matching Map constructor return null; } diff --git a/gson/src/test/java/com/google/gson/GsonTest.java b/gson/src/test/java/com/google/gson/GsonTest.java index 8bfc69b45a..9686db61f3 100644 --- a/gson/src/test/java/com/google/gson/GsonTest.java +++ b/gson/src/test/java/com/google/gson/GsonTest.java @@ -142,6 +142,61 @@ public Object read(JsonReader in) { } } + @Test + public void testFromJson_WrongResultType() { + class IntegerAdapter extends TypeAdapter { + @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 { + @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(); diff --git a/gson/src/test/java/com/google/gson/functional/MapTest.java b/gson/src/test/java/com/google/gson/functional/MapTest.java index 293178c607..d2a5d95e59 100644 --- a/gson/src/test/java/com/google/gson/functional/MapTest.java +++ b/gson/src/test/java/com/google/gson/functional/MapTest.java @@ -17,6 +17,7 @@ package com.google.gson.functional; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.assertThrows; import com.google.gson.Gson; @@ -32,9 +33,11 @@ import com.google.gson.JsonSyntaxException; import com.google.gson.common.TestTypes; import com.google.gson.internal.$Gson$Types; +import com.google.gson.internal.LinkedTreeMap; import com.google.gson.reflect.TypeToken; import java.lang.reflect.Type; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.Map; @@ -210,6 +213,48 @@ public void testMapDeserializationWithUnquotedLongKeys() { assertThat(map.get(longKey)).isEqualTo("456"); } + @Test + public void testMapStringKeyDeserialization() { + Type typeOfMap = new TypeToken>() {}.getType(); + Map map = gson.fromJson("{\"a\":1}", typeOfMap); + + assertWithMessage( + "Map should use LinkedTreeMap to protect against DoS in older JDK" + + " versions") + .that(map) + .isInstanceOf(LinkedTreeMap.class); + + Map expectedMap = Collections.singletonMap("a", 1); + assertThat(map).isEqualTo(expectedMap); + } + + @Test + public void testMapStringSupertypeKeyDeserialization() { + // Should only use Gson's LinkedTreeMap for String as key, but not for supertypes (e.g. Object) + Type typeOfMap = new TypeToken>() {}.getType(); + Map map = gson.fromJson("{\"a\":1}", typeOfMap); + + assertWithMessage("Map should not use Gson Map implementation") + .that(map) + .isNotInstanceOf(LinkedTreeMap.class); + + Map expectedMap = Collections.singletonMap("a", 1); + assertThat(map).isEqualTo(expectedMap); + } + + @Test + public void testMapNonStringKeyDeserialization() { + Type typeOfMap = new TypeToken>() {}.getType(); + Map map = gson.fromJson("{\"1\":1}", typeOfMap); + + assertWithMessage("Map should not use Gson Map implementation") + .that(map) + .isNotInstanceOf(LinkedTreeMap.class); + + Map expectedMap = Collections.singletonMap(1, 1); + assertThat(map).isEqualTo(expectedMap); + } + @Test public void testHashMapDeserialization() { Type typeOfMap = new TypeToken>() {}.getType(); diff --git a/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java b/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java index 13a1fe2989..ad201fb346 100644 --- a/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java +++ b/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java @@ -17,10 +17,24 @@ package com.google.gson.internal; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.assertThrows; import com.google.gson.reflect.TypeToken; +import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; +import java.util.EnumMap; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.TreeSet; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentSkipListMap; +import java.util.concurrent.LinkedBlockingDeque; import org.junit.Test; public class ConstructorConstructorTest { @@ -64,4 +78,194 @@ public void testGet_Interface() { + " this type. Interface name:" + " com.google.gson.internal.ConstructorConstructorTest$Interface"); } + + @SuppressWarnings("serial") + private static class CustomSortedSet extends TreeSet { + // Removes default no-args constructor + @SuppressWarnings("unused") + CustomSortedSet(Void v) {} + } + + @SuppressWarnings("serial") + private static class CustomSet extends HashSet { + // Removes default no-args constructor + @SuppressWarnings("unused") + CustomSet(Void v) {} + } + + @SuppressWarnings("serial") + private static class CustomQueue extends LinkedBlockingDeque { + // Removes default no-args constructor + @SuppressWarnings("unused") + CustomQueue(Void v) {} + } + + @SuppressWarnings("serial") + private static class CustomList extends ArrayList { + // Removes default no-args constructor + @SuppressWarnings("unused") + CustomList(Void v) {} + } + + /** + * Tests that creation of custom {@code Collection} subclasses without no-args constructor should + * not use default JDK types (which would cause {@link ClassCastException}). + * + *

Currently this test is rather contrived because the instances created using Unsafe are not + * usable because their fields are not properly initialized, but assume that user has custom + * classes which would be functional. + */ + @Test + public void testCustomCollectionCreation() { + Class[] collectionTypes = { + CustomSortedSet.class, CustomSet.class, CustomQueue.class, CustomList.class, + }; + + for (Class collectionType : collectionTypes) { + Object actual = + constructorConstructor + .get(TypeToken.getParameterized(collectionType, Integer.class)) + .construct(); + assertWithMessage( + "Failed for " + collectionType + "; created instance of " + actual.getClass()) + .that(actual) + .isInstanceOf(collectionType); + } + } + + private static interface CustomCollectionInterface extends Collection {} + + private static interface CustomSetInterface extends Set {} + + private static interface CustomListInterface extends List {} + + @Test + public void testCustomCollectionInterfaceCreation() { + Class[] interfaces = { + CustomCollectionInterface.class, CustomSetInterface.class, CustomListInterface.class, + }; + + for (Class interfaceType : interfaces) { + var objectConstructor = constructorConstructor.get(TypeToken.get(interfaceType)); + var exception = assertThrows(RuntimeException.class, () -> objectConstructor.construct()); + assertThat(exception) + .hasMessageThat() + .isEqualTo( + "Interfaces can't be instantiated! Register an InstanceCreator or a TypeAdapter" + + " for this type. Interface name: " + + interfaceType.getName()); + } + } + + @Test + public void testStringMapCreation() { + // When creating raw Map should use Gson's LinkedTreeMap, assuming keys could be String + Object actual = constructorConstructor.get(TypeToken.get(Map.class)).construct(); + assertThat(actual).isInstanceOf(LinkedTreeMap.class); + + // When creating a `Map` should use Gson's LinkedTreeMap + actual = constructorConstructor.get(new TypeToken>() {}).construct(); + assertThat(actual).isInstanceOf(LinkedTreeMap.class); + + // But when explicitly requesting a JDK `LinkedHashMap` should use LinkedHashMap + actual = + constructorConstructor.get(new TypeToken>() {}).construct(); + assertThat(actual).isInstanceOf(LinkedHashMap.class); + + // For all Map types with non-String key, should use JDK LinkedHashMap by default + // This is also done to avoid ClassCastException later, because Gson's LinkedTreeMap requires + // that keys are Comparable + Class[] nonStringTypes = {Integer.class, CharSequence.class, Object.class}; + for (Class keyType : nonStringTypes) { + actual = + constructorConstructor + .get(TypeToken.getParameterized(Map.class, keyType, Integer.class)) + .construct(); + assertWithMessage( + "Failed for key type " + keyType + "; created instance of " + actual.getClass()) + .that(actual) + .isInstanceOf(LinkedHashMap.class); + } + } + + private enum MyEnum {} + + @SuppressWarnings("serial") + private static class CustomEnumMap extends EnumMap { + @SuppressWarnings("unused") + CustomEnumMap(Void v) { + super(MyEnum.class); + } + } + + @SuppressWarnings("serial") + private static class CustomConcurrentNavigableMap extends ConcurrentSkipListMap { + // Removes default no-args constructor + @SuppressWarnings("unused") + CustomConcurrentNavigableMap(Void v) {} + } + + @SuppressWarnings("serial") + private static class CustomConcurrentMap extends ConcurrentHashMap { + // Removes default no-args constructor + @SuppressWarnings("unused") + CustomConcurrentMap(Void v) {} + } + + @SuppressWarnings("serial") + private static class CustomSortedMap extends TreeMap { + // Removes default no-args constructor + @SuppressWarnings("unused") + CustomSortedMap(Void v) {} + } + + @SuppressWarnings("serial") + private static class CustomLinkedHashMap extends LinkedHashMap { + // Removes default no-args constructor + @SuppressWarnings("unused") + CustomLinkedHashMap(Void v) {} + } + + /** + * Tests that creation of custom {@code Map} subclasses without no-args constructor should not use + * default JDK types (which would cause {@link ClassCastException}). + * + *

Currently this test is rather contrived because the instances created using Unsafe are not + * usable because their fields are not properly initialized, but assume that user has custom + * classes which would be functional. + */ + @Test + public void testCustomMapCreation() { + Class[] mapTypes = { + CustomEnumMap.class, + CustomConcurrentNavigableMap.class, + CustomConcurrentMap.class, + CustomSortedMap.class, + CustomLinkedHashMap.class, + }; + + for (Class mapType : mapTypes) { + Object actual = + constructorConstructor + .get(TypeToken.getParameterized(mapType, String.class, Integer.class)) + .construct(); + assertWithMessage("Failed for " + mapType + "; created instance of " + actual.getClass()) + .that(actual) + .isInstanceOf(mapType); + } + } + + private static interface CustomMapInterface extends Map {} + + @Test + public void testCustomMapInterfaceCreation() { + var objectConstructor = constructorConstructor.get(TypeToken.get(CustomMapInterface.class)); + var exception = assertThrows(RuntimeException.class, () -> objectConstructor.construct()); + assertThat(exception) + .hasMessageThat() + .isEqualTo( + "Interfaces can't be instantiated! Register an InstanceCreator or a TypeAdapter" + + " for this type. Interface name: " + + CustomMapInterface.class.getName()); + } } diff --git a/proto/src/main/java/com/google/gson/protobuf/ProtoTypeAdapter.java b/proto/src/main/java/com/google/gson/protobuf/ProtoTypeAdapter.java index 30146c54fc..932bca241e 100644 --- a/proto/src/main/java/com/google/gson/protobuf/ProtoTypeAdapter.java +++ b/proto/src/main/java/com/google/gson/protobuf/ProtoTypeAdapter.java @@ -20,6 +20,7 @@ import com.google.common.base.CaseFormat; import com.google.common.collect.MapMaker; +import com.google.common.reflect.TypeToken; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.gson.JsonArray; import com.google.gson.JsonDeserializationContext; @@ -44,6 +45,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentMap; @@ -331,8 +333,15 @@ public Message deserialize(JsonElement json, Type typeOfT, JsonDeserializationCo String protoArrayFieldName = protoFormat.to(CaseFormat.LOWER_CAMEL, fieldDescriptor.getName()) + "_"; Field protoArrayField = protoClass.getDeclaredField(protoArrayFieldName); - Type protoArrayFieldType = protoArrayField.getGenericType(); - fieldValue = context.deserialize(jsonElement, protoArrayFieldType); + + @SuppressWarnings("unchecked") + TypeToken> protoArrayFieldType = + (TypeToken>) TypeToken.of(protoArrayField.getGenericType()); + // Get the type as `List`, otherwise type might be Protobuf internal interface for + // which no instance can be created + Type protoArrayResolvedFieldType = + protoArrayFieldType.getSupertype(List.class).getType(); + fieldValue = context.deserialize(jsonElement, protoArrayResolvedFieldType); protoBuilder.setField(fieldDescriptor, fieldValue); } else { Object field = defaultInstance.getField(fieldDescriptor);