From 5b83a03aa2f1d33889ff47f957caccd707d4a77e Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 29 Jan 2022 19:15:48 +0100 Subject: [PATCH 1/8] Fix ConstructorConstructor creating mismatching Collection instances --- .../gson/internal/ConstructorConstructor.java | 212 +++++++++++------- .../gson/functional/CollectionTest.java | 23 +- .../com/google/gson/functional/MapTest.java | 48 ++-- .../gson/functional/ReflectionAccessTest.java | 2 +- .../internal/ConstructorConstructorTest.java | 177 +++++++++++++++ 5 files changed, 353 insertions(+), 109 deletions(-) create mode 100644 gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java 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 aa1e3ff61b..aacc149b28 100644 --- a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java +++ b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java @@ -16,6 +16,10 @@ package com.google.gson.internal; +import com.google.gson.InstanceCreator; +import com.google.gson.JsonIOException; +import com.google.gson.internal.reflect.ReflectionHelper; +import com.google.gson.reflect.TypeToken; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.ParameterizedType; @@ -27,22 +31,11 @@ import java.util.LinkedHashMap; import java.util.LinkedHashSet; 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; -import com.google.gson.InstanceCreator; -import com.google.gson.JsonIOException; -import com.google.gson.internal.reflect.ReflectionHelper; -import com.google.gson.reflect.TypeToken; - /** * Returns a function that can construct an instance of a requested type. */ @@ -151,88 +144,141 @@ 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 ObjectConstructor newDefaultImplementationConstructor( + private static ObjectConstructor newDefaultImplementationConstructor( final Type type, Class rawType) { + if (Collection.class.isAssignableFrom(rawType)) { - if (SortedSet.class.isAssignableFrom(rawType)) { - return new ObjectConstructor() { - @Override public T construct() { - return (T) new TreeSet(); - } - }; - } else if (EnumSet.class.isAssignableFrom(rawType)) { - return new ObjectConstructor() { - @SuppressWarnings("rawtypes") - @Override public T construct() { - if (type instanceof ParameterizedType) { - Type elementType = ((ParameterizedType) type).getActualTypeArguments()[0]; - if (elementType instanceof Class) { - return (T) EnumSet.noneOf((Class)elementType); - } else { - throw new JsonIOException("Invalid EnumSet type: " + type.toString()); - } + @SuppressWarnings("unchecked") + ObjectConstructor constructor = (ObjectConstructor) newCollectionConstructor(type, rawType); + return constructor; + } + + if (Map.class.isAssignableFrom(rawType)) { + @SuppressWarnings("unchecked") + ObjectConstructor constructor = (ObjectConstructor) newMapConstructor(type, rawType); + return constructor; + } + + // Unsupported type; try other means of creating constructor + return null; + } + + private static ObjectConstructor> newCollectionConstructor( + final Type type, Class rawType) { + + if (EnumSet.class.isAssignableFrom(rawType)) { + return new ObjectConstructor>() { + @Override public EnumSet construct() { + if (type instanceof ParameterizedType) { + Type elementType = ((ParameterizedType) type).getActualTypeArguments()[0]; + if (elementType instanceof Class) { + @SuppressWarnings({"unchecked", "rawtypes"}) + EnumSet set = EnumSet.noneOf((Class) elementType); + return set; } else { throw new JsonIOException("Invalid EnumSet type: " + type.toString()); } + } else { + throw new JsonIOException("Invalid EnumSet type: " + type.toString()); } - }; - } 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(); - } - }; - } + } + }; + } + // First try List implementation + else 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(); + } + }; } - 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(); - } - }; - } + // 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 TypeToken.get(typeArguments[0]).getRawType() == String.class; + } + + private static ObjectConstructor> newMapConstructor(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(LinkedHashMap.class) && !hasStringKeyType(type)) { + return new ObjectConstructor>() { + @Override public LinkedHashMap construct() { + return new LinkedHashMap(); + } + }; + } else if (rawType.isAssignableFrom(LinkedTreeMap.class)) { + return new ObjectConstructor>() { + @Override public LinkedTreeMap construct() { + return new LinkedTreeMap(); + } + }; + } + // 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/functional/CollectionTest.java b/gson/src/test/java/com/google/gson/functional/CollectionTest.java index 261c5d02d3..f618fdc66e 100644 --- a/gson/src/test/java/com/google/gson/functional/CollectionTest.java +++ b/gson/src/test/java/com/google/gson/functional/CollectionTest.java @@ -16,6 +16,16 @@ package com.google.gson.functional; +import static org.junit.Assert.assertArrayEquals; + +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import com.google.gson.JsonElement; +import com.google.gson.JsonPrimitive; +import com.google.gson.JsonSerializationContext; +import com.google.gson.JsonSerializer; +import com.google.gson.common.TestTypes.BagOfPrimitives; +import com.google.gson.reflect.TypeToken; import java.lang.reflect.Type; import java.util.ArrayList; import java.util.Arrays; @@ -30,18 +40,7 @@ import java.util.Set; import java.util.Stack; import java.util.Vector; - -import com.google.gson.Gson; -import com.google.gson.GsonBuilder; -import com.google.gson.JsonElement; -import com.google.gson.JsonPrimitive; -import com.google.gson.JsonSerializationContext; -import com.google.gson.JsonSerializer; -import com.google.gson.common.TestTypes.BagOfPrimitives; -import com.google.gson.reflect.TypeToken; - import junit.framework.TestCase; -import static org.junit.Assert.assertArrayEquals; /** * Functional tests for Json serialization and deserialization of collections. @@ -324,7 +323,7 @@ public void testFieldIsArrayList() { HasArrayListField copy = gson.fromJson("{\"longs\":[1,3]}", HasArrayListField.class); assertEquals(Arrays.asList(1L, 3L), copy.longs); } - + public void testUserCollectionTypeAdapter() { Type listOfString = new TypeToken>() {}.getType(); Object stringListSerializer = new JsonSerializer>() { 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 d14585b382..1d70b7d24d 100644 --- a/gson/src/test/java/com/google/gson/functional/MapTest.java +++ b/gson/src/test/java/com/google/gson/functional/MapTest.java @@ -16,18 +16,6 @@ package com.google.gson.functional; -import java.lang.reflect.Type; -import java.util.Collection; -import java.util.HashMap; -import java.util.LinkedHashMap; -import java.util.Map; -import java.util.SortedMap; -import java.util.TreeMap; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; -import java.util.concurrent.ConcurrentNavigableMap; -import java.util.concurrent.ConcurrentSkipListMap; - import com.google.gson.Gson; import com.google.gson.GsonBuilder; import com.google.gson.InstanceCreator; @@ -41,8 +29,20 @@ 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; +import java.util.SortedMap; +import java.util.TreeMap; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.ConcurrentNavigableMap; +import java.util.concurrent.ConcurrentSkipListMap; import junit.framework.TestCase; /** @@ -195,6 +195,28 @@ public void testMapDeserializationWithUnquotedLongKeys() { assertEquals("456", map.get(longKey)); } + public void testMapStringKeyDeserialization() { + Type typeOfMap = new TypeToken>() {}.getType(); + Map map = gson.fromJson("{\"a\":1}", typeOfMap); + + assertTrue("Map should use LinkedTreeMap to protect against DoS in older JDK versions", + map instanceof LinkedTreeMap); + + Map expectedMap = Collections.singletonMap("a", 1); + assertEquals(expectedMap, map); + } + + public void testMapNonStringKeyDeserialization() { + Type typeOfMap = new TypeToken>() {}.getType(); + Map map = gson.fromJson("{\"1\":1}", typeOfMap); + + assertFalse("non-Map should not use Gson Map implementation", + map instanceof LinkedTreeMap); + + Map expectedMap = Collections.singletonMap(1, 1); + assertEquals(expectedMap, map); + } + public void testHashMapDeserialization() throws Exception { Type typeOfMap = new TypeToken>() {}.getType(); HashMap map = gson.fromJson("{\"123\":\"456\"}", typeOfMap); diff --git a/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java b/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java index ece351240a..5f3d0ccb8c 100644 --- a/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java +++ b/gson/src/test/java/com/google/gson/functional/ReflectionAccessTest.java @@ -111,7 +111,7 @@ public void testSerializeInternalImplementationObject() { // But deserialization should fail Class internalClass = Collections.emptyList().getClass(); try { - gson.fromJson("{}", internalClass); + gson.fromJson("[]", internalClass); fail("Missing exception; test has to be run with `--illegal-access=deny`"); } catch (JsonIOException expected) { assertTrue(expected.getMessage().startsWith( diff --git a/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java b/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java new file mode 100644 index 0000000000..72713fd0b3 --- /dev/null +++ b/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java @@ -0,0 +1,177 @@ +package com.google.gson.internal; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import com.google.gson.InstanceCreator; +import com.google.gson.reflect.TypeToken; +import java.lang.reflect.Type; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +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 { + private final ConstructorConstructor constructor = new ConstructorConstructor(Collections.>emptyMap(), true); + + @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 = constructor.get(TypeToken.getParameterized(collectionType, Integer.class)).construct(); + assertTrue("Failed for " + collectionType + "; created instance of " + actual.getClass(), collectionType.isInstance(actual)); + } + } + + 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) { + try { + constructor.get(TypeToken.get(interfaceType)).construct(); + fail(); + } catch (RuntimeException e) { + assertEquals("Unable to create instance of " + interfaceType + ". Registering an InstanceCreator or a TypeAdapter" + + " for this type, or adding a no-args constructor may fix this problem.", e.getMessage()); + } + } + } + + @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 = { + CustomConcurrentNavigableMap.class, + CustomConcurrentMap.class, + CustomSortedMap.class, + CustomLinkedHashMap.class, + }; + + for (Class mapType : mapTypes) { + Object actual = constructor.get(TypeToken.getParameterized(mapType, String.class, Integer.class)).construct(); + assertTrue("Failed for " + mapType + "; created instance of " + actual.getClass(), mapType.isInstance(actual)); + } + } + + private static interface CustomMapInterface extends Map { + } + + @Test + public void testCustomMapInterfaceCreation() { + try { + constructor.get(TypeToken.get(CustomMapInterface.class)).construct(); + fail(); + } catch (RuntimeException e) { + assertEquals("Unable to create instance of " + CustomMapInterface.class + ". Registering an InstanceCreator or a TypeAdapter" + + " for this type, or adding a no-args constructor may fix this problem.", e.getMessage()); + } + } +} From d5d7cdd24c8b8233efa7130b456e0382f12420fb Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 29 Jan 2022 19:52:05 +0100 Subject: [PATCH 2/8] Support EnumMap deserialization --- gson/src/main/java/com/google/gson/Gson.java | 3 +++ .../gson/internal/ConstructorConstructor.java | 25 +++++++++++++++++-- .../com/google/gson/functional/EnumTest.java | 17 +++++++++++++ .../internal/ConstructorConstructorTest.java | 13 ++++++++++ 4 files changed, 56 insertions(+), 2 deletions(-) diff --git a/gson/src/main/java/com/google/gson/Gson.java b/gson/src/main/java/com/google/gson/Gson.java index 62c8b0c6b7..d99c875c06 100644 --- a/gson/src/main/java/com/google/gson/Gson.java +++ b/gson/src/main/java/com/google/gson/Gson.java @@ -989,6 +989,9 @@ public T fromJson(JsonReader reader, Type typeOfT) throws JsonIOException, J TypeToken typeToken = (TypeToken) TypeToken.get(typeOfT); TypeAdapter typeAdapter = getAdapter(typeToken); T object = typeAdapter.read(reader); + + // Verify that adapter created the correct type + Primitives.wrap(typeToken.getRawType()).cast(object); return object; } catch (EOFException e) { /* 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 aacc149b28..a656e852f5 100644 --- a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java +++ b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java @@ -27,6 +27,7 @@ import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; +import java.util.EnumMap; import java.util.EnumSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -234,13 +235,33 @@ private static boolean hasStringKeyType(Type mapType) { return TypeToken.get(typeArguments[0]).getRawType() == String.class; } - private static ObjectConstructor> newMapConstructor(Type type, Class rawType) { + private static ObjectConstructor> newMapConstructor(final Type type, Class rawType) { + // Only support creation of EnumMap, but not of custom subtypes; for them type parameters + // and constructor parameter might have completely different meaning + if (rawType == EnumMap.class) { + return new ObjectConstructor>() { + @Override public EnumMap construct() { + if (type instanceof ParameterizedType) { + Type elementType = ((ParameterizedType) type).getActualTypeArguments()[0]; + if (elementType instanceof Class) { + @SuppressWarnings({"unchecked", "rawtypes"}) + EnumMap map = new EnumMap((Class) elementType); + return map; + } else { + throw new JsonIOException("Invalid EnumMap type: " + type.toString()); + } + } else { + throw new JsonIOException("Invalid EnumMap type: " + type.toString()); + } + } + }; + } // 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(LinkedHashMap.class) && !hasStringKeyType(type)) { + else if (rawType.isAssignableFrom(LinkedHashMap.class) && !hasStringKeyType(type)) { return new ObjectConstructor>() { @Override public LinkedHashMap construct() { return new LinkedHashMap(); diff --git a/gson/src/test/java/com/google/gson/functional/EnumTest.java b/gson/src/test/java/com/google/gson/functional/EnumTest.java index 8a1c6e12c6..fd21a5c271 100644 --- a/gson/src/test/java/com/google/gson/functional/EnumTest.java +++ b/gson/src/test/java/com/google/gson/functional/EnumTest.java @@ -31,7 +31,10 @@ import java.lang.reflect.Type; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; +import java.util.EnumMap; import java.util.EnumSet; +import java.util.Map; import java.util.Set; import junit.framework.TestCase; /** @@ -150,6 +153,8 @@ public void testEnumCaseMapping() { public void testEnumSet() { EnumSet foo = EnumSet.of(Roshambo.ROCK, Roshambo.PAPER); String json = gson.toJson(foo); + assertEquals("[\"ROCK\",\"PAPER\"]", json); + Type type = new TypeToken>() {}.getType(); EnumSet bar = gson.fromJson(json, type); assertTrue(bar.contains(Roshambo.ROCK)); @@ -157,6 +162,18 @@ public void testEnumSet() { assertFalse(bar.contains(Roshambo.SCISSORS)); } + public void testEnumMap() throws Exception { + EnumMap map = new EnumMap(MyEnum.class); + map.put(MyEnum.VALUE1, "test"); + String json = gson.toJson(map); + assertEquals("{\"VALUE1\":\"test\"}", json); + + Type type = new TypeToken>() {}.getType(); + EnumMap actualMap = gson.fromJson("{\"VALUE1\":\"test\"}", type); + Map expectedMap = Collections.singletonMap(MyEnum.VALUE1, "test"); + assertEquals(expectedMap, actualMap); + } + public enum Roshambo { ROCK { @Override Roshambo defeats() { 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 72713fd0b3..08af9dbd46 100644 --- a/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java +++ b/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java @@ -10,6 +10,7 @@ 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; @@ -106,6 +107,17 @@ public void testCustomCollectionInterfaceCreation() { } } + 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 @@ -149,6 +161,7 @@ private static class CustomLinkedHashMap extends LinkedHashMap { @Test public void testCustomMapCreation() { Class[] mapTypes = { + CustomEnumMap.class, CustomConcurrentNavigableMap.class, CustomConcurrentMap.class, CustomSortedMap.class, From c6aa3b1e47cf0b2bb6694c37090032b4a498c2c3 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Wed, 18 May 2022 00:10:27 +0200 Subject: [PATCH 3/8] Use LinkedTreeMap for maps with String supertype as key --- .../gson/internal/ConstructorConstructor.java | 3 ++- .../com/google/gson/functional/MapTest.java | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) 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 a656e852f5..02893c498d 100644 --- a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java +++ b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java @@ -232,7 +232,8 @@ private static boolean hasStringKeyType(Type mapType) { if (typeArguments.length == 0) { return false; } - return TypeToken.get(typeArguments[0]).getRawType() == String.class; + // Consider String and supertypes of it + return TypeToken.get(typeArguments[0]).getRawType().isAssignableFrom(String.class); } private static ObjectConstructor> newMapConstructor(final Type type, Class rawType) { 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 1d70b7d24d..9a31f57b58 100644 --- a/gson/src/test/java/com/google/gson/functional/MapTest.java +++ b/gson/src/test/java/com/google/gson/functional/MapTest.java @@ -206,6 +206,17 @@ public void testMapStringKeyDeserialization() { assertEquals(expectedMap, map); } + public void testMapStringSupertypeKeyDeserialization() { + Type typeOfMap = new TypeToken>() {}.getType(); + Map map = gson.fromJson("{\"a\":1}", typeOfMap); + + assertTrue("Map should use LinkedTreeMap to protect against DoS in older JDK versions", + map instanceof LinkedTreeMap); + + Map expectedMap = Collections.singletonMap("a", 1); + assertEquals(expectedMap, map); + } + public void testMapNonStringKeyDeserialization() { Type typeOfMap = new TypeToken>() {}.getType(); Map map = gson.fromJson("{\"1\":1}", typeOfMap); @@ -306,7 +317,7 @@ public void testMapStandardSubclassDeserialization() { public void testMapSubclassDeserialization() { Gson gson = new GsonBuilder().registerTypeAdapter(MyMap.class, new InstanceCreator() { - public MyMap createInstance(Type type) { + @Override public MyMap createInstance(Type type) { return new MyMap(); } }).create(); @@ -321,7 +332,7 @@ public void testCustomSerializerForSpecificMapType() { null, Map.class, String.class, Long.class); Gson gson = new GsonBuilder() .registerTypeAdapter(type, new JsonSerializer>() { - public JsonElement serialize(Map src, Type typeOfSrc, + @Override public JsonElement serialize(Map src, Type typeOfSrc, JsonSerializationContext context) { JsonArray array = new JsonArray(); for (long value : src.values()) { @@ -515,7 +526,7 @@ public final void testInterfaceTypeMapWithSerializer() { + "\"subs\":{\"Test\":" + subTypeJson + "}}"; JsonSerializer baseTypeAdapter = new JsonSerializer() { - public JsonElement serialize(TestTypes.Base src, Type typeOfSrc, + @Override public JsonElement serialize(TestTypes.Base src, Type typeOfSrc, JsonSerializationContext context) { return baseTypeJsonElement; } From 6a63611a5762304a592feb00a9ffa8f5f5a8b508 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Wed, 18 May 2022 01:08:12 +0200 Subject: [PATCH 4/8] Fix test method name typo --- gson/src/test/java/com/google/gson/functional/MapTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 813b88a043..35fb0cb3c7 100644 --- a/gson/src/test/java/com/google/gson/functional/MapTest.java +++ b/gson/src/test/java/com/google/gson/functional/MapTest.java @@ -620,7 +620,7 @@ public void testSerializeMapOfMaps() { gson.toJson(map, type).replace('"', '\'')); } - public void testDeerializeMapOfMaps() { + public void testDeserializeMapOfMaps() { Type type = new TypeToken>>() {}.getType(); Map> map = newMap( "a", newMap("ka1", "va1", "ka2", "va2"), From 1f496d7e98f494cb86fe52a8e6973f435121a1ea Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 7 Sep 2024 00:14:25 +0200 Subject: [PATCH 5/8] Fix `ProtoTypeAdapter` being unable to deserialize certain repeated fields For example a `List` Protobuf field might internally have the Protobuf-internal `LongList` interface type. Previously Gson's ConstructorConstructor was nonetheless creating an ArrayList for this, which is wrong but worked. Now with the changes in ConstructorConstructor Gson will fail to create an instance, so this commit tries to solve this properly in ProtoTypeAdapter. --- .../com/google/gson/protobuf/ProtoTypeAdapter.java | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) 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); From f8ca948f8c7b51efcf23da0aa4b18da814d70bc8 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 7 Sep 2024 00:38:46 +0200 Subject: [PATCH 6/8] Adjust tests --- gson/src/test/java/com/google/gson/GsonTest.java | 2 +- .../google/gson/internal/ConstructorConstructorTest.java | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/gson/src/test/java/com/google/gson/GsonTest.java b/gson/src/test/java/com/google/gson/GsonTest.java index 30a964b5fe..3d09b2ddae 100644 --- a/gson/src/test/java/com/google/gson/GsonTest.java +++ b/gson/src/test/java/com/google/gson/GsonTest.java @@ -175,7 +175,7 @@ public String toString() { + " but got instance of class java.lang.Integer\n" + "Verify that the adapter was registed for the correct type."); - // Returning boxed object should be allowed (e.g. returning `Integer` for `int`) + // 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); 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 f70b063214..7a7c90ecb3 100644 --- a/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java +++ b/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java @@ -179,16 +179,11 @@ public void testStringMapCreation() { // But when explicitly requesting a `LinkedHashMap` should use JDK `LinkedHashMap` actual = - constructorConstructor - .get(TypeToken.getParameterized(LinkedHashMap.class, String.class, Integer.class)) - .construct(); + constructorConstructor.get(new TypeToken>() {}).construct(); assertThat(actual).isInstanceOf(LinkedHashMap.class); // For all Map types with non-String key, should use JDK `LinkedHashMap` by default - actual = - constructorConstructor - .get(TypeToken.getParameterized(Map.class, Integer.class, Integer.class)) - .construct(); + actual = constructorConstructor.get(new TypeToken>() {}).construct(); assertThat(actual).isInstanceOf(LinkedHashMap.class); } From e70a754b9f6be800f03b51102e5eabe08671bd86 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 7 Sep 2024 13:18:04 +0200 Subject: [PATCH 7/8] Don't use LinkedTreeMap for String supertypes as key & small test fix This reverts a previous commit of this pull request, and matches the original behavior again. --- .../gson/internal/ConstructorConstructor.java | 3 +- .../com/google/gson/functional/MapTest.java | 11 +++--- .../internal/ConstructorConstructorTest.java | 35 ++++++++++--------- 3 files changed, 24 insertions(+), 25 deletions(-) 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 5824011514..8a6f29908a 100644 --- a/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java +++ b/gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java @@ -399,8 +399,7 @@ private static boolean hasStringKeyType(Type mapType) { if (typeArguments.length == 0) { return false; } - // Consider String and supertypes of it - return TypeToken.get(typeArguments[0]).getRawType().isAssignableFrom(String.class); + return $Gson$Types.getRawType(typeArguments[0]) == String.class; } // Suppress Error Prone false positive: Pattern is reported for overridden methods, see 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 255ec9dd0c..d2a5d95e59 100644 --- a/gson/src/test/java/com/google/gson/functional/MapTest.java +++ b/gson/src/test/java/com/google/gson/functional/MapTest.java @@ -230,15 +230,13 @@ public void testMapStringKeyDeserialization() { @Test public void testMapStringSupertypeKeyDeserialization() { - // Uses `Object` (= supertype of String) as Map key + // 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 use LinkedTreeMap to protect against DoS in older JDK" - + " versions") + assertWithMessage("Map should not use Gson Map implementation") .that(map) - .isInstanceOf(LinkedTreeMap.class); + .isNotInstanceOf(LinkedTreeMap.class); Map expectedMap = Collections.singletonMap("a", 1); assertThat(map).isEqualTo(expectedMap); @@ -249,7 +247,8 @@ public void testMapNonStringKeyDeserialization() { Type typeOfMap = new TypeToken>() {}.getType(); Map map = gson.fromJson("{\"1\":1}", typeOfMap); - assertThat("non-Map should not use Gson Map implementation") + assertWithMessage("Map should not use Gson Map implementation") + .that(map) .isNotInstanceOf(LinkedTreeMap.class); Map expectedMap = Collections.singletonMap(1, 1); 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 7a7c90ecb3..ad201fb346 100644 --- a/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java +++ b/gson/src/test/java/com/google/gson/internal/ConstructorConstructorTest.java @@ -159,32 +159,33 @@ public void testCustomCollectionInterfaceCreation() { @Test public void testStringMapCreation() { - // When creating raw `Map` should use Gson's `LinkedTreeMap`, assuming keys _could_ be String + // 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` (or where key type is supertype of `String`) should use - // Gson's `LinkedTreeMap` - Class[] stringTypes = {String.class, CharSequence.class, Object.class}; - for (Class stringType : stringTypes) { + // 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, stringType, Integer.class)) + .get(TypeToken.getParameterized(Map.class, keyType, Integer.class)) .construct(); assertWithMessage( - "Failed for key type " + stringType + "; created instance of " + actual.getClass()) + "Failed for key type " + keyType + "; created instance of " + actual.getClass()) .that(actual) - .isInstanceOf(LinkedTreeMap.class); + .isInstanceOf(LinkedHashMap.class); } - - // But when explicitly requesting a `LinkedHashMap` should use JDK `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 - actual = constructorConstructor.get(new TypeToken>() {}).construct(); - assertThat(actual).isInstanceOf(LinkedHashMap.class); } private enum MyEnum {} From 86b13f20711cac403ce98e2aee9f65dd9015b9e2 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Fri, 13 Sep 2024 00:00:26 +0200 Subject: [PATCH 8/8] Fix typo in exception message --- gson/src/main/java/com/google/gson/Gson.java | 2 +- gson/src/test/java/com/google/gson/GsonTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gson/src/main/java/com/google/gson/Gson.java b/gson/src/main/java/com/google/gson/Gson.java index 131ac08865..5562961db3 100644 --- a/gson/src/main/java/com/google/gson/Gson.java +++ b/gson/src/main/java/com/google/gson/Gson.java @@ -1368,7 +1368,7 @@ public T fromJson(JsonReader reader, TypeToken typeOfT) + typeOfT.getRawType() + " but got instance of " + object.getClass() - + "\nVerify that the adapter was registed for the correct type."); + + "\nVerify that the adapter was registered for the correct type."); } return object; } catch (EOFException e) { diff --git a/gson/src/test/java/com/google/gson/GsonTest.java b/gson/src/test/java/com/google/gson/GsonTest.java index 3d09b2ddae..9686db61f3 100644 --- a/gson/src/test/java/com/google/gson/GsonTest.java +++ b/gson/src/test/java/com/google/gson/GsonTest.java @@ -173,7 +173,7 @@ public String toString() { .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 registed for the correct type."); + + "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();