Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix LinkedTreeMap being used for non-Comparable keys #2152

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fix LinkedTreeMap being used for non-Comparable keys #2152

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8e9f94e
Fix LinkedTreeMap being used for non-Comparable keys
Marcono1234 Jul 25, 2022
0496128
Document that JsonObject member names must be non-null
Marcono1234 Jul 25, 2022
3a308a9
Use LinkedHashMap(Map) constructor
Marcono1234 Jul 25, 2022
ec3fc11
Merge branch 'master' into marcono1234/LinkedTreeMap-non-Comparable-keys
Marcono1234 Aug 18, 2022
881d3cc
Don't use LinkedTreeMap for String supertypes; adjust comments
Marcono1234 Aug 18, 2022
aaa1283
Extend test to verify Map<String, ...> uses LinkedTreeMap
Marcono1234 Aug 18, 2022
ebc4511
Merge branch 'master' into marcono1234/LinkedTreeMap-non-Comparable-keys
Marcono1234 Aug 29, 2022
be5a780
Merge branch 'master' into marcono1234/LinkedTreeMap-non-Comparable-keys
Marcono1234 Sep 9, 2022
f6fc354
Merge branch 'master' into marcono1234/LinkedTreeMap-non-Comparable-keys
Marcono1234 Oct 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions gson/src/main/java/com/google/gson/JsonObject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@
import java.util.Set;

/**
* A class representing an object type in Json. An object consists of name-value pairs where names
* A class representing an object type in JSON. An object consists of name-value pairs where names
* are strings, and values are any other type of {@link JsonElement}. This allows for a creating a
* tree of JsonElements. The member elements of this object are maintained in order they were added.
* This class does not support {@code null} values. If {@code null} is provided as value argument
* to any of the methods, it is converted to a {@link JsonNull}.
* Member names must be non-{@code null}. Member values cannot be {@code null} either; if {@code null}
* is provided as value argument to any of the methods, it is converted to a {@link JsonNull}.
*
* @author Inderjeet Singh
* @author Joel Leitch
Expand Down
21 changes: 17 additions & 4 deletions gson/src/main/java/com/google/gson/internal/ConstructorConstructor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,20 @@ public T construct() {
};
}

private static boolean hasStringKeyType(Type mapType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this corresponds to what the logic did before, but I don't think it's correct in general. For example:

public class Class1<T> extends HashMap<Integer, T> // Class1<String> would return true but should not
public class Class2<T> extends HashMap<String, T> // Class2<Integer> would return false but should not
public class Class3 extends HashMap<String, Integer> // ditto

Could we maybe use TypeToken to tell whether Map<String, ?> is assignable from mapType? It might need some adjustment to the logic for wildcards.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what I mean with #2152 (comment) (unfortunately GitHub showed it as "outdated").
hasStringKeyType is really only intended for Map and not any subclasses, to determine whether a LinkedTreeMap should be used. The issue that it is also called for Map subtypes (as shown in your examples) is basically #1708, therefore I am not so keen to add any special logic here.

// If mapType is not parameterized, cannot assume that key is String, because if it
// is not String and does not implement Comparable it would lead to a ClassCastException
if (!(mapType instanceof ParameterizedType)) {
return false;
}
Comment on lines +280 to +284
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would prevent LinkedTreeMap from being used for raw map types, which should be invisible to the user (since LnkedTreeMap is an internal class) but would disable the DoS protection in that case for Java 7. Not sure if that would be acceptable, but on the other hand it prevents potential ClassCastExceptions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of a big change, isn't it? It seems to me that gson.fromJson(s, Map.class) is a pretty common idiom and we're changing what it does. Agreed that people shouldn't be relying on getting a LinkedTreeMap, but is it something they could observe indirectly? For example because of memory usage. Though it looks to me as if LinkedTreeMap is fairly expensive in memory terms because of all those fields in its Node class.

I see some other options. I think the whole situation is a bit of a mess to be honest, but here are some ways we might be able to clean it up a bit:

  1. Keep the approach here: raw Map never deserializes into a LinkedTreeMap, but some parameterized Type instances will. (We can work on adjusting exactly which ones.)
  2. Never deserialize into LinkedTreeMap here. We would still use the class for JsonObject but here we would always use LinkedHashMap.
  3. Give LinkedTreeMap a fallback mode, where if a key is added that is not Comparable, or that can't be compared to existing keys, it copies the existing entries into a private LinkedHashMap and uses that for all subsequent operations.

I suppose my underlying question is why LinkedTreeMap exists in the first place. What problem does it solve? It seems to me that it is usually going to be both bigger and less performant than LinkedHashMap, though I haven't measured that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below where this method hasStringKeyType is called, I switched the if statement branches, which is why it might be a bit confusing.

I think the intended logic here is:
If it is a Map<String, ...> use Gson's LinkedTreeMap to protect against potential DoS for JDK's LinkedHashMap in Java < 8 (JDK-8046170). Otherwise use JDK's LinkedHashMap.

The check below was previously (roughly):

if (type instanceof ParameterizedType && !(String.class.isAssignableFrom(
          TypeToken.get(((ParameterizedType) type).getActualTypeArguments()[0]).getRawType()))) {
    ...
} else {
    return new LinkedTreeMap<>();
}

For raw Map.class and for any parameterized type Map<String, ...> it would have used LinkedTreeMap.
However, using LinkedTreeMap for raw Map.class can lead to ClassCastExceptions, as shown by the examples in #1247.

Therefore the changes by this PR actually reduce the cases where LinkedTreeMap is used (which was the concern I wanted to express with my original comment here).
But if you think that is fine, then I guess we can leave it like this.


Type[] typeArguments = ((ParameterizedType) mapType).getActualTypeArguments();
if (typeArguments.length == 0) {
return false;
}
return TypeToken.get(typeArguments[0]).getRawType() == String.class;
}

/**
* Constructors for common interface types like Map and List and their
* subtypes.
Expand Down Expand Up @@ -320,17 +334,16 @@ private static <T> ObjectConstructor<T> newDefaultImplementationConstructor(
return (T) new TreeMap<>();
}
};
} else if (type instanceof ParameterizedType && !(String.class.isAssignableFrom(
TypeToken.get(((ParameterizedType) type).getActualTypeArguments()[0]).getRawType()))) {
} else if (hasStringKeyType(type)) {
return new ObjectConstructor<T>() {
@Override public T construct() {
return (T) new LinkedHashMap<>();
return (T) new LinkedTreeMap<>();
}
};
} else {
return new ObjectConstructor<T>() {
@Override public T construct() {
return (T) new LinkedTreeMap<>();
return (T) new LinkedHashMap<>();
}
};
}
Expand Down
21 changes: 21 additions & 0 deletions gson/src/main/java/com/google/gson/internal/bind/MapTypeAdapterFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.gson.internal.$Gson$Types;
import com.google.gson.internal.ConstructorConstructor;
import com.google.gson.internal.JsonReaderInternalAccess;
import com.google.gson.internal.LinkedTreeMap;
import com.google.gson.internal.ObjectConstructor;
import com.google.gson.internal.Streams;
import com.google.gson.reflect.TypeToken;
Expand All @@ -34,6 +35,7 @@
import java.io.IOException;
import java.lang.reflect.Type;
import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;

Expand Down Expand Up @@ -164,13 +166,23 @@ public Adapter(Gson context, Type keyType, TypeAdapter<K> keyTypeAdapter,
}

Map<K, V> map = constructor.construct();
boolean isLinkedTreeMap = map instanceof LinkedTreeMap;

if (peek == JsonToken.BEGIN_ARRAY) {
in.beginArray();
while (in.hasNext()) {
in.beginArray(); // entry array
K key = keyTypeAdapter.read(in);
V value = valueTypeAdapter.read(in);

// LinkedTreeMap requires that keys are Comparable, if they are not must switch to
// a different map type. LinkedTreeMap is an internal class so switching map type
// should not be noticeable for user code.
if (isLinkedTreeMap && !(key instanceof Comparable)) {
map = new LinkedHashMap<>(map);
isLinkedTreeMap = false;
}

V replaced = map.put(key, value);
if (replaced != null) {
throw new JsonSyntaxException("duplicate key: " + key);
Expand All @@ -184,6 +196,15 @@ public Adapter(Gson context, Type keyType, TypeAdapter<K> keyTypeAdapter,
JsonReaderInternalAccess.INSTANCE.promoteNameToValue(in);
K key = keyTypeAdapter.read(in);
V value = valueTypeAdapter.read(in);

// LinkedTreeMap requires that keys are Comparable, if they are not must switch to
// a different map type. LinkedTreeMap is an internal class so switching map type
// should not be noticeable for user code.
if (isLinkedTreeMap && !(key instanceof Comparable)) {
map = new LinkedHashMap<>(map);
isLinkedTreeMap = false;
}

V replaced = map.put(key, value);
if (replaced != null) {
throw new JsonSyntaxException("duplicate key: " + key);
Expand Down
147 changes: 132 additions & 15 deletions gson/src/test/java/com/google/gson/functional/MapTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -39,10 +27,27 @@
import com.google.gson.JsonSerializationContext;
import com.google.gson.JsonSerializer;
import com.google.gson.JsonSyntaxException;
import com.google.gson.TypeAdapter;
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 com.google.gson.stream.JsonReader;
import com.google.gson.stream.JsonWriter;
import java.io.IOException;
import java.lang.reflect.Type;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
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;

/**
Expand Down Expand Up @@ -551,7 +556,12 @@ public void testStringKeyDeserialization() {
Map<String, String> map = new LinkedHashMap<>();
map.put("2,3", "a");
map.put("5,7", "b");
assertEquals(map, gson.fromJson(json, new TypeToken<Map<String, String>>() {}.getType()));

Map<String, String> actualMap = gson.fromJson(json, new TypeToken<Map<String, String>>() {}.getType());
// Should use LinkedTreeMap to protect against potential DoS in older Java versions
// See also https://bugs.openjdk.org/browse/JDK-8046170
assertTrue(actualMap instanceof LinkedTreeMap);
assertEquals(map, actualMap);
}

public void testNumberKeyDeserialization() {
Expand Down Expand Up @@ -587,7 +597,7 @@ public void testSerializeMapOfMaps() {
gson.toJson(map, type).replace('"', '\''));
}

public void testDeerializeMapOfMaps() {
public void testDeserializeMapOfMaps() {
Type type = new TypeToken<Map<String, Map<String, String>>>() {}.getType();
Map<String, Map<String, String>> map = newMap(
"a", newMap("ka1", "va1", "ka2", "va2"),
Expand All @@ -596,6 +606,113 @@ public void testDeerializeMapOfMaps() {
assertEquals(map, gson.fromJson(json, type));
}

/**
* Tests deserializing as {@code Map<String, ...>} with a key being deserialized as
* {@code null}. This can only occur if a custom type adapter for {@code String} is
* used.
*/
public void testDeserializeStringMapNullKey() {
Gson gson = new GsonBuilder().registerTypeAdapter(String.class, new TypeAdapter<String>() {
@Override public String read(JsonReader in) throws IOException {
String value = in.nextString();
return value.isEmpty() ? null : value;
}

@Override public void write(JsonWriter out, String value) throws IOException {
throw new AssertionError("not needed for this test");
}
}).create();

String json = "{\"a\":1,\"\":2,\"b\":3}";

Map<String, Integer> expectedMap = new LinkedHashMap<>();
expectedMap.put("a", 1);
expectedMap.put(null, 2);
expectedMap.put("b", 3);

Type type = new TypeToken<Map<String, Integer>>() {}.getType();
assertEquals(expectedMap, gson.fromJson(json, type));
}

/**
* Tests deserializing as {@code Map<String, ...>} with the map being encoded as JSON array
* and containing a {@code null} key.
*/
public void testDeserializeArrayStringMapNullKey() {
String json = "[[\"a\", 1], [null, 2], [\"b\", 3]]";

Map<String, Integer> expectedMap = new LinkedHashMap<>();
expectedMap.put("a", 1);
expectedMap.put(null, 2);
expectedMap.put("b", 3);

Type type = new TypeToken<Map<String, Integer>>() {}.getType();
assertEquals(expectedMap, gson.fromJson(json, type));
}

/**
* Deserialization of array should work, even if {@link GsonBuilder#enableComplexMapKeySerialization()}
* is not used.
*/
public void testDeserializeArray() {
Type type = new TypeToken<Map<Integer, Integer>>() {}.getType();
String json = "[[1, 11], [2, 22]]";
Map<Integer, Integer> expectedMap = newMap(1, 11, 2, 22);
assertEquals(expectedMap, gson.fromJson(json, type));

try {
gson.fromJson("[[1, 11], 2, 22]", type);
fail();
} catch (JsonSyntaxException e) {
assertEquals("java.lang.IllegalStateException: Expected BEGIN_ARRAY but was NUMBER at line 1 column 12 path $[1]", e.getMessage());
}
}

/**
* Tests deserializing as raw {@code Map} with non-{@code Comparable} keys, with the map being
* encoded as JSON array.
*/
public void testDeserializeArrayRawMapNonComparableKey() {
String json = "[[\"a\", 1.0], [{}, 2.0], [\"b\", 3.0]]";

Map<Object, Double> expectedMap = new LinkedHashMap<>();
expectedMap.put("a", 1.0);
expectedMap.put(Collections.emptyMap(), 2.0);
expectedMap.put("b", 3.0);

@SuppressWarnings("unchecked")
Map<Object, Double> map = gson.fromJson(json, Map.class);

assertEquals(expectedMap, map);
// LinkedTreeMap requires that keys implement Comparable so it should not have
// been used
assertFalse(map instanceof LinkedTreeMap);

Iterator<Map.Entry<Object, Double>> iterator = map.entrySet().iterator();
iterator.next();
Map.Entry<Object, Double> entry = iterator.next();
Object key = entry.getKey();
assertEquals(Collections.emptyMap(), key);
// Explicitly check that key is not Comparable
assertFalse(key instanceof Comparable);
assertEquals(2.0, entry.getValue());
}

/**
* Tests deserializing as raw {@code Map} with {@code null} key, with the map being encoded
* as JSON array.
*/
public void testDeserializeArrayRawMapNullKey() {
String json = "[[\"a\", 1.0], [null, 2.0], [\"b\", 3.0]]";

Map<Object, Double> expectedMap = new LinkedHashMap<>();
expectedMap.put("a", 1.0);
expectedMap.put(null, 2.0);
expectedMap.put("b", 3.0);

assertEquals(expectedMap, gson.fromJson(json, Map.class));
}

private <K, V> Map<K, V> newMap(K key1, V value1, K key2, V value2) {
Map<K, V> result = new LinkedHashMap<>();
result.put(key1, value1);
Expand Down