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

Better handle String hashCode collisions in JsLookup #674

Merged
merged 1 commit into from
Nov 23, 2021

Conversation

htmldoug
Copy link
Contributor

@htmldoug htmldoug commented Nov 22, 2021

#186 is still exploitable when JsLookup is used (jsObject \ "foo") or jsObject.value is called directly.

Thankfully, the Reads macro doesn't use either of these approaches. Instead, it extracts fields from jsObject.underlying (typically a java.util.LinkedHashMap). If this approach is good enough for the macro, it seems to make sense to do for JsLookup as well.

jsObject.value is still vulnerable, but there are no remaining references to it internally after this PR.

Before:

[info] Benchmark                                           (n)   Mode  Cnt    Score    Error  Units
[info] JsonParsing_01_ParseManyFields.parseObject        10000  thrpt    3  170.811 ± 24.671  ops/s
[info] JsonParsing_01_ParseManyFields.parseObjectAs      10000  thrpt    3  163.436 ± 13.332  ops/s
[info] JsonParsing_01_ParseManyFields.parseObjectLookup  10000  thrpt    3    1.462 ±  0.077  ops/s
[info] JsonParsing_01_ParseManyFields.parseObjectValue   10000  thrpt    3    1.452 ±  0.144  ops/s

After:

[info] Benchmark                                           (n)   Mode  Cnt    Score    Error  Units
[info] JsonParsing_01_ParseManyFields.parseObject        10000  thrpt    3  174.182 ±  18.880  ops/s
[info] JsonParsing_01_ParseManyFields.parseObjectAs      10000  thrpt    3  158.034 ±  44.744  ops/s
[info] JsonParsing_01_ParseManyFields.parseObjectLookup  10000  thrpt    3  167.274 ±  54.709  ops/s
[info] JsonParsing_01_ParseManyFields.parseObjectValue   10000  thrpt    3    1.451 ±   0.111  ops/s

Relates to #193. @gmethvin @marcospereira

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants