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.

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

Switch from Gson LinkedTreeMap to JDK LinkedHashMap as Map implementation #2737

Open
Marcono1234 opened this issue Sep 7, 2024 · 0 comments
Labels
enhancement java8 Issues related to making Java 8 the minimum supported version

Comments

@Marcono1234
Copy link
Collaborator

Problem solved by the feature

Gson uses its internal LinkedTreeMap as Map implementation when a Map<String, ...> (or a raw Map) should be created. The reasons for this LinkedTreeMap seem to be that old JDK versions were vulnerable to denial-of-service attacks due to hash code collision. See previous discussions in #1992 (comment) (that also covers another Gson Map implementation which we had removed in the meantime) and #2152 (comment).

The problem is that Gson's LinkedTreeMap:

  • Does not permit null keys
  • Requires that keys must be Comparable

This can cause issues such as #1247, but also for less contrived cases where the user tries to add null or non-Comparable to the deserialized map, such as:

@SuppressWarnings("unchecked")
Map<Object, Object> map = new Gson().fromJson("{}", Map.class);
map.put(null, 1);

var map2 = new Gson().fromJson("{}", new TypeToken<Map<String, Integer>>() {});
map2.put(null, 1);

Note: I have added the java8 Issues related to making Java 8 the minimum supported version label because for Java >= 8 we can probably be relatively sure that LinkedTreeMap is not needed as denial-of-service protection, see JDK-8046170.
But we could also already make this change while Gson is still targeting Java 7 as minimum.

Feature description

We should consider not creating Gson's LinkedTreeMap in ConstructorConstructor anymore, but only JDK LinkedHashMap.
(And adjust unit tests which assert isInstanceof(LinkedTreeMap.class) and isNotInstanceof(LinkedTreeMap.class).)

Note: We can probably not completely remove LinkedTreeMap because:

  • External users rely on it (even though they shouldn't because the class is internal); we could mark it as @Deprecated though
  • JsonObject uses it, and especially its asMap method relies on LinkedTreeMap not permitting null keys and values
    Otherwise we would have to write a custom wrapper class which disallows null keys and values (similar to what we have for JsonArray#asList)

This would then:

Alternatives / workarounds

@Marcono1234 Marcono1234 added enhancement java8 Issues related to making Java 8 the minimum supported version labels Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement java8 Issues related to making Java 8 the minimum supported version
Projects
None yet
Development

No branches or pull requests

1 participant