Skip to content

Commit

Permalink
Bacport #2331 in 2.9 (for 2.9.10)
Browse files Browse the repository at this point in the history
  • Loading branch information
@cowtowncoder
cowtowncoder committed Jul 24, 2019
1 parent 9fe6c6f commit dd4c5ac
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 0 deletions.
1 change: 1 addition & 0 deletions release-notes/VERSION-2.x
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ Project: jackson-databind

2.9.9.1 (03-Jul-2019)

#2331: `JsonMappingException` through nested getter with generic wildcard return type
#2334: Block one more gadget type (CVE-2019-12384)
#2341: Block one more gadget type (CVE-2019-12814)
#2374: `ObjectMapper. getRegisteredModuleIds()` throws NPE if no modules registered
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/com/fasterxml/jackson/databind/type/ResolvedRecursiveType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ public JavaType getSuperClass() {

public JavaType getSelfReferencedType() { return _referencedType; }

// 23-Jul-2019, tatu: [databind#2331] Need to also delegate this...
@Override
public TypeBindings getBindings() {
return _referencedType.getBindings();
}

@Override
public StringBuilder getGenericSignature(StringBuilder sb) {
return _referencedType.getGenericSignature(sb);
Expand Down

6 comments on commit dd4c5ac

@chadlwilson
Copy link

Choose a reason for hiding this comment

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

Hi @cowtowncoder, this change on 2.9 branch to address #2331 seems to have caused some kind of regression for us in 2.9.9.2 - the changelog has it in 2.9.9.1 per above, however I believe it's actually been released in 2.9.9.2, although the intention was probably for 2.9.10 based on the commit comment.

We started getting NPEs on this exact line after this bump

Haven't dug into root cause and reproduced, as I'm not super-familiar with Jackson internals, but we're seeing it on our use with spring-data-redis's Jackson2HashMapper in this line here

java.lang.NullPointerException
	at com.fasterxml.jackson.databind.type.ResolvedRecursiveType.getBindings(ResolvedRecursiveType.java:42)
	at com.fasterxml.jackson.databind.type.TypeFactory._fromAny(TypeFactory.java:1243)
	at com.fasterxml.jackson.databind.type.TypeFactory._fromParamType(TypeFactory.java:1452)
	at com.fasterxml.jackson.databind.type.TypeFactory._fromAny(TypeFactory.java:1221)
	at com.fasterxml.jackson.databind.type.TypeFactory._resolveSuperInterfaces(TypeFactory.java:1367)
	at com.fasterxml.jackson.databind.type.TypeFactory._fromClass(TypeFactory.java:1314)
	at com.fasterxml.jackson.databind.type.TypeFactory._fromAny(TypeFactory.java:1217)
	at com.fasterxml.jackson.databind.type.TypeFactory.constructType(TypeFactory.java:631)
	at com.fasterxml.jackson.databind.cfg.MapperConfig.constructType(MapperConfig.java:281)
	at com.fasterxml.jackson.databind.cfg.MapperConfig.introspectClassAnnotations(MapperConfig.java:311)
	at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.findTypeDeserializer(BasicDeserializerFactory.java:1571)
	at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:483)
	at com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:4191)
	at com.fasterxml.jackson.databind.ObjectMapper._readValue(ObjectMapper.java:3980)
	at com.fasterxml.jackson.databind.ObjectMapper.readTree(ObjectMapper.java:2385)
	at com.fasterxml.jackson.databind.ObjectMapper.valueToTree(ObjectMapper.java:2800)
	at org.springframework.data.redis.hash.Jackson2HashMapper.toHash(Jackson2HashMapper.java:134)
        at <REDACTED>

ResolvedRecursiveType is a JsonNode with no _referencedType

this = {ResolvedRecursiveType@3886} "[recursive type; UNRESOLVED"
 _referencedType = null
 _superClass = null
 _superInterfaces = null
 _bindings = {TypeBindings@3896} "<>"
 _canonicalName = null
 _class = {Class@3897} "class com.fasterxml.jackson.databind.JsonNode"
 _hash = -887947255
 _valueHandler = null
 _typeHandler = null
 _asStatic = false
_referencedType = null

Any chance of reverting this so we can pick up the CVE fixes, and relook at this for 2.9.10? Let me know if you'd like me to file an issue and what additional detail would be useful.

@cowtowncoder
Copy link
Member Author

Choose a reason for hiding this comment

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

@chadlwilson Ok first of all thank you for reporting this & apologies for breakages. I usually follow fairly conservative approach to fixes late in patch cycle so in hindsight would have made sense to only fix in 2.10.

But one think that'd be great to have would be reproduction, as obviously none of the tests we have catches this?

As to revert, yes, that probably makes sense even tho I think fix/work-around of checking for nulls (I think this is just because resolved type has initial state of not resolved) probably handles it. But there may be more to it.

I probably should do 2.9.9.3, yes, will see how timing looks like.

But one thing first: could you file a new issue for this if you haven't already?

@chadlwilson
Copy link

Choose a reason for hiding this comment

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

Thanks - I haven't raised one but will.

@michael-simons
Copy link

Choose a reason for hiding this comment

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

Neo4j-OGM HTTP Transport fails with NPE, too. @chadlwilson Did you already create an issue?

@michael-simons
Copy link

Choose a reason for hiding this comment

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

I created a ticket: #2395 Happens when the Jdk8 module is registered, too.

@chadlwilson
Copy link

Choose a reason for hiding this comment

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

Apologies for the delay - and thanks for raising a ticket @michael-simons.

Please sign in to comment.