Array index out of bounds in hex lookup

[emilyselwood]

Fixing an index out of bounds if a negative number manages to get here. Found two paths that were able to trigger this by fuzzing.

[cowtowncoder]

Would it be possible to trigger those 2 paths, to guard against regression? I should be able to track it back without big problems (not commonly used path), but might help if you happen to have stack traces.

[emilyselwood]

I can add the two inputs that caused problems, they are not reduced, but one is pretty small. Where would be the best place to add tests? I was calling in through ObjectMapper.readTree

Stack traces follow:

java.lang.ArrayIndexOutOfBoundsException: Index -63 out of bounds for length 128
	at com.fasterxml.jackson.core.io.CharTypes.charToHex(CharTypes.java:215)
	at com.fasterxml.jackson.core.json.UTF8StreamJsonParser._decodeEscaped(UTF8StreamJsonParser.java:3261)
	at com.fasterxml.jackson.core.json.UTF8StreamJsonParser.parseEscapedName(UTF8StreamJsonParser.java:1910)
	at com.fasterxml.jackson.core.json.UTF8StreamJsonParser.slowParseName(UTF8StreamJsonParser.java:1867)
	at com.fasterxml.jackson.core.json.UTF8StreamJsonParser._parseName(UTF8StreamJsonParser.java:1651)
	at com.fasterxml.jackson.core.json.UTF8StreamJsonParser.nextFieldName(UTF8StreamJsonParser.java:1005)
	at com.fasterxml.jackson.databind.deser.std.BaseNodeDeserializer.deserializeObject(JsonNodeDeserializer.java:246)
	at com.fasterxml.jackson.databind.deser.std.JsonNodeDeserializer.deserialize(JsonNodeDeserializer.java:68)
	at com.fasterxml.jackson.databind.deser.std.JsonNodeDeserializer.deserialize(JsonNodeDeserializer.java:15)
	at com.fasterxml.jackson.databind.ObjectMapper._readTreeAndClose(ObjectMapper.java:4056)
	at com.fasterxml.jackson.databind.ObjectMapper.readTree(ObjectMapper.java:2571)
	at org.catapult.sa.fuzz.jackson.FuzzJackson.test(FuzzJackson.java:15)
	at org.catapult.sa.tribble.Fuzzer.$anonfun$runOnce$1(Fuzzer.scala:192)
	at scala.concurrent.Future$.$anonfun$apply$1(Future.scala:658)
	at scala.util.Success.$anonfun$map$1(Try.scala:255)
	at scala.util.Success.map(Try.scala:213)
	at scala.concurrent.Future.$anonfun$map$1(Future.scala:292)
	at scala.concurrent.impl.Promise.liftedTree1$1(Promise.scala:33)
	at scala.concurrent.impl.Promise.$anonfun$transform$1(Promise.scala:33)
	at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:64)
	at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1426)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:177)

java.lang.ArrayIndexOutOfBoundsException: Index -60 out of bounds for length 128
	at com.fasterxml.jackson.core.io.CharTypes.charToHex(CharTypes.java:215)
	at com.fasterxml.jackson.core.json.UTF8StreamJsonParser._decodeEscaped(UTF8StreamJsonParser.java:3261)
	at com.fasterxml.jackson.core.json.UTF8StreamJsonParser._finishString2(UTF8StreamJsonParser.java:2458)
	at com.fasterxml.jackson.core.json.UTF8StreamJsonParser._finishAndReturnString(UTF8StreamJsonParser.java:2413)
	at com.fasterxml.jackson.core.json.UTF8StreamJsonParser.getText(UTF8StreamJsonParser.java:269)
	at com.fasterxml.jackson.databind.deser.std.BaseNodeDeserializer.deserializeAny(JsonNodeDeserializer.java:523)
	at com.fasterxml.jackson.databind.deser.std.JsonNodeDeserializer.deserialize(JsonNodeDeserializer.java:73)
	at com.fasterxml.jackson.databind.deser.std.JsonNodeDeserializer.deserialize(JsonNodeDeserializer.java:15)
	at com.fasterxml.jackson.databind.ObjectMapper._readTreeAndClose(ObjectMapper.java:4056)
	at com.fasterxml.jackson.databind.ObjectMapper.readTree(ObjectMapper.java:2571)
	at org.catapult.sa.fuzz.jackson.FuzzJackson.test(FuzzJackson.java:15)
	at org.catapult.sa.tribble.Fuzzer.$anonfun$runOnce$1(Fuzzer.scala:192)
	at scala.concurrent.Future$.$anonfun$apply$1(Future.scala:658)
	at scala.util.Success.$anonfun$map$1(Try.scala:255)
	at scala.util.Success.map(Try.scala:213)
	at scala.concurrent.Future.$anonfun$map$1(Future.scala:292)
	at scala.concurrent.impl.Promise.liftedTree1$1(Promise.scala:33)
	at scala.concurrent.impl.Promise.$anonfun$transform$1(Promise.scala:33)
	at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:64)
	at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1426)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:177)

[cowtowncoder]

Thanks! Interesting -- I had a look at 2.10, and didn't think this path was problematic -- though there are couple via NonBlockingJsonParser.

Test would be best triggered through jackson-core itself. It shouldn't be too difficult for me create synthetic one now that I know the path, although of course if you want to try that's fine too.
Main thing being that this is byte-backed input so needs InputStream or byte[] (different code path for Reader / String / char[]).

[cowtowncoder]

Ah! I think this might be what #540 was about?

But given that this is unlikely to be on critical performance path, I think maybe I really should add masking in helper method itself to reduce likelihood of any regression. I mean, while it may be extra work in some cases it really won't have any measurable impact.
And like I mentined, non-blocking parser actually does seem to have paths where masking by 0xFF isn't done and same problem would be triggered.

[cowtowncoder]

On Fri, Nov 8, 2019 at 9:53 AM Andriy Plokhotnyuk ***@***.***> wrote: Why not just use sHexValues[ch & 0x7f] instead branching?

Yes, in the end that makes sense. Added notes to that effect. Was just avoiding as unnecessary optimization :)

[cowtowncoder]

So, ended up changing implementation to be bit more robust; merged the test so should be all good.