Skip to content

Commit 2549ba9

Browse files
authored
Fix ConstructorConstructor creating mismatching Collection and Map instances (#2068)
* Fix ConstructorConstructor creating mismatching Collection instances * Support EnumMap deserialization * Use LinkedTreeMap for maps with String supertype as key * Fix test method name typo * Fix `ProtoTypeAdapter` being unable to deserialize certain repeated fields For example a `List<Long>` 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. * Adjust tests * 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. * Fix typo in exception message * Adjust ConstructorConstructor for Java 8 * Use lambdas to make it more explicit which constructor overloads are used * Don't use Unsafe to create Collection and Map instances
1 parent f323240 commit 2549ba9

File tree

11 files changed

+548
-46
lines changed

11 files changed

+548
-46
lines changed

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

+16-7
Original file line numberDiff line numberDiff line change
@@ -1104,8 +1104,7 @@ public JsonReader newJsonReader(Reader reader) {
11041104
* @see #fromJson(String, TypeToken)
11051105
*/
11061106
public <T> T fromJson(String json, Class<T> classOfT) throws JsonSyntaxException {
1107-
T object = fromJson(json, TypeToken.get(classOfT));
1108-
return Primitives.wrap(classOfT).cast(object);
1107+
return fromJson(json, TypeToken.get(classOfT));
11091108
}
11101109

11111110
/**
@@ -1196,8 +1195,7 @@ public <T> T fromJson(String json, TypeToken<T> typeOfT) throws JsonSyntaxExcept
11961195
*/
11971196
public <T> T fromJson(Reader json, Class<T> classOfT)
11981197
throws JsonSyntaxException, JsonIOException {
1199-
T object = fromJson(json, TypeToken.get(classOfT));
1200-
return Primitives.wrap(classOfT).cast(object);
1198+
return fromJson(json, TypeToken.get(classOfT));
12011199
}
12021200

12031201
/**
@@ -1358,7 +1356,19 @@ public <T> T fromJson(JsonReader reader, TypeToken<T> typeOfT)
13581356
JsonToken unused = reader.peek();
13591357
isEmpty = false;
13601358
TypeAdapter<T> typeAdapter = getAdapter(typeOfT);
1361-
return typeAdapter.read(reader);
1359+
T object = typeAdapter.read(reader);
1360+
Class<?> expectedTypeWrapped = Primitives.wrap(typeOfT.getRawType());
1361+
if (object != null && !expectedTypeWrapped.isInstance(object)) {
1362+
throw new ClassCastException(
1363+
"Type adapter '"
1364+
+ typeAdapter
1365+
+ "' returned wrong type; requested "
1366+
+ typeOfT.getRawType()
1367+
+ " but got instance of "
1368+
+ object.getClass()
1369+
+ "\nVerify that the adapter was registered for the correct type.");
1370+
}
1371+
return object;
13621372
} catch (EOFException e) {
13631373
/*
13641374
* 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)
14031413
* @see #fromJson(JsonElement, TypeToken)
14041414
*/
14051415
public <T> T fromJson(JsonElement json, Class<T> classOfT) throws JsonSyntaxException {
1406-
T object = fromJson(json, TypeToken.get(classOfT));
1407-
return Primitives.wrap(classOfT).cast(object);
1416+
return fromJson(json, TypeToken.get(classOfT));
14081417
}
14091418

14101419
/**

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

+99-33
Original file line numberDiff line numberDiff line change
@@ -36,15 +36,9 @@
3636
import java.util.LinkedHashSet;
3737
import java.util.List;
3838
import java.util.Map;
39-
import java.util.Queue;
40-
import java.util.Set;
41-
import java.util.SortedMap;
42-
import java.util.SortedSet;
4339
import java.util.TreeMap;
4440
import java.util.TreeSet;
4541
import java.util.concurrent.ConcurrentHashMap;
46-
import java.util.concurrent.ConcurrentMap;
47-
import java.util.concurrent.ConcurrentNavigableMap;
4842
import java.util.concurrent.ConcurrentSkipListMap;
4943

5044
/** Returns a function that can construct an instance of a requested type. */
@@ -94,7 +88,19 @@ static String checkInstantiable(Class<?> c) {
9488
return null;
9589
}
9690

91+
/** Calls {@link #get(TypeToken, boolean)}, and allows usage of JDK Unsafe. */
9792
public <T> ObjectConstructor<T> get(TypeToken<T> typeToken) {
93+
return get(typeToken, true);
94+
}
95+
96+
/**
97+
* Retrieves an object constructor for the given type.
98+
*
99+
* @param typeToken type for which a constructor should be retrieved
100+
* @param allowUnsafe whether to allow usage of JDK Unsafe; has no effect if {@link #useJdkUnsafe}
101+
* is false
102+
*/
103+
public <T> ObjectConstructor<T> get(TypeToken<T> typeToken, boolean allowUnsafe) {
98104
Type type = typeToken.getType();
99105
Class<? super T> rawType = typeToken.getRawType();
100106

@@ -142,12 +148,19 @@ public <T> ObjectConstructor<T> get(TypeToken<T> typeToken) {
142148
};
143149
}
144150

151+
if (!allowUnsafe) {
152+
String message =
153+
"Unable to create instance of "
154+
+ rawType
155+
+ "; Register an InstanceCreator or a TypeAdapter for this type.";
156+
return () -> {
157+
throw new JsonIOException(message);
158+
};
159+
}
160+
145161
// Consider usage of Unsafe as reflection, so don't use if BLOCK_ALL
146162
// Additionally, since it is not calling any constructor at all, don't use if BLOCK_INACCESSIBLE
147-
if (filterResult == FilterResult.ALLOW) {
148-
// finally try unsafe
149-
return newUnsafeAllocator(rawType);
150-
} else {
163+
if (filterResult != FilterResult.ALLOW) {
151164
String message =
152165
"Unable to create instance of "
153166
+ rawType
@@ -158,6 +171,9 @@ public <T> ObjectConstructor<T> get(TypeToken<T> typeToken) {
158171
throw new JsonIOException(message);
159172
};
160173
}
174+
175+
// finally try unsafe
176+
return newUnsafeAllocator(rawType);
161177
}
162178

163179
/**
@@ -290,7 +306,6 @@ private static <T> ObjectConstructor<T> newDefaultConstructor(
290306
}
291307

292308
/** Constructors for common interface types like Map and List and their subtypes. */
293-
@SuppressWarnings("unchecked") // use runtime checks to guarantee that 'T' is what it is
294309
private static <T> ObjectConstructor<T> newDefaultImplementationConstructor(
295310
Type type, Class<? super T> rawType) {
296311

@@ -303,33 +318,84 @@ private static <T> ObjectConstructor<T> newDefaultImplementationConstructor(
303318
*/
304319

305320
if (Collection.class.isAssignableFrom(rawType)) {
306-
if (SortedSet.class.isAssignableFrom(rawType)) {
307-
return () -> (T) new TreeSet<>();
308-
} else if (Set.class.isAssignableFrom(rawType)) {
309-
return () -> (T) new LinkedHashSet<>();
310-
} else if (Queue.class.isAssignableFrom(rawType)) {
311-
return () -> (T) new ArrayDeque<>();
312-
} else {
313-
return () -> (T) new ArrayList<>();
314-
}
321+
@SuppressWarnings("unchecked")
322+
ObjectConstructor<T> constructor = (ObjectConstructor<T>) newCollectionConstructor(rawType);
323+
return constructor;
315324
}
316325

317326
if (Map.class.isAssignableFrom(rawType)) {
318-
if (ConcurrentNavigableMap.class.isAssignableFrom(rawType)) {
319-
return () -> (T) new ConcurrentSkipListMap<>();
320-
} else if (ConcurrentMap.class.isAssignableFrom(rawType)) {
321-
return () -> (T) new ConcurrentHashMap<>();
322-
} else if (SortedMap.class.isAssignableFrom(rawType)) {
323-
return () -> (T) new TreeMap<>();
324-
} else if (type instanceof ParameterizedType
325-
&& !String.class.isAssignableFrom(
326-
TypeToken.get(((ParameterizedType) type).getActualTypeArguments()[0]).getRawType())) {
327-
return () -> (T) new LinkedHashMap<>();
328-
} else {
329-
return () -> (T) new LinkedTreeMap<>();
330-
}
327+
@SuppressWarnings("unchecked")
328+
ObjectConstructor<T> constructor = (ObjectConstructor<T>) newMapConstructor(type, rawType);
329+
return constructor;
330+
}
331+
332+
// Unsupported type; try other means of creating constructor
333+
return null;
334+
}
335+
336+
private static ObjectConstructor<? extends Collection<? extends Object>> newCollectionConstructor(
337+
Class<?> rawType) {
338+
339+
// First try List implementation
340+
if (rawType.isAssignableFrom(ArrayList.class)) {
341+
return () -> new ArrayList<>();
342+
}
343+
// Then try Set implementation
344+
else if (rawType.isAssignableFrom(LinkedHashSet.class)) {
345+
return () -> new LinkedHashSet<>();
346+
}
347+
// Then try SortedSet / NavigableSet implementation
348+
else if (rawType.isAssignableFrom(TreeSet.class)) {
349+
return () -> new TreeSet<>();
350+
}
351+
// Then try Queue implementation
352+
else if (rawType.isAssignableFrom(ArrayDeque.class)) {
353+
return () -> new ArrayDeque<>();
354+
}
355+
356+
// Was unable to create matching Collection constructor
357+
return null;
358+
}
359+
360+
private static boolean hasStringKeyType(Type mapType) {
361+
// If mapType is not parameterized, assume it might have String as key type
362+
if (!(mapType instanceof ParameterizedType)) {
363+
return true;
364+
}
365+
366+
Type[] typeArguments = ((ParameterizedType) mapType).getActualTypeArguments();
367+
if (typeArguments.length == 0) {
368+
return false;
369+
}
370+
return $Gson$Types.getRawType(typeArguments[0]) == String.class;
371+
}
372+
373+
private static ObjectConstructor<? extends Map<? extends Object, Object>> newMapConstructor(
374+
Type type, Class<?> rawType) {
375+
// First try Map implementation
376+
/*
377+
* Legacy special casing for Map<String, ...> to avoid DoS from colliding String hashCode
378+
* values for older JDKs; use own LinkedTreeMap<String, Object> instead
379+
*/
380+
if (rawType.isAssignableFrom(LinkedTreeMap.class) && hasStringKeyType(type)) {
381+
return () -> new LinkedTreeMap<>();
382+
} else if (rawType.isAssignableFrom(LinkedHashMap.class)) {
383+
return () -> new LinkedHashMap<>();
384+
}
385+
// Then try SortedMap / NavigableMap implementation
386+
else if (rawType.isAssignableFrom(TreeMap.class)) {
387+
return () -> new TreeMap<>();
388+
}
389+
// Then try ConcurrentMap implementation
390+
else if (rawType.isAssignableFrom(ConcurrentHashMap.class)) {
391+
return () -> new ConcurrentHashMap<>();
392+
}
393+
// Then try ConcurrentNavigableMap implementation
394+
else if (rawType.isAssignableFrom(ConcurrentSkipListMap.class)) {
395+
return () -> new ConcurrentSkipListMap<>();
331396
}
332397

398+
// Was unable to create matching Map constructor
333399
return null;
334400
}
335401

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

+4-1
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,10 @@ public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> typeToken) {
5151
TypeAdapter<?> elementTypeAdapter = gson.getAdapter(TypeToken.get(elementType));
5252
TypeAdapter<?> wrappedTypeAdapter =
5353
new TypeAdapterRuntimeTypeWrapper<>(gson, elementTypeAdapter, elementType);
54-
ObjectConstructor<T> constructor = constructorConstructor.get(typeToken);
54+
// Don't allow Unsafe usage to create instance; instances might be in broken state and calling
55+
// Collection methods could lead to confusing exceptions
56+
boolean allowUnsafe = false;
57+
ObjectConstructor<T> constructor = constructorConstructor.get(typeToken, allowUnsafe);
5558

5659
@SuppressWarnings({"unchecked", "rawtypes"}) // create() doesn't define a type parameter
5760
TypeAdapter<T> result = new Adapter(wrappedTypeAdapter, constructor);

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

+4-1
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,10 @@ private static Object createAdapter(
9090
// TODO: The exception messages created by ConstructorConstructor are currently written in the
9191
// context of deserialization and for example suggest usage of TypeAdapter, which would not work
9292
// for @JsonAdapter usage
93-
return constructorConstructor.get(TypeToken.get(adapterClass)).construct();
93+
// TODO: Should probably not allow usage of Unsafe; instances might be in broken state and
94+
// calling adapter methods on them might lead to confusing exceptions
95+
boolean allowUnsafe = true;
96+
return constructorConstructor.get(TypeToken.get(adapterClass), allowUnsafe).construct();
9497
}
9598

9699
private TypeAdapterFactory putFactoryAndGetCurrent(Class<?> rawType, TypeAdapterFactory factory) {

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

+4-1
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,10 @@ public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> typeToken) {
140140
TypeAdapter<?> valueAdapter = gson.getAdapter(TypeToken.get(valueType));
141141
TypeAdapter<?> wrappedValueAdapter =
142142
new TypeAdapterRuntimeTypeWrapper<>(gson, valueAdapter, valueType);
143-
ObjectConstructor<T> constructor = constructorConstructor.get(typeToken);
143+
// Don't allow Unsafe usage to create instance; instances might be in broken state and calling
144+
// Map methods could lead to confusing exceptions
145+
boolean allowUnsafe = false;
146+
ObjectConstructor<T> constructor = constructorConstructor.get(typeToken, allowUnsafe);
144147

145148
@SuppressWarnings({"unchecked", "rawtypes"})
146149
// we don't define a type parameter for the key or value types

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ public String toString() {
156156
return adapter;
157157
}
158158

159-
ObjectConstructor<T> constructor = constructorConstructor.get(type);
159+
ObjectConstructor<T> constructor = constructorConstructor.get(type, true);
160160
return new FieldReflectionAdapter<>(
161161
constructor, getBoundFields(gson, type, raw, blockInaccessible, false));
162162
}

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

+55
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,61 @@ public Object read(JsonReader in) {
134134
}
135135
}
136136

137+
@Test
138+
public void testFromJson_WrongResultType() {
139+
class IntegerAdapter extends TypeAdapter<Integer> {
140+
@Override
141+
public Integer read(JsonReader in) throws IOException {
142+
in.skipValue();
143+
return 3;
144+
}
145+
146+
@Override
147+
public void write(JsonWriter out, Integer value) {
148+
throw new AssertionError("not needed for test");
149+
}
150+
151+
@Override
152+
public String toString() {
153+
return "custom-adapter";
154+
}
155+
}
156+
157+
Gson gson = new GsonBuilder().registerTypeAdapter(Boolean.class, new IntegerAdapter()).create();
158+
// Use `Class<?>` here to avoid that the JVM itself creates the ClassCastException (though the
159+
// check below for the custom message would detect that as well)
160+
Class<?> deserializedClass = Boolean.class;
161+
var exception =
162+
assertThrows(ClassCastException.class, () -> gson.fromJson("true", deserializedClass));
163+
assertThat(exception)
164+
.hasMessageThat()
165+
.isEqualTo(
166+
"Type adapter 'custom-adapter' returned wrong type; requested class java.lang.Boolean"
167+
+ " but got instance of class java.lang.Integer\n"
168+
+ "Verify that the adapter was registered for the correct type.");
169+
170+
// Returning boxed primitive should be allowed (e.g. returning `Integer` for `int`)
171+
Gson gson2 = new GsonBuilder().registerTypeAdapter(int.class, new IntegerAdapter()).create();
172+
assertThat(gson2.fromJson("0", int.class)).isEqualTo(3);
173+
174+
class NullAdapter extends TypeAdapter<Object> {
175+
@Override
176+
public Object read(JsonReader in) throws IOException {
177+
in.skipValue();
178+
return null;
179+
}
180+
181+
@Override
182+
public void write(JsonWriter out, Object value) {
183+
throw new AssertionError("not needed for test");
184+
}
185+
}
186+
187+
// Returning `null` should be allowed
188+
Gson gson3 = new GsonBuilder().registerTypeAdapter(Boolean.class, new NullAdapter()).create();
189+
assertThat(gson3.fromJson("true", Boolean.class)).isNull();
190+
}
191+
137192
@Test
138193
public void testGetAdapter_Null() {
139194
Gson gson = new Gson();

0 commit comments

Comments
 (0)